[clang] [clang] [MinGW] Handle linking ARM64EC code (PR #78912)
https://github.com/mstorsjo approved this pull request. LGTM, thanks for adding the test! https://github.com/llvm/llvm-project/pull/78912 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] [MinGW] Handle linking ARM64EC code (PR #78912)
https://github.com/mstorsjo commented: Code wise, this seems good, but I think we'd like to have a testcase for it. https://github.com/llvm/llvm-project/pull/78912 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-repl] Fix PLT offset too large linker error on ARM (PR #78959)
mstorsjo wrote: > > I don't really know more about the issue that requires --long-plt at the > > moment and why it's only needed for clang-repl > > clang-repl binary size is ~3.7G in debug mode and this seems to exceed the > branch range of default ARM PLT slots. The instruction sequence that's > necessary for long-range PLT slots is likely more expensive. I guess this > situation is too much of an edge-case to make the effort and detect it > upfront, so they just bail out if it happens an ask for the flag. Right, I see. Though - why is this only an issue for the clang-repl binary and not the other clang-based ones? Does it happen to hit some maximum when it uses both everything that is in normal clang, plus all the JIT stuff? (Normally I would expect LLDB to be the largest one, as it includes much of Clang, but that's also always split into a separate liblldb.so.) https://github.com/llvm/llvm-project/pull/78959 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-repl] Fix PLT offset too large linker error on ARM (PR #78959)
mstorsjo wrote: > Oh, I usually don't do that, but it's certainly a valid point. Can you think > of a better way to express the condition here? We need `-Wl,--long-plt` for > ARM targets whenever the used linker supports it. Otherwise we have to assume > that it emits such PLTs by default. Not sure; architecture detection in CMake generally is problematic. It's possible to infer the actual real destination architecture with a compiler test (like compiling and testing if `__arm__` is defined). CMake doesn't really provide anything out of the box for that though (it does provide `CMAKE_C_COMPILER_ARCHITECTURE_ID` on MSVC like compilers, but not elsewhere, see https://gitlab.kitware.com/cmake/cmake/-/issues/17702), and it feels like overkill to add such a compile test here. Also note that `CMAKE_SYSTEM_PROCESSOR` is unclear in Darwin universal builds anyway - if I'm compiling with `-DCMAKE_OSX_ARCHITECTURES=x86_64;arm64` what will be set in `CMAKE_SYSTEM_PROCESSOR`? :-) I think this current form of the check might be ok enough (I don't really know more about the issue that requires `--long-plt` at the moment and why it's only needed for clang-repl). If you'd want it to hit more universally, perhaps just remove the architecture check - if we test that the linker does support `--long-plt` without erroring out, we probably can add it, even if we don't really know the exact architecture we're linking for? https://github.com/llvm/llvm-project/pull/78959 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-repl] Fix PLT offset too large linker error on ARM (PR #78959)
mstorsjo wrote: > > When cross compiling LLVM, I never have set `CMAKE_SYSTEM_PROCESSOR` so > > far, since we don't really have anything that uses it (before this), which > > means that this expands to an empty string. I guess I should set it still > > though. > > Yes, I am just getting used to it as well. I think it's worth noting that > this should be set in a toolchain file, because CMake seems to have special > handling for them. When I pass `-DCMAKE_SYSTEM_PROCESSOR=ARM` at > configuration time, it gets overwritten with my host arch. I think that's (somewhat?) expected. When you're not cross compiling, cmake explicitly sets `CMAKE_SYSTEM_PROCESSOR` to `CMAKE_HOST_SYSTEM_PROCESSOR`, ignoring whatever you set manually. (Dunno if it would make any difference if you'd set it in a toolchain file.) Only if you're cross compiling (which CMake defines as if you're setting `CMAKE_SYSTEM_NAME`, regardless if cross compiling from Linux to Linux on another arch), it doesn't set it and passes whatever you set originally through. This actually makes `CMAKE_SYSTEM_PROCESSOR` a bit problematic; if you're on x86_64 but targeting i386, or on aarch64 but targeting arm, you might not want to consider it a cross build (as you can execute the build products) but CMake then will hide whatever you're really targeting with the info gathered from the host environment. https://github.com/llvm/llvm-project/pull/78959 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] e3d73ad - [clang-repl] Fix CMake errors when cross compiling
Author: Martin Storsjö
Date: 2024-01-23T13:42:24+02:00
New Revision: e3d73ad58c41b945d9fc5d5fb16ea44850ccf652
URL:
https://github.com/llvm/llvm-project/commit/e3d73ad58c41b945d9fc5d5fb16ea44850ccf652
DIFF:
https://github.com/llvm/llvm-project/commit/e3d73ad58c41b945d9fc5d5fb16ea44850ccf652.diff
LOG: [clang-repl] Fix CMake errors when cross compiling
These stem from 4821c90c24d52d4a42990fd9371caedb157bc58b.
When cross compiling, CMAKE_SYSTEM_PROCESSOR is empty, if the
target processor hasn't been set when setting up the cross
compilation. Ideally, when setting up cross compilation with
CMake, the user is supposed to set CMAKE_SYSTEM_PROCESSOR, but
so far, compilation has worked just fine even without it.
Quote the string where CMAKE_SYSTEM_PROCESSOR is expanded, to
avoid argument errors when it expands to an empty string.
Added:
Modified:
clang/tools/clang-repl/CMakeLists.txt
Removed:
diff --git a/clang/tools/clang-repl/CMakeLists.txt
b/clang/tools/clang-repl/CMakeLists.txt
index 2a0f617a2c0ff6..031dcaba5e4468 100644
--- a/clang/tools/clang-repl/CMakeLists.txt
+++ b/clang/tools/clang-repl/CMakeLists.txt
@@ -23,7 +23,7 @@ if(CLANG_PLUGIN_SUPPORT)
export_executable_symbols_for_plugins(clang-repl)
endif()
-string(TOUPPER ${CMAKE_SYSTEM_PROCESSOR} system_processor)
+string(TOUPPER "${CMAKE_SYSTEM_PROCESSOR}" system_processor)
if(system_processor MATCHES "ARM")
set(FLAG_LONG_PLT "-Wl,--long-plt")
llvm_check_linker_flag(CXX ${FLAG_LONG_PLT} LINKER_HAS_FLAG_LONG_PLT)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][AArch64] Define __USER_LABEL_PREFIX__ to # for ARM64EC (PR #78913)
@@ -1462,10 +1462,12 @@ WindowsARM64TargetInfo::WindowsARM64TargetInfo(const
llvm::Triple &Triple,
}
void WindowsARM64TargetInfo::setDataLayout() {
- resetDataLayout(Triple.isOSBinFormatMachO()
- ? "e-m:o-i64:64-i128:128-n32:64-S128"
- : "e-m:w-p:64:64-i32:32-i64:64-i128:128-n32:64-S128",
- Triple.isOSBinFormatMachO() ? "_" : "");
+ if (Triple.isOSBinFormatMachO()) {
+resetDataLayout("e-m:o-i64:64-i128:128-n32:64-S128", "_");
mstorsjo wrote:
AFAIK Windows+MachO is used in some cases for JIT scenarios, because the
runtime linking support in the JIT is much more complete for MachO than it is
for PE/COFF.
https://github.com/llvm/llvm-project/pull/78913
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] [Driver] Treat MuslEABIHF as a hardfloat environment wrt multiarch directories (PR #77536)
https://github.com/mstorsjo closed https://github.com/llvm/llvm-project/pull/77536 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] [Driver] Treat MuslEABIHF as a hardfloat environment wrt multiarch directories (PR #77536)
mstorsjo wrote: > `bool isEABIHF` from clang/lib/CodeGen/Targets/ARM.cpp can probably be > factored. Yep - any suggestion on where we could move it? Up to the `Triple` class? https://github.com/llvm/llvm-project/pull/77536 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[libunwind] [libunwind] Convert a few options from CACHE PATH to CACHE STRING (PR #77534)
https://github.com/mstorsjo closed https://github.com/llvm/llvm-project/pull/77534 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] [Driver] Treat MuslEABIHF as a hardfloat environment wrt multiarch directories (PR #77536)
https://github.com/mstorsjo created
https://github.com/llvm/llvm-project/pull/77536
If using multiarch directories with musl, the multiarch directory still uses
*-linux-gnu triples - which may or may not be intentional, while it is somewhat
consistent at least.
However, for musl armhf targets, make sure that this also picks
arm-linux-gnueabihf, rather than arm-linux-gnueabi.
From f715cfc716e5a4bacdc0ef041f6a53c6821e81cc Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Martin=20Storsj=C3=B6?=
Date: Mon, 8 Jan 2024 12:35:36 +0200
Subject: [PATCH] [clang] [Driver] Treat MuslEABIHF as a hardfloat environment
wrt multiarch directories
If using multiarch directories with musl, the multiarch directory
still uses *-linux-gnu triples - which may or may not be intentional,
while it is somewhat consistent at least.
However, for musl armhf targets, make sure that this also picks
arm-linux-gnueabihf, rather than arm-linux-gnueabi.
---
clang/lib/Driver/ToolChains/Gnu.cpp | 8 ++--
clang/lib/Driver/ToolChains/Linux.cpp | 8 ++--
clang/test/Driver/linux-ld.c | 9 +
3 files changed, 21 insertions(+), 4 deletions(-)
diff --git a/clang/lib/Driver/ToolChains/Gnu.cpp
b/clang/lib/Driver/ToolChains/Gnu.cpp
index 24681dfdc99c03..771240dac7a83e 100644
--- a/clang/lib/Driver/ToolChains/Gnu.cpp
+++ b/clang/lib/Driver/ToolChains/Gnu.cpp
@@ -2668,7 +2668,9 @@ void
Generic_GCC::GCCInstallationDetector::AddDefaultGCCPrefixes(
case llvm::Triple::arm:
case llvm::Triple::thumb:
LibDirs.append(begin(ARMLibDirs), end(ARMLibDirs));
-if (TargetTriple.getEnvironment() == llvm::Triple::GNUEABIHF) {
+if (TargetTriple.getEnvironment() == llvm::Triple::GNUEABIHF ||
+TargetTriple.getEnvironment() == llvm::Triple::MuslEABIHF ||
+TargetTriple.getEnvironment() == llvm::Triple::EABIHF) {
TripleAliases.append(begin(ARMHFTriples), end(ARMHFTriples));
} else {
TripleAliases.append(begin(ARMTriples), end(ARMTriples));
@@ -2677,7 +2679,9 @@ void
Generic_GCC::GCCInstallationDetector::AddDefaultGCCPrefixes(
case llvm::Triple::armeb:
case llvm::Triple::thumbeb:
LibDirs.append(begin(ARMebLibDirs), end(ARMebLibDirs));
-if (TargetTriple.getEnvironment() == llvm::Triple::GNUEABIHF) {
+if (TargetTriple.getEnvironment() == llvm::Triple::GNUEABIHF ||
+TargetTriple.getEnvironment() == llvm::Triple::MuslEABIHF ||
+TargetTriple.getEnvironment() == llvm::Triple::EABIHF) {
TripleAliases.append(begin(ARMebHFTriples), end(ARMebHFTriples));
} else {
TripleAliases.append(begin(ARMebTriples), end(ARMebTriples));
diff --git a/clang/lib/Driver/ToolChains/Linux.cpp
b/clang/lib/Driver/ToolChains/Linux.cpp
index 735af54f114cef..4300a2bdff1791 100644
--- a/clang/lib/Driver/ToolChains/Linux.cpp
+++ b/clang/lib/Driver/ToolChains/Linux.cpp
@@ -61,12 +61,16 @@ std::string Linux::getMultiarchTriple(const Driver &D,
case llvm::Triple::thumb:
if (IsAndroid)
return "arm-linux-androideabi";
-if (TargetEnvironment == llvm::Triple::GNUEABIHF)
+if (TargetEnvironment == llvm::Triple::GNUEABIHF ||
+TargetEnvironment == llvm::Triple::MuslEABIHF ||
+TargetEnvironment == llvm::Triple::EABIHF)
return "arm-linux-gnueabihf";
return "arm-linux-gnueabi";
case llvm::Triple::armeb:
case llvm::Triple::thumbeb:
-if (TargetEnvironment == llvm::Triple::GNUEABIHF)
+if (TargetEnvironment == llvm::Triple::GNUEABIHF ||
+TargetEnvironment == llvm::Triple::MuslEABIHF ||
+TargetEnvironment == llvm::Triple::EABIHF)
return "armeb-linux-gnueabihf";
return "armeb-linux-gnueabi";
case llvm::Triple::x86:
diff --git a/clang/test/Driver/linux-ld.c b/clang/test/Driver/linux-ld.c
index 15643d6491ae52..d5cc3103a3a746 100644
--- a/clang/test/Driver/linux-ld.c
+++ b/clang/test/Driver/linux-ld.c
@@ -541,6 +541,15 @@
// RUN: --gcc-toolchain="" \
// RUN: --sysroot=%S/Inputs/ubuntu_12.04_LTS_multiarch_tree \
// RUN: | FileCheck --check-prefix=CHECK-UBUNTU-12-04-ARM-HF %s
+//
+// Check that musleabihf is treated as a hardfloat config, with respect to
+// multiarch directories.
+//
+// RUN: %clang -### %s -no-pie 2>&1 \
+// RUN: --target=arm-unknown-linux-musleabihf -rtlib=platform
--unwindlib=platform \
+// RUN: --gcc-toolchain="" \
+// RUN: --sysroot=%S/Inputs/ubuntu_12.04_LTS_multiarch_tree \
+// RUN: | FileCheck --check-prefix=CHECK-UBUNTU-12-04-ARM-HF %s
// CHECK-UBUNTU-12-04-ARM-HF: "{{.*}}ld{{(.exe)?}}"
"--sysroot=[[SYSROOT:[^"]+]]"
// CHECK-UBUNTU-12-04-ARM-HF:
"{{.*}}/usr/lib/arm-linux-gnueabihf{{/|}}crt1.o"
// CHECK-UBUNTU-12-04-ARM-HF:
"{{.*}}/usr/lib/arm-linux-gnueabihf{{/|}}crti.o"
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[libunwind] [libunwind] Convert a few options from CACHE PATH to CACHE STRING (PR #77534)
https://github.com/mstorsjo created
https://github.com/llvm/llvm-project/pull/77534
This applies the same change as in
760261a3daf98882ccbd177e3133fb4a058f47ad (where they were applied to libcxxabi
and libcxx) to libunwind as well.
These options can reasonably be set either as an absolute or relative path, but
if set as type PATH, they are rewritten from relative into absolute relative to
the build directory, while the relative form is intended to be relative to the
install prefix.
From f4d654ff157f7a1ae3237d22dbbc541e6c083c4f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Martin=20Storsj=C3=B6?=
Date: Mon, 8 Jan 2024 14:32:47 +0200
Subject: [PATCH] [libunwind] Convert a few options from CACHE PATH to CACHE
STRING
This applies the same change as in
760261a3daf98882ccbd177e3133fb4a058f47ad (where they were applied
to libcxxabi and libcxx) to libunwind as well.
These options can reasonably be set either as an absolute or
relative path, but if set as type PATH, they are rewritten from
relative into absolute relative to the build directory, while
the relative form is intended to be relative to the install prefix.
---
libunwind/CMakeLists.txt | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/libunwind/CMakeLists.txt b/libunwind/CMakeLists.txt
index 248e888619e40b..bb1b052f61d875 100644
--- a/libunwind/CMakeLists.txt
+++ b/libunwind/CMakeLists.txt
@@ -105,9 +105,9 @@ set(CMAKE_MODULE_PATH
"${CMAKE_CURRENT_SOURCE_DIR}/cmake"
${CMAKE_MODULE_PATH})
-set(LIBUNWIND_INSTALL_INCLUDE_DIR "${CMAKE_INSTALL_INCLUDEDIR}" CACHE PATH
+set(LIBUNWIND_INSTALL_INCLUDE_DIR "${CMAKE_INSTALL_INCLUDEDIR}" CACHE STRING
"Path where built libunwind headers should be installed.")
-set(LIBUNWIND_INSTALL_RUNTIME_DIR "${CMAKE_INSTALL_BINDIR}" CACHE PATH
+set(LIBUNWIND_INSTALL_RUNTIME_DIR "${CMAKE_INSTALL_BINDIR}" CACHE STRING
"Path where built libunwind runtime libraries should be installed.")
set(LIBUNWIND_SHARED_OUTPUT_NAME "unwind" CACHE STRING "Output name for the
shared libunwind runtime library.")
@@ -115,7 +115,7 @@ set(LIBUNWIND_STATIC_OUTPUT_NAME "unwind" CACHE STRING
"Output name for the stat
if(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR AND NOT APPLE)
set(LIBUNWIND_LIBRARY_DIR
${LLVM_LIBRARY_OUTPUT_INTDIR}/${LLVM_DEFAULT_TARGET_TRIPLE})
- set(LIBUNWIND_INSTALL_LIBRARY_DIR
lib${LLVM_LIBDIR_SUFFIX}/${LLVM_DEFAULT_TARGET_TRIPLE} CACHE PATH
+ set(LIBUNWIND_INSTALL_LIBRARY_DIR
lib${LLVM_LIBDIR_SUFFIX}/${LLVM_DEFAULT_TARGET_TRIPLE} CACHE STRING
"Path where built libunwind libraries should be installed.")
if(LIBCXX_LIBDIR_SUBDIR)
string(APPEND LIBUNWIND_LIBRARY_DIR /${LIBUNWIND_LIBDIR_SUBDIR})
@@ -127,7 +127,7 @@ else()
else()
set(LIBUNWIND_LIBRARY_DIR
${CMAKE_BINARY_DIR}/lib${LIBUNWIND_LIBDIR_SUFFIX})
endif()
- set(LIBUNWIND_INSTALL_LIBRARY_DIR lib${LIBUNWIND_LIBDIR_SUFFIX} CACHE PATH
+ set(LIBUNWIND_INSTALL_LIBRARY_DIR lib${LIBUNWIND_LIBDIR_SUFFIX} CACHE STRING
"Path where built libunwind libraries should be installed.")
endif()
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] [MinGW] Don't look for a GCC in path if the install base has a proper mingw sysroot (PR #76949)
https://github.com/mstorsjo closed https://github.com/llvm/llvm-project/pull/76949 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] [MinGW] Don't look for a GCC in path if the install base has a proper mingw sysroot (PR #76949)
mstorsjo wrote: > > Although, on a second thought, it might actually still be good to adjust it > > in sync. If we're invoking Clang with `-m32` and deciding on whether to use > > i386/i586/i686, and we end up using the install base as sysroot, without > > inferring any triple from there, we shouldn't go on and check another > > unrelated GCC in path in order to influence this. Therefore, I think we > > perhaps should amend this with the following: > > Yes, I think that it would be better to avoid any decisions based on an > unrelated GCC and the additional check looks good to me. Ok, updated the PR with the patch that way; will merge it sometime later if nobody has any further objections to this. https://github.com/llvm/llvm-project/pull/76949 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] [MinGW] Don't look for a GCC in path if the install base has a proper mingw sysroot (PR #76949)
https://github.com/mstorsjo updated
https://github.com/llvm/llvm-project/pull/76949
From ce2a49c1a052b30fb1df91f3a7293e89e0a8726d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Martin=20Storsj=C3=B6?=
Date: Tue, 19 Dec 2023 15:53:21 +0200
Subject: [PATCH] [clang] [MinGW] Don't look for a GCC in path if the install
base has a proper mingw sysroot
This fixes uses of the MSYS2 clang64 environment compilers, if
another set of GCC based compilers are available further back
in PATH (which may be explicitly added, or inherited unintentionally
from other software installed).
(The issue in the clang64 environment can be worked around somewhat
by installing *-gcc-compat packages which present aliases named
-gcc within the clang64 environment as well.)
This fixes https://github.com/msys2/MINGW-packages/issues/11495
and https://github.com/msys2/MINGW-packages/issues/19279.
---
clang/lib/Driver/ToolChains/MinGW.cpp | 25 ++--
clang/test/Driver/mingw-sysroot.cpp | 28 +++
2 files changed, 51 insertions(+), 2 deletions(-)
diff --git a/clang/lib/Driver/ToolChains/MinGW.cpp
b/clang/lib/Driver/ToolChains/MinGW.cpp
index 65512f16357d04..18fc9d4b6807e3 100644
--- a/clang/lib/Driver/ToolChains/MinGW.cpp
+++ b/clang/lib/Driver/ToolChains/MinGW.cpp
@@ -471,12 +471,23 @@ findClangRelativeSysroot(const Driver &D, const
llvm::Triple &LiteralTriple,
return make_error_code(std::errc::no_such_file_or_directory);
}
+static bool looksLikeMinGWSysroot(const std::string &Directory) {
+ StringRef Sep = llvm::sys::path::get_separator();
+ if (!llvm::sys::fs::exists(Directory + Sep + "include" + Sep + "_mingw.h"))
+return false;
+ if (!llvm::sys::fs::exists(Directory + Sep + "lib" + Sep + "libkernel32.a"))
+return false;
+ return true;
+}
+
toolchains::MinGW::MinGW(const Driver &D, const llvm::Triple &Triple,
const ArgList &Args)
: ToolChain(D, Triple, Args), CudaInstallation(D, Triple, Args),
RocmInstallation(D, Triple, Args) {
getProgramPaths().push_back(getDriver().getInstalledDir());
+ std::string InstallBase =
+ std::string(llvm::sys::path::parent_path(getDriver().getInstalledDir()));
// The sequence for detecting a sysroot here should be kept in sync with
// the testTriple function below.
llvm::Triple LiteralTriple = getLiteralTriple(D, getTriple());
@@ -487,13 +498,17 @@ toolchains::MinGW::MinGW(const Driver &D, const
llvm::Triple &Triple,
else if (llvm::ErrorOr TargetSubdir = findClangRelativeSysroot(
getDriver(), LiteralTriple, getTriple(), SubdirName))
Base = std::string(llvm::sys::path::parent_path(TargetSubdir.get()));
+ // If the install base of Clang seems to have mingw sysroot files directly
+ // in the toplevel include and lib directories, use this as base instead of
+ // looking for a triple prefixed GCC in the path.
+ else if (looksLikeMinGWSysroot(InstallBase))
+Base = InstallBase;
else if (llvm::ErrorOr GPPName =
findGcc(LiteralTriple, getTriple()))
Base = std::string(llvm::sys::path::parent_path(
llvm::sys::path::parent_path(GPPName.get(;
else
-Base = std::string(
-llvm::sys::path::parent_path(getDriver().getInstalledDir()));
+Base = InstallBase;
Base += llvm::sys::path::get_separator();
findGccLibDir(LiteralTriple);
@@ -778,9 +793,15 @@ static bool testTriple(const Driver &D, const llvm::Triple
&Triple,
if (D.SysRoot.size())
return true;
llvm::Triple LiteralTriple = getLiteralTriple(D, Triple);
+ std::string InstallBase =
+ std::string(llvm::sys::path::parent_path(D.getInstalledDir()));
if (llvm::ErrorOr TargetSubdir =
findClangRelativeSysroot(D, LiteralTriple, Triple, SubdirName))
return true;
+ // If the install base itself looks like a mingw sysroot, we'll use that
+ // - don't use any potentially unrelated gcc to influence what triple to use.
+ if (looksLikeMinGWSysroot(InstallBase))
+return false;
if (llvm::ErrorOr GPPName = findGcc(LiteralTriple, Triple))
return true;
// If we neither found a colocated sysroot or a matching gcc executable,
diff --git a/clang/test/Driver/mingw-sysroot.cpp
b/clang/test/Driver/mingw-sysroot.cpp
index 911dab4927073d..50152b2ca210d2 100644
--- a/clang/test/Driver/mingw-sysroot.cpp
+++ b/clang/test/Driver/mingw-sysroot.cpp
@@ -14,6 +14,12 @@
// RUN: ln -s %S/Inputs/mingw_ubuntu_posix_tree/usr/x86_64-w64-mingw32
%T/testroot-clang/x86_64-w64-mingw32
// RUN: ln -s %S/Inputs/mingw_arch_tree/usr/i686-w64-mingw32
%T/testroot-clang/i686-w64-mingw32
+// RUN: rm -rf %T/testroot-clang-native
+// RUN: mkdir -p %T/testroot-clang-native/bin
+// RUN: ln -s %clang %T/testroot-clang-native/bin/clang
+// RUN: mkdir -p %T/testroot-clang-native/include/_mingw.h
+// RUN: mkdir -p %T/testroot-clang-native/lib/libkernel32.a
+
// RUN: rm -rf %T/testroot-custom-triple
// RUN: mkdir -p %T/testroot-custom-triple/bin
// RUN: ln
[clang] [clang] [MinGW] Don't look for a GCC in path if the install base has a proper mingw sysroot (PR #76949)
mstorsjo wrote: > > Looks mostly good to me, but I wonder if we should change testTriple as > > well. > > I thought so too based on the comment, but reviewing the code it seems > `testTriple` is trying to find evidence that a given triple (and more > specifically arch for things like `i386` vs `i686`) is valid. The evidence > found by `looksLikeMinGWSysroot` does not provide any hint about what the > triple or arch name should be, so I don't think it helps `testTriple`. Thanks, this explanation is absolutely spot on. (In all honesty, I had forgot about `testTriple` when I wrote this patch - and despite the comment right next to the code I touched, I didn't remember to check it...) Although, on a second thought, it might actually still be good to adjust it in sync. If we're invoking Clang with `-m32` and deciding on whether to use i386/i586/i686, and we end up using the install base as sysroot, without inferring any triple from there, we shouldn't go on and check another unrelated GCC in path in order to influence this. Therefore, I think we perhaps should amend this with the following: ```diff diff --git a/clang/lib/Driver/ToolChains/MinGW.cpp b/clang/lib/Driver/ToolChains/MinGW.cpp index e2965a08a0b9..18fc9d4b6807 100644 --- a/clang/lib/Driver/ToolChains/MinGW.cpp +++ b/clang/lib/Driver/ToolChains/MinGW.cpp @@ -793,9 +793,15 @@ static bool testTriple(const Driver &D, const llvm::Triple &Triple, if (D.SysRoot.size()) return true; llvm::Triple LiteralTriple = getLiteralTriple(D, Triple); + std::string InstallBase = + std::string(llvm::sys::path::parent_path(D.getInstalledDir())); if (llvm::ErrorOr TargetSubdir = findClangRelativeSysroot(D, LiteralTriple, Triple, SubdirName)) return true; + // If the install base itself looks like a mingw sysroot, we'll use that + // - don't use any potentially unrelated gcc to influence what triple to use. + if (looksLikeMinGWSysroot(InstallBase)) +return false; if (llvm::ErrorOr GPPName = findGcc(LiteralTriple, Triple)) return true; // If we neither found a colocated sysroot or a matching gcc executable, ``` Actually, for such cases, we could potentially check for the per-target runtime installs, e.g. looking for `/include/` or `/lib/`. Switching between multiple arches with e.g. `-m32` in such a setup could work, if all the arch specific files are in `/lib/`, but I'm not sure if anyone does full mingw setups with such a hierarchy (yet). So this would be a separate potential future step. https://github.com/llvm/llvm-project/pull/76949 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] [MinGW] Don't look for a GCC in path if the install base has a proper mingw sysroot (PR #76949)
https://github.com/mstorsjo updated
https://github.com/llvm/llvm-project/pull/76949
From c67187043168b79e57c0e4f3261293d799852e90 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Martin=20Storsj=C3=B6?=
Date: Tue, 19 Dec 2023 15:53:21 +0200
Subject: [PATCH] [clang] [MinGW] Don't look for a GCC in path if the install
base has a proper mingw sysroot
This fixes uses of the MSYS2 clang64 environment compilers, if
another set of GCC based compilers are available further back
in PATH (which may be explicitly added, or inherited unintentionally
from other software installed).
(The issue in the clang64 environment can be worked around somewhat
by installing *-gcc-compat packages which present aliases named
-gcc within the clang64 environment as well.)
This fixes https://github.com/msys2/MINGW-packages/issues/11495
and https://github.com/msys2/MINGW-packages/issues/19279.
---
clang/lib/Driver/ToolChains/MinGW.cpp | 19 --
clang/test/Driver/mingw-sysroot.cpp | 28 +++
2 files changed, 45 insertions(+), 2 deletions(-)
diff --git a/clang/lib/Driver/ToolChains/MinGW.cpp
b/clang/lib/Driver/ToolChains/MinGW.cpp
index 65512f16357d04..e2965a08a0b9a2 100644
--- a/clang/lib/Driver/ToolChains/MinGW.cpp
+++ b/clang/lib/Driver/ToolChains/MinGW.cpp
@@ -471,12 +471,23 @@ findClangRelativeSysroot(const Driver &D, const
llvm::Triple &LiteralTriple,
return make_error_code(std::errc::no_such_file_or_directory);
}
+static bool looksLikeMinGWSysroot(const std::string &Directory) {
+ StringRef Sep = llvm::sys::path::get_separator();
+ if (!llvm::sys::fs::exists(Directory + Sep + "include" + Sep + "_mingw.h"))
+return false;
+ if (!llvm::sys::fs::exists(Directory + Sep + "lib" + Sep + "libkernel32.a"))
+return false;
+ return true;
+}
+
toolchains::MinGW::MinGW(const Driver &D, const llvm::Triple &Triple,
const ArgList &Args)
: ToolChain(D, Triple, Args), CudaInstallation(D, Triple, Args),
RocmInstallation(D, Triple, Args) {
getProgramPaths().push_back(getDriver().getInstalledDir());
+ std::string InstallBase =
+ std::string(llvm::sys::path::parent_path(getDriver().getInstalledDir()));
// The sequence for detecting a sysroot here should be kept in sync with
// the testTriple function below.
llvm::Triple LiteralTriple = getLiteralTriple(D, getTriple());
@@ -487,13 +498,17 @@ toolchains::MinGW::MinGW(const Driver &D, const
llvm::Triple &Triple,
else if (llvm::ErrorOr TargetSubdir = findClangRelativeSysroot(
getDriver(), LiteralTriple, getTriple(), SubdirName))
Base = std::string(llvm::sys::path::parent_path(TargetSubdir.get()));
+ // If the install base of Clang seems to have mingw sysroot files directly
+ // in the toplevel include and lib directories, use this as base instead of
+ // looking for a triple prefixed GCC in the path.
+ else if (looksLikeMinGWSysroot(InstallBase))
+Base = InstallBase;
else if (llvm::ErrorOr GPPName =
findGcc(LiteralTriple, getTriple()))
Base = std::string(llvm::sys::path::parent_path(
llvm::sys::path::parent_path(GPPName.get(;
else
-Base = std::string(
-llvm::sys::path::parent_path(getDriver().getInstalledDir()));
+Base = InstallBase;
Base += llvm::sys::path::get_separator();
findGccLibDir(LiteralTriple);
diff --git a/clang/test/Driver/mingw-sysroot.cpp
b/clang/test/Driver/mingw-sysroot.cpp
index 911dab4927073d..50152b2ca210d2 100644
--- a/clang/test/Driver/mingw-sysroot.cpp
+++ b/clang/test/Driver/mingw-sysroot.cpp
@@ -14,6 +14,12 @@
// RUN: ln -s %S/Inputs/mingw_ubuntu_posix_tree/usr/x86_64-w64-mingw32
%T/testroot-clang/x86_64-w64-mingw32
// RUN: ln -s %S/Inputs/mingw_arch_tree/usr/i686-w64-mingw32
%T/testroot-clang/i686-w64-mingw32
+// RUN: rm -rf %T/testroot-clang-native
+// RUN: mkdir -p %T/testroot-clang-native/bin
+// RUN: ln -s %clang %T/testroot-clang-native/bin/clang
+// RUN: mkdir -p %T/testroot-clang-native/include/_mingw.h
+// RUN: mkdir -p %T/testroot-clang-native/lib/libkernel32.a
+
// RUN: rm -rf %T/testroot-custom-triple
// RUN: mkdir -p %T/testroot-custom-triple/bin
// RUN: ln -s %clang %T/testroot-custom-triple/bin/clang
@@ -58,6 +64,28 @@
// RUN: env "PATH=%T/testroot-gcc/bin:%PATH%"
%T/testroot-gcc/bin/x86_64-w64-mingw32-clang -target x86_64-w64-mingw32
-rtlib=platform -stdlib=libstdc++ --sysroot="" -c -### %s 2>&1 | FileCheck
-check-prefix=CHECK_TESTROOT_GCC %s
+// If we're executing clang from a directory with what looks like a mingw
sysroot,
+// with headers in /include and libs in /lib, use that rather than
looking
+// for another GCC in the path.
+//
+// Note, this test has a surprising quirk: We're testing with an install
directory,
+// testroot-clang-native, which lacks the "x86_64-w64-mingw32" subdirectory,
it only
+// has the include and lib subdirectories without any triple prefix.
+//
+// Since commit fd15cb935d7aae25ad62bfe06fe9f17cea58597
[clang] [clang] [MinGW] Don't look for a GCC in path if the install base has a proper mingw sysroot (PR #76949)
mstorsjo wrote: CC @mati865 @jeremyd2019 @huangqinjin https://github.com/llvm/llvm-project/pull/76949 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] [MinGW] Don't look for a GCC in path if the install base has a proper mingw sysroot (PR #76949)
https://github.com/mstorsjo created
https://github.com/llvm/llvm-project/pull/76949
This fixes uses of the MSYS2 clang64 environment compilers, if another set of
GCC based compilers are available further back in PATH (which may be explicitly
added, or inherited unintentionally from other software installed).
(The issue in the clang64 environment can be worked around somewhat by
installing *-gcc-compat packages which present aliases named -gcc
within the clang64 environment as well.)
This fixes https://github.com/msys2/MINGW-packages/issues/11495 and
https://github.com/msys2/MINGW-packages/issues/19279.
From 355e2530e855249adf9657c58d4e1a6727d969bd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Martin=20Storsj=C3=B6?=
Date: Tue, 19 Dec 2023 15:53:21 +0200
Subject: [PATCH] [clang] [MinGW] Don't look for a GCC in path if the install
base has a proper mingw sysroot
This fixes uses of the MSYS2 clang64 environment compilers, if
another set of GCC based compilers are available further back
in PATH (which may be explicitly added, or inherited unintentionally
from other software installed).
(The issue in the clang64 environment can be worked around somewhat
by installing *-gcc-compat packages which present aliases named
-gcc within the clang64 environment as well.)
This fixes https://github.com/msys2/MINGW-packages/issues/11495
and https://github.com/msys2/MINGW-packages/issues/19279.
---
clang/lib/Driver/ToolChains/MinGW.cpp | 19 --
clang/test/Driver/mingw-sysroot.cpp | 28 +++
2 files changed, 45 insertions(+), 2 deletions(-)
diff --git a/clang/lib/Driver/ToolChains/MinGW.cpp
b/clang/lib/Driver/ToolChains/MinGW.cpp
index 65512f16357d04..3868657659602e 100644
--- a/clang/lib/Driver/ToolChains/MinGW.cpp
+++ b/clang/lib/Driver/ToolChains/MinGW.cpp
@@ -471,12 +471,23 @@ findClangRelativeSysroot(const Driver &D, const
llvm::Triple &LiteralTriple,
return make_error_code(std::errc::no_such_file_or_directory);
}
+static bool looksLikeMinGWSysroot(const std::string &Directory) {
+ StringRef Sep = llvm::sys::path::get_separator();
+ if (!llvm::sys::fs::exists(Directory + Sep + "include" + Sep + "_mingw.h"))
+return false;
+ if (!llvm::sys::fs::exists(Directory + Sep + "lib" + Sep + "libkernel32.a"))
+return false;
+ return true;
+}
+
toolchains::MinGW::MinGW(const Driver &D, const llvm::Triple &Triple,
const ArgList &Args)
: ToolChain(D, Triple, Args), CudaInstallation(D, Triple, Args),
RocmInstallation(D, Triple, Args) {
getProgramPaths().push_back(getDriver().getInstalledDir());
+ std::string InstallBase = std::string(
+llvm::sys::path::parent_path(getDriver().getInstalledDir()));
// The sequence for detecting a sysroot here should be kept in sync with
// the testTriple function below.
llvm::Triple LiteralTriple = getLiteralTriple(D, getTriple());
@@ -487,13 +498,17 @@ toolchains::MinGW::MinGW(const Driver &D, const
llvm::Triple &Triple,
else if (llvm::ErrorOr TargetSubdir = findClangRelativeSysroot(
getDriver(), LiteralTriple, getTriple(), SubdirName))
Base = std::string(llvm::sys::path::parent_path(TargetSubdir.get()));
+ // If the install base of Clang seems to have mingw sysroot files directly
+ // in the toplevel include and lib directories, use this as base instead of
+ // looking for a triple prefixed GCC in the path.
+ else if (looksLikeMinGWSysroot(InstallBase))
+Base = InstallBase;
else if (llvm::ErrorOr GPPName =
findGcc(LiteralTriple, getTriple()))
Base = std::string(llvm::sys::path::parent_path(
llvm::sys::path::parent_path(GPPName.get(;
else
-Base = std::string(
-llvm::sys::path::parent_path(getDriver().getInstalledDir()));
+Base = InstallBase;
Base += llvm::sys::path::get_separator();
findGccLibDir(LiteralTriple);
diff --git a/clang/test/Driver/mingw-sysroot.cpp
b/clang/test/Driver/mingw-sysroot.cpp
index 911dab4927073d..50152b2ca210d2 100644
--- a/clang/test/Driver/mingw-sysroot.cpp
+++ b/clang/test/Driver/mingw-sysroot.cpp
@@ -14,6 +14,12 @@
// RUN: ln -s %S/Inputs/mingw_ubuntu_posix_tree/usr/x86_64-w64-mingw32
%T/testroot-clang/x86_64-w64-mingw32
// RUN: ln -s %S/Inputs/mingw_arch_tree/usr/i686-w64-mingw32
%T/testroot-clang/i686-w64-mingw32
+// RUN: rm -rf %T/testroot-clang-native
+// RUN: mkdir -p %T/testroot-clang-native/bin
+// RUN: ln -s %clang %T/testroot-clang-native/bin/clang
+// RUN: mkdir -p %T/testroot-clang-native/include/_mingw.h
+// RUN: mkdir -p %T/testroot-clang-native/lib/libkernel32.a
+
// RUN: rm -rf %T/testroot-custom-triple
// RUN: mkdir -p %T/testroot-custom-triple/bin
// RUN: ln -s %clang %T/testroot-custom-triple/bin/clang
@@ -58,6 +64,28 @@
// RUN: env "PATH=%T/testroot-gcc/bin:%PATH%"
%T/testroot-gcc/bin/x86_64-w64-mingw32-clang -target x86_64-w64-mingw32
-rtlib=platform -stdlib=libstdc++ --sysroot="" -c -### %s 2>&1 | FileCheck
-chec
[clang] 71b3ead - [clang][dataflow] Remove a redundant trailing semicolon. NFC.
Author: Martin Storsjö
Date: 2024-01-04T15:01:17+02:00
New Revision: 71b3ead870107e39e998f6480e545eb01d9d28be
URL:
https://github.com/llvm/llvm-project/commit/71b3ead870107e39e998f6480e545eb01d9d28be
DIFF:
https://github.com/llvm/llvm-project/commit/71b3ead870107e39e998f6480e545eb01d9d28be.diff
LOG: [clang][dataflow] Remove a redundant trailing semicolon. NFC.
This silences the following warning with GCC:
llvm-project/llvm/tools/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp:89:4:
warning: extra ‘;’ [-Wpedantic]
89 | };
|^
|-
Added:
Modified:
clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
Removed:
diff --git
a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
index 8d481788af208a..fe5ba5ab5426f7 100644
--- a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
@@ -86,7 +86,7 @@ class DataflowAnalysisTest : public Test {
const std::optional &MaybeState = BlockStates[Block->getBlockID()];
assert(MaybeState.has_value());
return *MaybeState;
- };
+ }
std::unique_ptr AST;
std::unique_ptr CFCtx;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[lld] [llvm] [libcxxabi] [compiler-rt] [libc] [openmp] [mlir] [clang-tools-extra] [clang] [lldb] [libcxx] [flang] [builtins][arm64] Build __init_cpu_features_resolver on Apple platforms (PR #73685)
mstorsjo wrote: > > BTW, when compiling the file I also get a bunch of warnings in this style: > > @mstorsjo maybe `unsigned long` is 32 bits on that platform... what's the > target triple? Ah, indeed - yes, Windows has 32 bit `long`s. The triples are `aarch64-windows-gnu` or `aarch64-windows-msvc`. https://github.com/llvm/llvm-project/pull/73685 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [clang-tools-extra] [libc] [compiler-rt] [libcxx] [openmp] [mlir] [lldb] [flang] [libcxxabi] [lld] [builtins][arm64] Build __init_cpu_features_resolver on Apple platforms (PR #73685)
mstorsjo wrote:
This commit broken building compiler-rt builtins for Windows on aarch64;
building now hits these errors:
```
llvm-project/compiler-rt/lib/builtins/cpu_model.c:1192:2: error: No support for
checking for lse atomics on this platfrom yet.
1192 | #error No support for checking for lse atomics on this platfrom yet.
| ^
llvm-project/compiler-rt/lib/builtins/cpu_model.c:1571:2: error: No support for
checking hwcap on this platform yet.
1571 | #error No support for checking hwcap on this platform yet.
| ^
2 errors generated.
```
Before this change, most of this whole file was ifdeffed out when building on
Windows (and Apple platforms, I would presume), but now most of it is included,
then hitting this `#error`.
I guess it could work to just remove the `#error` cases, but this file suffers
from a pretty deep ifdef nesting jungle, so I'm not sure if that's the best
solution. (FWIW, if we wanted to add aarch64 CPU feature detection for Windows
here, the code would be more of a separate codepath just like the Apple case,
it doesn't share the linux/BSD HWCAP style.)
I can push a quick fix, either removing the `#error` or reverting this commit,
later during the day.
BTW, when compiling the file I also get a bunch of warnings in this style:
```
llvm-project/compiler-rt/lib/builtins/cpu_model.c:1448:36: warning: value size
does not match register size specified by the constraint and modifier
[-Wasm-operand-widths]
1448 | getCPUFeature(ID_AA64PFR1_EL1, ftr);
|^
llvm-project/compiler-rt/lib/builtins/cpu_model.c:1448:5: note: use constraint
modifier "w"
1448 | getCPUFeature(ID_AA64PFR1_EL1, ftr);
| ^
llvm-project/compiler-rt/lib/builtins/cpu_model.c:1345:45: note: expanded from
macro 'getCPUFeature'
1345 | #define getCPUFeature(id, ftr) __asm__("mrs %0, " #id : "=r"(ftr))
|
```
https://github.com/llvm/llvm-project/pull/73685
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Cygwin] Cygwin driver (PR #74933)
mstorsjo wrote: > > @carlo-bramini has spent some effort on using Clang in Cygwin environments > > before, so as far as I know, it does work in general from before. So this > > change, which adds an entirely new driver for Cygwin environments, would > > need to be explained why it does that (I don't disagree, it's probably the > > right thing to do in general), how things worked before and how this > > changes things. And I would like to have @carlo-bramini's eye on this (and > > all the related Cygwin patches from @xu-chiheng). > > And changes like this need some general tests, have a look at > > `clang/test/Driver` for how other drivers are tested. > > The Cygwin driver is basically the same as MinGW driver with minor difference. Right. What are the actual observable differences to what was executed before? (I presume this before used the `Generic_GCC` toolchain?) Also, I wonder if it would make sense to share the MinGW driver code with Cygwin, and just add exceptions where necessary, instead of duplicating it into a separate one? Maybe, but perhaps not. https://github.com/llvm/llvm-project/pull/74933 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [MinGW] MinGW dynamicbase (PR #74979)
mstorsjo wrote: > > Also > > In Cygwin with binutils 2.41, --dynamicbase make a difference, so I thought > MinGW also need it. No, MinGW does not need it, as it has been enabled by default since binutils 2.36. Apparently that change, https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=514b4e191d5f46de8e142fe216e677a35fa9c4bb, didn't apply to Cygwin but only to MinGW. But if Cygwin works with dynamicbase (I think it might have issues with it but I'm not sure?) then perhaps binutils should be changed to enable dynamicbase by default there, instead of changing compilers to pass the option by default. https://github.com/llvm/llvm-project/pull/74979 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [MinGW] Fix the regression caused by commit 592e935e115ffb451eb9b782376711dab6558fe0, that, in MinGW, Clang can't be built by system Clang 15.0.4. (PR #74982)
mstorsjo wrote: > I have build scripts and patches at: https://github.com/xu-chiheng/Note This does not answer the question. You need to explain what is broken, and why, and how this fixes it. And address the concern that this actually breaks functionality in some cases. I guess this partially answers the question on in what exact environment the issue occurs, although it would require me to dissect your build script environment to figure it out. https://github.com/llvm/llvm-project/pull/74982 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [MinGW] MinGW dynamicbase (PR #74979)
https://github.com/mstorsjo requested changes to this pull request. No, you do not need to do this. There's no need to add `--dynamicbase` manually in Clang. As I already posted, both ld.bfd and ld.lld default to `--dynamicbase` enabled since 2020. https://github.com/llvm/llvm-project/pull/74979 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [MinGW] Fix the regression caused by commit 592e935e115ffb451eb9b782376711dab6558fe0, that, in MinGW, Clang can't be built by system Clang 15.0.4. (PR #74982)
mstorsjo wrote: I don't know what issue/regression you're referring to. Please explain, in detail, what the issue is and all the relevant aspects of your configuration. Also explain what the suggested fix does, and how it handles the various cases (I just tested building latest llvm-project main with Clang 15 and LLD, for a mingw target, and it worked just fine, both as a regular non-dylib build, and with `LLVM_LINK_LLVM_DYLIB` enabled.) I also believe that the suggested patch would break actual use of clang-repl; if `LLVM_BUILD_LLVM_DYLIB` or `LLVM_BUILD_SHARED_LIBS` aren't defined, then those symbols wouldn't be dllexported at all. This causes them to not be found at runtime when the JIT runtime tries to locate them. https://github.com/llvm/llvm-project/pull/74982 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Cygwin] Cygwin driver (PR #74933)
mstorsjo wrote: @carlo-bramini has spent some effort on using Clang in Cygwin environments before, so as far as I know, it does work in general from before. So this change, which adds an entirely new driver for Cygwin environments, would need to be explained why it does that (I don't disagree, it's probably the right thing to do in general), how things worked before and how this changes things. And I would like to have @carlo-bramini's eye on this (and all the related Cygwin patches from @xu-chiheng). And changes like this need some general tests, have a look at `clang/test/Driver` for how other drivers are tested. https://github.com/llvm/llvm-project/pull/74933 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [MinGW] MinGW pthread (PR #74981)
mstorsjo wrote: This breaks bootstrapping llvm-mingw. Not all mingw environments use or require pthreads; llvm-mingw is one such environment, and the clang64 environment in msys2 is another one. While llvm-mingw does contain winpthreads, it is built later in the build process, and if this patch is applied, the setup procedure is broken; one would need to reorder how these libraries are linked, or create a dummy empty `libpthread.a` to make sure that linking works until the read winpthreads library is built. Note that within msys2, they do apply a patch that does exactly what this patch does, for the mingw64 environment, where the system libstdc++ and similar does require winpthreads. The fact that this is patched for the GCC environments isn't ideal, but any attempt to modify this needs to first acknowledge the current state of things and not just blindly barge ahead with a breaking change like this. Also do note that the upcoming GCC 14 will have the win32 thread model supporting C++11, so it is quite possible for GCC based environments to stop relying so much on winpthreads, which would reduce the need for this patch. CC @mati865 @lazka @jeremyd2019 https://github.com/llvm/llvm-project/pull/74981 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [MinGW] MinGW dynamicbase (PR #74979)
https://github.com/mstorsjo requested changes to this pull request. This is not necessary. Since 514b4e191d5f46de8e142fe216e677a35fa9c4bb in binutils (https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=514b4e191d5f46de8e142fe216e677a35fa9c4bb), dynamicbase is enabled by default. Also since e72403f96de7f1c681acd5968f72aa986412dfce in llvm-project, LLD also does the same. https://github.com/llvm/llvm-project/pull/74979 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Stub out gcc_struct attribute (PR #71148)
mstorsjo wrote: > Right, I'd just like to make sure that we're not deepening a divergence here. > It would be good to get agreement from the GCC devs that they think > `ms_struct` probably ought to do something on e.g. ARM MinGW targets and that > they consider this a bug (in a feature that they may not really support, > which is fine). But if they think _we're_ wrong and that this really should > only have effect on x86, I would like to know that. I'm not a GCC developer, but I would not think that GCC would consider this an x86-only feature. It just so happens that (upstream) GCC only supports Windows on x86. But MSVC does their own funky bitfield packing on all architectures - so it seems reasonable to want to be able to match it on all architectures. https://github.com/llvm/llvm-project/pull/71148 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[lldb] [clang] [clang][DebugInfo] Revert "emit definitions for constant-initialized static data-members" (PR #74580)
mstorsjo wrote: Could we please land this now? https://github.com/llvm/llvm-project/pull/74580 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Stub out gcc_struct attribute (PR #71148)
mstorsjo wrote:
> Okay, @mstorsjo @MaskRay, what is the way forward?
I'm totally not authoritative for these things, but...
> Am I right that, as for the user-facing changes, `[[gcc_struct]]` cancelling
> implicit `-mms-bitfilds` on MinGW is fine
Sounds quite fine for me
> and silently ignoring `-m{no-}ms-bitfields` on `windows-msvc` is not?
Silently ignoring options is clearly not good IMO, so either we warn about them
or implement them
> Should we (and if yes, when exactly) disallow `-m{no-,}ms-bitfields`? Should
> the aforementioned `--target=x86_64-pc-windows-msvc -fc++-abi=itanium
> -mms-bitfields` be accepted?
FWIW I wasn't even aware that it was possible to pick a nondefault C++ ABI, so
I don't have a strong opinion on this matter. If it works and doesn't create
inconsistencies, then I don't mind, but I guess the regular Clang maintainers
have more of a final say on that.
> Is it fine to provide `[[gcc_struct]]` on MSVC because of the reasons I
> outlined before?
I would be totally fine with that.
https://github.com/llvm/llvm-project/pull/71148
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Stub out gcc_struct attribute (PR #71148)
mstorsjo wrote: > One more thing. Re binary compatibility concerns: `-mno-ms-bitfields` on > MinGW is an equally-sized footgun as on MSVC. Without proper header > annotation with `#pragma ms_struct on`, either of them will silently make an > ABI mismatch. However, for some reason, supporting `-mno-ms-bitfields` on > MinGW is not argued upon. I guess this is for historical reasons. Originally the MinGW target had this as an opt-in, and this was indeed a silent ABI mismatch. Users who needed to use affected structs and interact with Microsoft APIs had to remember to build their code with `-mms-bitfields` (and hope they don't link in some code that is built without it, with affected structs in their interface). It became the default only by GCC 4.7 (in 2012), and we picked up on this way much later in Clang, in Clang 11 in 2020. https://github.com/llvm/llvm-project/pull/71148 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Stub out gcc_struct attribute (PR #71148)
mstorsjo wrote:
> Microsoft bit-field layout didn't break an overly-specific regression test
> but rendered unusable double to string conversion. The culprit was the
> following snippet:
>
> ```c++
> union Extractor {
> double value;
> struct {
> bool sign : 1;
> u32 exponent : 11;
> u64 mantissa : 52;
> };
> };
> ```
>
> According to MSVC ABI, there should be padding between fields. I hope you
> agree that this is not an intuitive and expected behavior.
It is indeed an unexpected thing. However it is possible to work around it for
all these cases; if you declare the inner structure like this:
```
struct {
u64 sign : 1;
u64 exponent : 11;
u64 mantissa : 52;
};
```
Then there will be no extra padding between the fields.
https://github.com/llvm/llvm-project/pull/71148
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Stub out gcc_struct attribute (PR #71148)
mstorsjo wrote:
> `-mms-bitfields` is a GCC x86 specific option (`aarch64-linux-gnu-gcc
> -mms-bitfields -xc /dev/null -E` => `error: unrecognized command-line option
> ‘-mms-bitfields’`).
While it is implemented as an x86 specific option in GCC right now, that
doesn't mean that it only is supposed to have an effect for x86. Upstream GCC
only supports Windows on x86, and my recollection is that lots of the Windows
specific logic is located in x86 specific files, even if the same logic also
should apply for Windows on any other architecture - it's just not implemented
(yet).
As implemented in Clang, the option works for any MinGW target.
As an example:
```c
struct field {
unsigned char a : 4;
unsigned int b : 4;
};
int size = sizeof(struct field);
```
```console
$ clang -target x86_64-windows-gnu -S -o - bitfields.c | grep -A1 size
.globl size# @size
.p2align2, 0x0
size:
.long 8 # 0x8
$ clang -target x86_64-windows-gnu -S -o - bitfields.c -mno-ms-bitfields | grep
-A1 size
.globl size# @size
.p2align2, 0x0
size:
.long 4 # 0x4
$ clang -target aarch64-windows-gnu -S -o - bitfields.c | grep -A1 size
.globl size// @size
.p2align2, 0x0
size:
.word 8 // 0x8
$ clang -target aarch64-windows-gnu -S -o - bitfields.c -mno-ms-bitfields |
grep -A1 size
.globl size// @size
.p2align2, 0x0
size:
.word 4 // 0x4
```
---
> https://gitlab.com/qemu-project/qemu/-/issues/1782#note_1495842591 seems like
> ignored `gcc_struct` for windows-gnu targets (feature request #24757).
> I agree that if there are real needs and the needs seem genuine, Clang should
> support `gcc_struct`.
Yes, this would clearly be good to have. For orthogonality, it would be good to
have both `gcc_struct` and `ms_struct` available. GCC does support them on
non-windows targets as well; I think that can be useful for implementing things
like Wine.
---
As noted somewhere (I don't see the comment to quote right now), the MS
bitfield packing logic (as enabled by `-mms-bitfields`) is enabled by default
when using the MSVC C++ ABI, but not when using the Itanium C++ ABI. But as
referenced, since https://reviews.llvm.org/D81795, we do enable the option
`-mms-bitfields` automatically for MinGW targets which otherwise do use the
Itanium C++ ABI. Being able to override this back to the default format for
individual structs would be great.
I don't know and can't speculate about what the implications would be for being
able to switch to GCC struct layout in the MSVC C++ ABI, though. For individual
structs, I guess it should be doable (I'm only thinking for trivial-ish structs
like the one in my example above, right now though). For anything relating to
C++ classes and the mechanisms behind them, I'm pretty sure one doesn't want to
change how anything of that works. Therefore I don't think it's too relevant to
implement `-mno-ms-bitfields` for MSVC targets (as I guess it could open a huge
can of worms).
https://github.com/llvm/llvm-project/pull/71148
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[libunwind] [libunwind] Fix an inconsistent indentation (NFC) (PR #72314)
https://github.com/mstorsjo approved this pull request. LGTM, thanks! (I have no idea how I botched that previous fix commit...) https://github.com/llvm/llvm-project/pull/72314 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [X86][AVX10] Permit AVX512 options/features used together with AVX10 (PR #71318)
mstorsjo wrote: > Hi Phoebe, starting seeing this error on rather old codes after this patch > landed . is there a particular flag you recommend i should compile with to > get previous behavior ? > > error: always_inline function '_mm_setzero_pd' requires target feature > 'evex512', but would be inlined into function '_mm_getexp_pd' that is > compiled without support for 'evex512' I also ran into something similar, when compiling Qt; I filed https://github.com/llvm/llvm-project/issues/72106 with a different reproducer. https://github.com/llvm/llvm-project/pull/71318 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix build dllexport/dllimport issues when doing a shared build for Windows using GCC (PR #66881)
mstorsjo wrote: This is superseded by #71393 which was merged now. https://github.com/llvm/llvm-project/pull/66881 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-repl] Fix BUILD_SHARED_LIBS symbols from libclangInterpreter on MinGW (PR #71393)
https://github.com/mstorsjo closed https://github.com/llvm/llvm-project/pull/71393 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[llvm] [clang] Reapply #2 [clang-repl] [test] Make an XFAIL more precise (PR #71168)
https://github.com/mstorsjo closed https://github.com/llvm/llvm-project/pull/71168 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[llvm] [clang] Reapply #2 [clang-repl] [test] Make an XFAIL more precise (PR #71168)
https://github.com/mstorsjo edited https://github.com/llvm/llvm-project/pull/71168 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[llvm] [clang] Reapply #2 [clang-repl] [test] Make an XFAIL more precise (PR #71168)
https://github.com/mstorsjo edited https://github.com/llvm/llvm-project/pull/71168 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix build dllexport/dllimport issues when doing a shared build for Windows using GCC (PR #66881)
mstorsjo wrote: Thanks, I wasn't aware of this issue (I don't routinely try building with `-DBUILD_SHARED_LIBS=ON`, which I presume is what you've done to trigger this). See 592e935e115ffb451eb9b782376711dab6558fe0 for earlier context on this issue; therefore I'd prefer to fix this as I do in #71393; can you confirm if that change works for you as well? https://github.com/llvm/llvm-project/pull/66881 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-repl] Fix BUILD_SHARED_LIBS symbols from libclangInterpreter on MinGW (PR #71393)
mstorsjo wrote: CC @brechtsanders, this is an alternative to #66881. https://github.com/llvm/llvm-project/pull/71393 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-repl] Fix BUILD_SHARED_LIBS symbols from libclangInterpreter on MinGW (PR #71393)
https://github.com/mstorsjo created https://github.com/llvm/llvm-project/pull/71393 A few symbols within libclangInterpreter have got explicit dllexport attributes, in order to make them exported (and thus visible at runtime) in any build, not only when they are part of e.g. a DLL libclang-cpp, but also when they are part of a plain .exe. Due to the explicit dllexports, these symbols would sidestep the regular MinGW logic of exporting all symbols if there are no dllexports. Therefore, for libclang-cpp, a separate fix was made in 592e935e115ffb451eb9b782376711dab6558fe0, to pass --export-all-symbols to the build of libclang-cpp. If building with BUILD_SHARED_LIBS enabled, then the same issue appears in libclangInterpreter; pass the same flag --export-all-symbols there as well, to make sure all symbols are visible, not only the ones that are explicitly marked as dllexport. From 9de9617b46bd3c2ff4abd9446afc72c6e91f2493 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Storsj=C3=B6?= Date: Mon, 6 Nov 2023 15:42:42 +0200 Subject: [PATCH] [clang-repl] Fix BUILD_SHARED_LIBS symbols from libclangInterpreter on MinGW A few symbols within libclangInterpreter have got explicit dllexport attributes, in order to make them exported (and thus visible at runtime) in any build, not only when they are part of e.g. a DLL libclang-cpp, but also when they are part of a plain .exe. Due to the explicit dllexports, these symbols would sidestep the regular MinGW logic of exporting all symbols if there are no dllexports. Therefore, for libclang-cpp, a separate fix was made in 592e935e115ffb451eb9b782376711dab6558fe0, to pass --export-all-symbols to the build of libclang-cpp. If building with BUILD_SHARED_LIBS enabled, then the same issue appears in libclangInterpreter; pass the same flag --export-all-symbols there as well, to make sure all symbols are visible, not only the ones that are explicitly marked as dllexport. --- clang/lib/Interpreter/CMakeLists.txt | 11 +++ 1 file changed, 11 insertions(+) diff --git a/clang/lib/Interpreter/CMakeLists.txt b/clang/lib/Interpreter/CMakeLists.txt index 84f6ca5271d2ab0..9065f998f73c473 100644 --- a/clang/lib/Interpreter/CMakeLists.txt +++ b/clang/lib/Interpreter/CMakeLists.txt @@ -38,3 +38,14 @@ add_clang_library(clangInterpreter clangSema clangSerialization ) + +if ((MINGW OR CYGWIN) AND BUILD_SHARED_LIBS) + # The DLLs are supposed to export all symbols (except for ones that are + # explicitly hidden). Normally, this is what happens anyway, but if there + # are symbols that are marked explicitly as dllexport, we'd only export them + # and nothing else. The Interpreter contains a few cases of such dllexports + # (for symbols that need to be exported even from standalone exe files); + # therefore, add --export-all-symbols to make sure we export all symbols + # despite potential dllexports. + target_link_options(clangInterpreter PRIVATE LINKER:--export-all-symbols) +endif() ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[llvm] [clang] Reapply #2 [clang-repl] [test] Make an XFAIL more precise (#70991) (PR #71168)
mstorsjo wrote: Posting for a second review instead of just relanding the patch as is; in order to check the host triple, I had to add the `host=triple` string; it was previously only available for tests under `llvm/test`, but let's move it to the common llvm test configuration just like the `target=triple` strings, so that it is available for tests under clang as well. https://github.com/llvm/llvm-project/pull/71168 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] Reapply #2 [clang-repl] [test] Make an XFAIL more precise (#70991) (PR #71168)
https://github.com/mstorsjo created
https://github.com/llvm/llvm-project/pull/71168
The const.cpp testcase fails when running in MSVC mode, while it does succeed
in MinGW mode.
In MSVC mode, there are more constructor invocations than expected, as the
printout looks like this:
A(1), this = 02559793
A(1), this = 02559793
f: this = 02559793, val = 1
A(1), this = 02559793
f: this = 02559793, val = 1
~A, this = 02559793, val = 1
~A, this = 02559793, val = 1
~A, this = 02559793, val = 1
While the expected printout looks like this:
A(1), this = 02C903E1
f: this = 02C903E1, val = 1
f: this = 02C903E1, val = 1
~A, this = 02C903E1, val = 1
Reapplying with the XFAIL changed to check the host triple, not the target
triple. On an MSVC based build of Clang, but with the default target triple set
to PS4/PS5, we will still see the failure. And a Linux based build of Clang
that targets PS4/PS5 won't see the issue.
From 2fb453c2cdcf982aa2dd50208329785f8c338d45 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Martin=20Storsj=C3=B6?=
Date: Thu, 2 Nov 2023 09:51:33 +0200
Subject: [PATCH] Reapply #2 [clang-repl] [test] Make an XFAIL more precise
(#70991)
The const.cpp testcase fails when running in MSVC mode, while it does
succeed in MinGW mode.
In MSVC mode, there are more constructor invocations than expected, as
the printout looks like this:
A(1), this = 02559793
A(1), this = 02559793
f: this = 02559793, val = 1
A(1), this = 02559793
f: this = 02559793, val = 1
~A, this = 02559793, val = 1
~A, this = 02559793, val = 1
~A, this = 02559793, val = 1
While the expected printout looks like this:
A(1), this = 02C903E1
f: this = 02C903E1, val = 1
f: this = 02C903E1, val = 1
~A, this = 02C903E1, val = 1
Reapplying with the XFAIL changed to check the host triple,
not the target triple. On an MSVC based build of Clang, but
with the default target triple set to PS4/PS5, we will still
see the failure. And a Linux based build of Clang that targets
PS4/PS5 won't see the issue.
---
clang/test/Interpreter/const.cpp | 2 +-
llvm/test/lit.cfg.py | 4
llvm/utils/lit/lit/llvm/config.py | 1 +
3 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/clang/test/Interpreter/const.cpp b/clang/test/Interpreter/const.cpp
index 4b6ce65e3643e64..86358c1a54fbdde 100644
--- a/clang/test/Interpreter/const.cpp
+++ b/clang/test/Interpreter/const.cpp
@@ -1,6 +1,6 @@
// UNSUPPORTED: system-aix
// see https://github.com/llvm/llvm-project/issues/68092
-// XFAIL: system-windows
+// XFAIL: host={{.*}}-windows-msvc
// RUN: cat %s | clang-repl | FileCheck %s
// RUN: cat %s | clang-repl -Xcc -O2 | FileCheck %s
diff --git a/llvm/test/lit.cfg.py b/llvm/test/lit.cfg.py
index ab245b71cdd16a5..022d1aedbdcdbb6 100644
--- a/llvm/test/lit.cfg.py
+++ b/llvm/test/lit.cfg.py
@@ -477,10 +477,6 @@ def have_cxx_shared_library():
if not config.target_triple.startswith(("nvptx", "xcore")):
config.available_features.add("object-emission")
-# Allow checking for specific details in the host triple
-if config.host_triple:
-config.available_features.add('host=%s' % config.host_triple)
-
if config.have_llvm_driver:
config.available_features.add("llvm-driver")
diff --git a/llvm/utils/lit/lit/llvm/config.py
b/llvm/utils/lit/lit/llvm/config.py
index 16cc2968034bf74..79094b839e772e7 100644
--- a/llvm/utils/lit/lit/llvm/config.py
+++ b/llvm/utils/lit/lit/llvm/config.py
@@ -97,6 +97,7 @@ def __init__(self, lit_config, config):
# part of the standard header. But currently they aren't)
host_triple = getattr(config, "host_triple", None)
target_triple = getattr(config, "target_triple", None)
+features.add("host=%s" % host_triple)
features.add("target=%s" % target_triple)
if host_triple and host_triple == target_triple:
features.add("native")
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] 89a336a - Revert "Reapply [clang-repl] [test] Make an XFAIL more precise (#70991)"
Author: Martin Storsjö
Date: 2023-11-03T11:55:33+02:00
New Revision: 89a336add722f57f61c99b3eafab1c89f943db5e
URL:
https://github.com/llvm/llvm-project/commit/89a336add722f57f61c99b3eafab1c89f943db5e
DIFF:
https://github.com/llvm/llvm-project/commit/89a336add722f57f61c99b3eafab1c89f943db5e.diff
LOG: Revert "Reapply [clang-repl] [test] Make an XFAIL more precise (#70991)"
This reverts commit e9db60c05e2fb96ff40cbb1f78790abc5de9237e.
This was still failing (unexpectedly passing) on some Sony PS
buildbots.
The issue is that the clang-repl testcases don't depend on what
the default target triple is, but what the host triple is,
which is used for JIT purposes.
Added:
Modified:
clang/test/Interpreter/const.cpp
Removed:
diff --git a/clang/test/Interpreter/const.cpp
b/clang/test/Interpreter/const.cpp
index ca141d69f84d302..4b6ce65e3643e64 100644
--- a/clang/test/Interpreter/const.cpp
+++ b/clang/test/Interpreter/const.cpp
@@ -1,6 +1,6 @@
// UNSUPPORTED: system-aix
// see https://github.com/llvm/llvm-project/issues/68092
-// XFAIL: target={{.*}}-windows-msvc, target={{.*}}-ps4, target={{.*}}-ps5
+// XFAIL: system-windows
// RUN: cat %s | clang-repl | FileCheck %s
// RUN: cat %s | clang-repl -Xcc -O2 | FileCheck %s
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] e9db60c - Reapply [clang-repl] [test] Make an XFAIL more precise (#70991)
Author: Martin Storsjö
Date: 2023-11-03T11:30:08+02:00
New Revision: e9db60c05e2fb96ff40cbb1f78790abc5de9237e
URL:
https://github.com/llvm/llvm-project/commit/e9db60c05e2fb96ff40cbb1f78790abc5de9237e
DIFF:
https://github.com/llvm/llvm-project/commit/e9db60c05e2fb96ff40cbb1f78790abc5de9237e.diff
LOG: Reapply [clang-repl] [test] Make an XFAIL more precise (#70991)
The const.cpp testcase fails when running in MSVC mode, while it does
succeed in MinGW mode.
In MSVC mode, there are more constructor invocations than expected, as
the printout looks like this:
A(1), this = 02559793
A(1), this = 02559793
f: this = 02559793, val = 1
A(1), this = 02559793
f: this = 02559793, val = 1
~A, this = 02559793, val = 1
~A, this = 02559793, val = 1
~A, this = 02559793, val = 1
While the expected printout looks like this:
A(1), this = 02C903E1
f: this = 02C903E1, val = 1
f: this = 02C903E1, val = 1
~A, this = 02C903E1, val = 1
Reapplying with the XFAIL pattern expanded to include PS4/PS5
triples as well, which also seem to have the same behaviour
as MSVC.
Added:
Modified:
clang/test/Interpreter/const.cpp
Removed:
diff --git a/clang/test/Interpreter/const.cpp
b/clang/test/Interpreter/const.cpp
index 4b6ce65e3643e64..ca141d69f84d302 100644
--- a/clang/test/Interpreter/const.cpp
+++ b/clang/test/Interpreter/const.cpp
@@ -1,6 +1,6 @@
// UNSUPPORTED: system-aix
// see https://github.com/llvm/llvm-project/issues/68092
-// XFAIL: system-windows
+// XFAIL: target={{.*}}-windows-msvc, target={{.*}}-ps4, target={{.*}}-ps5
// RUN: cat %s | clang-repl | FileCheck %s
// RUN: cat %s | clang-repl -Xcc -O2 | FileCheck %s
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-repl] [test] Make an XFAIL more precise (PR #70991)
mstorsjo wrote:
> > > > If you still need help reproducing or debugging the issue on our bot,
> > > > please let me know.
> > >
> > >
> > > Thanks, much appreciated. Can you test if
> > > [mstorsjo@clang-repl-xfail](https://github.com/mstorsjo/llvm-project/commit/clang-repl-xfail)
> > > seems to run correctly in this environment? Otherwise I'll try to push
> > > it tomorrow and see how it fares on the bot.
>
> It failed, but due to a typo in the XFAIL you added:
>
> ```
> target={{.*}-ps4, target={{.*}-ps5
> ```
>
> These should have balanced braces. If I add the missing braces, the test
> passes with an XFAIL on the bot.
Oh, oops - but thanks for checking and verifying with the typo fixed. I'll try
to reland this change now then, with that fixed. Thanks again!
https://github.com/llvm/llvm-project/pull/70991
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-repl] [test] Make an XFAIL more precise (PR #70991)
mstorsjo wrote: > If you still need help reproducing or debugging the issue on our bot, please > let me know. Thanks, much appreciated. Can you test if https://github.com/mstorsjo/llvm-project/commit/clang-repl-xfail seems to run correctly in this environment? Otherwise I'll try to push it tomorrow and see how it fares on the bot. https://github.com/llvm/llvm-project/pull/70991 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-repl] [test] Make an XFAIL more precise (PR #70991)
mstorsjo wrote: > FTR, the "Worker" tab on that buildbot page will point you to the maintainer. Ah, there it is, I tried looking around, but overlooked that one... > But tagging me is also fine in general. Ok, thanks! > I'm unable to repro the problem locally because my local build doesn't seem > to include clang-repl.exe, so the whole clang/test/Interpreter directory is > Unsupported. Is there some cmake parameter to enable JIT? Not sure - by doing `ninja clang-repl` I was able to get those tests running at least. > If you want to XFAIL specifically for the Sony targets, what you suggested > would work. I'm unclear about the "MSVC C++ ABI" aspect, but if that gets the > test to work, go for it. Ok, great, thanks! I'll try that and reland the commit. https://github.com/llvm/llvm-project/pull/70991 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] b73d739 - Revert "[clang-repl] [test] Make an XFAIL more precise (#70991)"
Author: Martin Storsjö
Date: 2023-11-02T10:49:55+02:00
New Revision: b73d7390732b48014983aa9569e68c139f61bfcb
URL:
https://github.com/llvm/llvm-project/commit/b73d7390732b48014983aa9569e68c139f61bfcb
DIFF:
https://github.com/llvm/llvm-project/commit/b73d7390732b48014983aa9569e68c139f61bfcb.diff
LOG: Revert "[clang-repl] [test] Make an XFAIL more precise (#70991)"
This reverts commit 3bc056d5f0ebe9e4074afa088c3a0355f9ab901a.
This broke on bots with a target triple of x86_64-sie-ps5,
which also appear to behave like the MSVC case.
Added:
Modified:
clang/test/Interpreter/const.cpp
Removed:
diff --git a/clang/test/Interpreter/const.cpp
b/clang/test/Interpreter/const.cpp
index 1412a1d85d6f58f..4b6ce65e3643e64 100644
--- a/clang/test/Interpreter/const.cpp
+++ b/clang/test/Interpreter/const.cpp
@@ -1,6 +1,6 @@
// UNSUPPORTED: system-aix
// see https://github.com/llvm/llvm-project/issues/68092
-// XFAIL: target={{.*}}-windows-msvc
+// XFAIL: system-windows
// RUN: cat %s | clang-repl | FileCheck %s
// RUN: cat %s | clang-repl -Xcc -O2 | FileCheck %s
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-repl] [test] Make an XFAIL more precise (PR #70991)
mstorsjo wrote:
This broke on PS5 bots, like
https://lab.llvm.org/buildbot/#/builders/216/builds/29677; those are configured
with a triple like `x86_64-sie-ps5`, which seems to use an MSVC like C++ ABI
behaviour, so I pushed a revert.
Not sure whom to CC to pull in Sony people to discuss this matter, so trying
@pogo59. Can we use something like `XFAIL: target={{.*}}-windows-msvc,
target={{.*}-ps4, target={{.*}-ps5` to specifically point towards the Sony PS
triples that also use the MSVC C++ ABI here? The ideal would be something like
`XFAIL: default-target-is-msvc-cxx-abi`, but I don't think we have that. I see
triples `x86_64-scei-ps4` and `x86_64-sie-ps5` being mentioned elsewhere in
Clang tests as examples of PS4/PS5 triples.
https://github.com/llvm/llvm-project/pull/70991
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-repl] [test] Make an XFAIL more precise (PR #70991)
https://github.com/mstorsjo closed https://github.com/llvm/llvm-project/pull/70991 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-repl] [test] Make an XFAIL more precise (PR #70991)
mstorsjo wrote: > Very interesting... See also #68092, now I understand even less what the > problem is... No idea actually, but I tested passing `-Xcc --target=x86_64-w64-mingw32` to an MSVC-built clang-repl, and then it outputs the expected things. Not sure at what level some JIT deduplication should be happening, but anyway, the MSVC C++ ABI is distinctly different from the Itanium C++ ABI, in most aspects. https://github.com/llvm/llvm-project/pull/70991 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-repl] [test] Make an XFAIL more precise (PR #70991)
https://github.com/mstorsjo created
https://github.com/llvm/llvm-project/pull/70991
The const.cpp testcase fails when running in MSVC mode, while it does succeed
in MinGW mode.
In MSVC mode, there are more constructor invocations than expected, as the
printout looks like this:
A(1), this = 02559793
A(1), this = 02559793
f: this = 02559793, val = 1
A(1), this = 02559793
f: this = 02559793, val = 1
~A, this = 02559793, val = 1
~A, this = 02559793, val = 1
~A, this = 02559793, val = 1
While the expected printout looks like this:
A(1), this = 02C903E1
f: this = 02C903E1, val = 1
f: this = 02C903E1, val = 1
~A, this = 02C903E1, val = 1
From 83554507ba60e8d8d0864176f2f0629c3bb7e75a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Martin=20Storsj=C3=B6?=
Date: Wed, 1 Nov 2023 23:35:43 +0200
Subject: [PATCH] [clang-repl] [test] Make an XFAIL more precise
The const.cpp testcase fails when running in MSVC mode, while they
do succeed in MinGW mode.
In MSVC mode, there are more constructor invocations than expected,
as the printout looks like this:
A(1), this = 02559793
A(1), this = 02559793
f: this = 02559793, val = 1
A(1), this = 02559793
f: this = 02559793, val = 1
~A, this = 02559793, val = 1
~A, this = 02559793, val = 1
~A, this = 02559793, val = 1
While the expected printout looks like this:
A(1), this = 02C903E1
f: this = 02C903E1, val = 1
f: this = 02C903E1, val = 1
~A, this = 02C903E1, val = 1
---
clang/test/Interpreter/const.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/clang/test/Interpreter/const.cpp b/clang/test/Interpreter/const.cpp
index 4b6ce65e3643e64..1412a1d85d6f58f 100644
--- a/clang/test/Interpreter/const.cpp
+++ b/clang/test/Interpreter/const.cpp
@@ -1,6 +1,6 @@
// UNSUPPORTED: system-aix
// see https://github.com/llvm/llvm-project/issues/68092
-// XFAIL: system-windows
+// XFAIL: target={{.*}}-windows-msvc
// RUN: cat %s | clang-repl | FileCheck %s
// RUN: cat %s | clang-repl -Xcc -O2 | FileCheck %s
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[flang] [clang] [flang][windows] Add option to link against specific MSVC CRT (PR #70833)
@@ -53,3 +53,26 @@ add_flang_library(FortranDecimal INSTALL_WITH_TOOLCHAIN binary-to-decimal.cpp decimal-to-binary.cpp ) + +if (DEFINED MSVC) + set(CMAKE_MSVC_RUNTIME_LIBRARY MultiThreaded) mstorsjo wrote: Instead of redefining `CMAKE_MSVC_RUNTIME_LIBRARY` repeatedly, if you really want to set it specifically for one library, it's better to set the `MSVC_RUNTIME_LIBRARY` target property instead - see https://cmake.org/cmake/help/latest/prop_tgt/MSVC_RUNTIME_LIBRARY.html. https://github.com/llvm/llvm-project/pull/70833 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] [Gnu] Improve GCCVersion parsing to match versions such as "10-win32" (PR #69079)
https://github.com/mstorsjo closed https://github.com/llvm/llvm-project/pull/69079 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] [Gnu] Improve GCCVersion parsing to match versions such as "10-win32" (PR #69079)
https://github.com/mstorsjo updated
https://github.com/llvm/llvm-project/pull/69079
From df2dba040dadb5e3222b44b41ea92978d9ddafed Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Martin=20Storsj=C3=B6?=
Date: Sat, 14 Oct 2023 00:55:18 +0300
Subject: [PATCH 1/4] [clang] [Gnu] Improve GCCVersion parsing to match
versions such as "10-win32"
In earlier GCC versions, the Debian/Ubuntu provided mingw toolchains
were packaged in /usr/lib/gcc/ with version strings such
as "5.3-win32", which were matched and found since
6afcd64eb65fca233a7b173f88cffb2c2c9c114c. However in recent versions,
they have stopped including the minor version number and only
have version strings such as "10-win32" and "10-posix".
Generalize the parsing code to tolerate the patch suffix to be
present on a version number with only a major number.
Refactor the string parsing code to highlight the overall structure
of the parsing. This implementation should yield the same result
as before, except for when there's only one segment and it has
trailing, non-number contents.
This allows Clang to find the GCC libraries and headers in
Debian/Ubuntu provided MinGW cross compilers.
---
clang/lib/Driver/ToolChains/Gnu.cpp | 82 +++
clang/unittests/Driver/GCCVersionTest.cpp | 1 +
2 files changed, 55 insertions(+), 28 deletions(-)
diff --git a/clang/lib/Driver/ToolChains/Gnu.cpp
b/clang/lib/Driver/ToolChains/Gnu.cpp
index cdd911af9a73361..e6f94836c4110a1 100644
--- a/clang/lib/Driver/ToolChains/Gnu.cpp
+++ b/clang/lib/Driver/ToolChains/Gnu.cpp
@@ -2007,45 +2007,71 @@ Generic_GCC::GCCVersion
Generic_GCC::GCCVersion::Parse(StringRef VersionText) {
std::pair First = VersionText.split('.');
std::pair Second = First.second.split('.');
- GCCVersion GoodVersion = {VersionText.str(), -1, -1, -1, "", "", ""};
- if (First.first.getAsInteger(10, GoodVersion.Major) || GoodVersion.Major < 0)
-return BadVersion;
- GoodVersion.MajorStr = First.first.str();
- if (First.second.empty())
-return GoodVersion;
+ StringRef MajorStr = First.first;
StringRef MinorStr = Second.first;
- if (Second.second.empty()) {
-if (size_t EndNumber = MinorStr.find_first_not_of("0123456789")) {
- GoodVersion.PatchSuffix = std::string(MinorStr.substr(EndNumber));
- MinorStr = MinorStr.slice(0, EndNumber);
-}
- }
- if (MinorStr.getAsInteger(10, GoodVersion.Minor) || GoodVersion.Minor < 0)
-return BadVersion;
- GoodVersion.MinorStr = MinorStr.str();
+ StringRef PatchStr = Second.second;
- // First look for a number prefix and parse that if present. Otherwise just
- // stash the entire patch string in the suffix, and leave the number
- // unspecified. This covers versions strings such as:
- // 5(handled above)
+ GCCVersion GoodVersion = {VersionText.str(), -1, -1, -1, "", "", ""};
+
+ // Parse version number strings such as:
+ // 5
// 4.4
// 4.4-patched
// 4.4.0
// 4.4.x
// 4.4.2-rc4
// 4.4.x-patched
- // And retains any patch number it finds.
- StringRef PatchText = Second.second;
- if (!PatchText.empty()) {
-if (size_t EndNumber = PatchText.find_first_not_of("0123456789")) {
- // Try to parse the number and any suffix.
- if (PatchText.slice(0, EndNumber).getAsInteger(10, GoodVersion.Patch) ||
- GoodVersion.Patch < 0)
-return BadVersion;
- GoodVersion.PatchSuffix = std::string(PatchText.substr(EndNumber));
+ // 10-win32
+ // Split on '.', handle 1, 2 or 3 such segments. Each segment must contain
+ // purely a number, except for the last one, where a non-number suffix
+ // is stored in PatchSuffix. The third segment is allowed to not contain
+ // a number at all.
+
+ auto HandleLastNumber = [&](StringRef Segment, int &Number,
+ std::string &OutStr) -> bool {
+// Look for a number prefix and parse that, and split out any trailing
+// string into GoodVersion.PatchSuffix.
+
+if (size_t EndNumber = Segment.find_first_not_of("0123456789")) {
+ StringRef NumberStr = Segment.slice(0, EndNumber);
+ if (NumberStr.getAsInteger(10, Number) || Number < 0)
+return false;
+ OutStr = NumberStr;
+ GoodVersion.PatchSuffix = Segment.substr(EndNumber);
+ return true;
}
+return false;
+ };
+ auto HandleNumber = [](StringRef Segment, int &Number) -> bool {
+if (Segment.getAsInteger(10, Number) || Number < 0)
+ return false;
+return true;
+ };
+
+ if (MinorStr.empty()) {
+// If no minor string, major is the last segment
+if (!HandleLastNumber(MajorStr, GoodVersion.Major, GoodVersion.MajorStr))
+ return BadVersion;
+return GoodVersion;
+ } else {
+if (!HandleNumber(MajorStr, GoodVersion.Major))
+ return BadVersion;
+GoodVersion.MajorStr = MajorStr;
+ }
+ if (PatchStr.empty()) {
+// If no patch string, minor is the last segment
+if (!HandleLastNumber(MinorStr, GoodVersion.Minor, GoodVersion.MinorStr))
+
[clang] [clang] [Gnu] Improve GCCVersion parsing to match versions such as "10-win32" (PR #69079)
https://github.com/mstorsjo updated
https://github.com/llvm/llvm-project/pull/69079
From df2dba040dadb5e3222b44b41ea92978d9ddafed Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Martin=20Storsj=C3=B6?=
Date: Sat, 14 Oct 2023 00:55:18 +0300
Subject: [PATCH 1/3] [clang] [Gnu] Improve GCCVersion parsing to match
versions such as "10-win32"
In earlier GCC versions, the Debian/Ubuntu provided mingw toolchains
were packaged in /usr/lib/gcc/ with version strings such
as "5.3-win32", which were matched and found since
6afcd64eb65fca233a7b173f88cffb2c2c9c114c. However in recent versions,
they have stopped including the minor version number and only
have version strings such as "10-win32" and "10-posix".
Generalize the parsing code to tolerate the patch suffix to be
present on a version number with only a major number.
Refactor the string parsing code to highlight the overall structure
of the parsing. This implementation should yield the same result
as before, except for when there's only one segment and it has
trailing, non-number contents.
This allows Clang to find the GCC libraries and headers in
Debian/Ubuntu provided MinGW cross compilers.
---
clang/lib/Driver/ToolChains/Gnu.cpp | 82 +++
clang/unittests/Driver/GCCVersionTest.cpp | 1 +
2 files changed, 55 insertions(+), 28 deletions(-)
diff --git a/clang/lib/Driver/ToolChains/Gnu.cpp
b/clang/lib/Driver/ToolChains/Gnu.cpp
index cdd911af9a73361..e6f94836c4110a1 100644
--- a/clang/lib/Driver/ToolChains/Gnu.cpp
+++ b/clang/lib/Driver/ToolChains/Gnu.cpp
@@ -2007,45 +2007,71 @@ Generic_GCC::GCCVersion
Generic_GCC::GCCVersion::Parse(StringRef VersionText) {
std::pair First = VersionText.split('.');
std::pair Second = First.second.split('.');
- GCCVersion GoodVersion = {VersionText.str(), -1, -1, -1, "", "", ""};
- if (First.first.getAsInteger(10, GoodVersion.Major) || GoodVersion.Major < 0)
-return BadVersion;
- GoodVersion.MajorStr = First.first.str();
- if (First.second.empty())
-return GoodVersion;
+ StringRef MajorStr = First.first;
StringRef MinorStr = Second.first;
- if (Second.second.empty()) {
-if (size_t EndNumber = MinorStr.find_first_not_of("0123456789")) {
- GoodVersion.PatchSuffix = std::string(MinorStr.substr(EndNumber));
- MinorStr = MinorStr.slice(0, EndNumber);
-}
- }
- if (MinorStr.getAsInteger(10, GoodVersion.Minor) || GoodVersion.Minor < 0)
-return BadVersion;
- GoodVersion.MinorStr = MinorStr.str();
+ StringRef PatchStr = Second.second;
- // First look for a number prefix and parse that if present. Otherwise just
- // stash the entire patch string in the suffix, and leave the number
- // unspecified. This covers versions strings such as:
- // 5(handled above)
+ GCCVersion GoodVersion = {VersionText.str(), -1, -1, -1, "", "", ""};
+
+ // Parse version number strings such as:
+ // 5
// 4.4
// 4.4-patched
// 4.4.0
// 4.4.x
// 4.4.2-rc4
// 4.4.x-patched
- // And retains any patch number it finds.
- StringRef PatchText = Second.second;
- if (!PatchText.empty()) {
-if (size_t EndNumber = PatchText.find_first_not_of("0123456789")) {
- // Try to parse the number and any suffix.
- if (PatchText.slice(0, EndNumber).getAsInteger(10, GoodVersion.Patch) ||
- GoodVersion.Patch < 0)
-return BadVersion;
- GoodVersion.PatchSuffix = std::string(PatchText.substr(EndNumber));
+ // 10-win32
+ // Split on '.', handle 1, 2 or 3 such segments. Each segment must contain
+ // purely a number, except for the last one, where a non-number suffix
+ // is stored in PatchSuffix. The third segment is allowed to not contain
+ // a number at all.
+
+ auto HandleLastNumber = [&](StringRef Segment, int &Number,
+ std::string &OutStr) -> bool {
+// Look for a number prefix and parse that, and split out any trailing
+// string into GoodVersion.PatchSuffix.
+
+if (size_t EndNumber = Segment.find_first_not_of("0123456789")) {
+ StringRef NumberStr = Segment.slice(0, EndNumber);
+ if (NumberStr.getAsInteger(10, Number) || Number < 0)
+return false;
+ OutStr = NumberStr;
+ GoodVersion.PatchSuffix = Segment.substr(EndNumber);
+ return true;
}
+return false;
+ };
+ auto HandleNumber = [](StringRef Segment, int &Number) -> bool {
+if (Segment.getAsInteger(10, Number) || Number < 0)
+ return false;
+return true;
+ };
+
+ if (MinorStr.empty()) {
+// If no minor string, major is the last segment
+if (!HandleLastNumber(MajorStr, GoodVersion.Major, GoodVersion.MajorStr))
+ return BadVersion;
+return GoodVersion;
+ } else {
+if (!HandleNumber(MajorStr, GoodVersion.Major))
+ return BadVersion;
+GoodVersion.MajorStr = MajorStr;
+ }
+ if (PatchStr.empty()) {
+// If no patch string, minor is the last segment
+if (!HandleLastNumber(MinorStr, GoodVersion.Minor, GoodVersion.MinorStr))
+
[clang] [clang] [Gnu] Improve GCCVersion parsing to match versions such as "10-win32" (PR #69079)
@@ -2007,45 +2007,71 @@ Generic_GCC::GCCVersion
Generic_GCC::GCCVersion::Parse(StringRef VersionText) {
std::pair First = VersionText.split('.');
std::pair Second = First.second.split('.');
- GCCVersion GoodVersion = {VersionText.str(), -1, -1, -1, "", "", ""};
- if (First.first.getAsInteger(10, GoodVersion.Major) || GoodVersion.Major < 0)
-return BadVersion;
- GoodVersion.MajorStr = First.first.str();
- if (First.second.empty())
-return GoodVersion;
+ StringRef MajorStr = First.first;
StringRef MinorStr = Second.first;
- if (Second.second.empty()) {
-if (size_t EndNumber = MinorStr.find_first_not_of("0123456789")) {
- GoodVersion.PatchSuffix = std::string(MinorStr.substr(EndNumber));
- MinorStr = MinorStr.slice(0, EndNumber);
-}
- }
- if (MinorStr.getAsInteger(10, GoodVersion.Minor) || GoodVersion.Minor < 0)
-return BadVersion;
- GoodVersion.MinorStr = MinorStr.str();
+ StringRef PatchStr = Second.second;
- // First look for a number prefix and parse that if present. Otherwise just
- // stash the entire patch string in the suffix, and leave the number
- // unspecified. This covers versions strings such as:
- // 5(handled above)
+ GCCVersion GoodVersion = {VersionText.str(), -1, -1, -1, "", "", ""};
+
+ // Parse version number strings such as:
+ // 5
// 4.4
// 4.4-patched
// 4.4.0
// 4.4.x
// 4.4.2-rc4
// 4.4.x-patched
- // And retains any patch number it finds.
- StringRef PatchText = Second.second;
- if (!PatchText.empty()) {
-if (size_t EndNumber = PatchText.find_first_not_of("0123456789")) {
- // Try to parse the number and any suffix.
- if (PatchText.slice(0, EndNumber).getAsInteger(10, GoodVersion.Patch) ||
- GoodVersion.Patch < 0)
-return BadVersion;
- GoodVersion.PatchSuffix = std::string(PatchText.substr(EndNumber));
+ // 10-win32
+ // Split on '.', handle 1, 2 or 3 such segments. Each segment must contain
+ // purely a number, except for the last one, where a non-number suffix
+ // is stored in PatchSuffix. The third segment is allowed to not contain
+ // a number at all.
+
+ auto HandleLastNumber = [&](StringRef Segment, int &Number,
+ std::string &OutStr) -> bool {
+// Look for a number prefix and parse that, and split out any trailing
+// string into GoodVersion.PatchSuffix.
+
+if (size_t EndNumber = Segment.find_first_not_of("0123456789")) {
+ StringRef NumberStr = Segment.slice(0, EndNumber);
+ if (NumberStr.getAsInteger(10, Number) || Number < 0)
+return false;
+ OutStr = NumberStr;
+ GoodVersion.PatchSuffix = Segment.substr(EndNumber);
+ return true;
}
+return false;
+ };
+ auto HandleNumber = [](StringRef Segment, int &Number) -> bool {
+if (Segment.getAsInteger(10, Number) || Number < 0)
+ return false;
+return true;
+ };
+
+ if (MinorStr.empty()) {
+// If no minor string, major is the last segment
+if (!HandleLastNumber(MajorStr, GoodVersion.Major, GoodVersion.MajorStr))
+ return BadVersion;
+return GoodVersion;
+ } else {
+if (!HandleNumber(MajorStr, GoodVersion.Major))
+ return BadVersion;
+GoodVersion.MajorStr = MajorStr;
+ }
+ if (PatchStr.empty()) {
+// If no patch string, minor is the last segment
+if (!HandleLastNumber(MinorStr, GoodVersion.Minor, GoodVersion.MinorStr))
+ return BadVersion;
+return GoodVersion;
+ } else {
+if (!HandleNumber(MinorStr, GoodVersion.Minor))
+ return BadVersion;
+GoodVersion.MinorStr = MinorStr;
}
+ // For the last segment, tolerate a missing number.
+ std::string DummyStr;
+ HandleLastNumber(PatchStr, GoodVersion.Patch, DummyStr);
mstorsjo wrote:
Ah, I see.
As the snippet looks like this:
```
if (size_t EndNumber = Segment.find_first_not_of("0123456789")) {
StringRef NumberStr = Segment.slice(0, EndNumber);
if (NumberStr.getAsInteger(10, Number) || Number < 0)
return false;
```
Due to the `find_first_not_of`, the substring `NumberStr` can only contain the
chars `[0-9]` (and `EndNumber` must be nonzero here), so the integer parsing
really should succeed (unless it's out of range for a regular int?), and can't
really be negative either (as the string can't contain a leading `-`).
In practice, `1.2.3-4` does get parsed as one would like. However the
`find_first_not_of` also has the effect that the `PatchSuffix` doesn't really
need to start with a dash either; if we parse `1.2.3x4`, we get
Major/Minor/Patch set as 1, 2, 3, and `PatchSuffix` set to `x4`.
I'm not sure if this is the ideal implementation or not, I'm mostly keeping
this untouched and just abstracts away to apply it at any of the positions in
the version string.
https://github.com/llvm/llvm-project/pull/69079
___
cfe-commits mailing list
cfe-commits@lists.llvm.o
[clang] [clang] [Gnu] Improve GCCVersion parsing to match versions such as "10-win32" (PR #69079)
https://github.com/mstorsjo updated
https://github.com/llvm/llvm-project/pull/69079
From df2dba040dadb5e3222b44b41ea92978d9ddafed Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Martin=20Storsj=C3=B6?=
Date: Sat, 14 Oct 2023 00:55:18 +0300
Subject: [PATCH 1/2] [clang] [Gnu] Improve GCCVersion parsing to match
versions such as "10-win32"
In earlier GCC versions, the Debian/Ubuntu provided mingw toolchains
were packaged in /usr/lib/gcc/ with version strings such
as "5.3-win32", which were matched and found since
6afcd64eb65fca233a7b173f88cffb2c2c9c114c. However in recent versions,
they have stopped including the minor version number and only
have version strings such as "10-win32" and "10-posix".
Generalize the parsing code to tolerate the patch suffix to be
present on a version number with only a major number.
Refactor the string parsing code to highlight the overall structure
of the parsing. This implementation should yield the same result
as before, except for when there's only one segment and it has
trailing, non-number contents.
This allows Clang to find the GCC libraries and headers in
Debian/Ubuntu provided MinGW cross compilers.
---
clang/lib/Driver/ToolChains/Gnu.cpp | 82 +++
clang/unittests/Driver/GCCVersionTest.cpp | 1 +
2 files changed, 55 insertions(+), 28 deletions(-)
diff --git a/clang/lib/Driver/ToolChains/Gnu.cpp
b/clang/lib/Driver/ToolChains/Gnu.cpp
index cdd911af9a73361..e6f94836c4110a1 100644
--- a/clang/lib/Driver/ToolChains/Gnu.cpp
+++ b/clang/lib/Driver/ToolChains/Gnu.cpp
@@ -2007,45 +2007,71 @@ Generic_GCC::GCCVersion
Generic_GCC::GCCVersion::Parse(StringRef VersionText) {
std::pair First = VersionText.split('.');
std::pair Second = First.second.split('.');
- GCCVersion GoodVersion = {VersionText.str(), -1, -1, -1, "", "", ""};
- if (First.first.getAsInteger(10, GoodVersion.Major) || GoodVersion.Major < 0)
-return BadVersion;
- GoodVersion.MajorStr = First.first.str();
- if (First.second.empty())
-return GoodVersion;
+ StringRef MajorStr = First.first;
StringRef MinorStr = Second.first;
- if (Second.second.empty()) {
-if (size_t EndNumber = MinorStr.find_first_not_of("0123456789")) {
- GoodVersion.PatchSuffix = std::string(MinorStr.substr(EndNumber));
- MinorStr = MinorStr.slice(0, EndNumber);
-}
- }
- if (MinorStr.getAsInteger(10, GoodVersion.Minor) || GoodVersion.Minor < 0)
-return BadVersion;
- GoodVersion.MinorStr = MinorStr.str();
+ StringRef PatchStr = Second.second;
- // First look for a number prefix and parse that if present. Otherwise just
- // stash the entire patch string in the suffix, and leave the number
- // unspecified. This covers versions strings such as:
- // 5(handled above)
+ GCCVersion GoodVersion = {VersionText.str(), -1, -1, -1, "", "", ""};
+
+ // Parse version number strings such as:
+ // 5
// 4.4
// 4.4-patched
// 4.4.0
// 4.4.x
// 4.4.2-rc4
// 4.4.x-patched
- // And retains any patch number it finds.
- StringRef PatchText = Second.second;
- if (!PatchText.empty()) {
-if (size_t EndNumber = PatchText.find_first_not_of("0123456789")) {
- // Try to parse the number and any suffix.
- if (PatchText.slice(0, EndNumber).getAsInteger(10, GoodVersion.Patch) ||
- GoodVersion.Patch < 0)
-return BadVersion;
- GoodVersion.PatchSuffix = std::string(PatchText.substr(EndNumber));
+ // 10-win32
+ // Split on '.', handle 1, 2 or 3 such segments. Each segment must contain
+ // purely a number, except for the last one, where a non-number suffix
+ // is stored in PatchSuffix. The third segment is allowed to not contain
+ // a number at all.
+
+ auto HandleLastNumber = [&](StringRef Segment, int &Number,
+ std::string &OutStr) -> bool {
+// Look for a number prefix and parse that, and split out any trailing
+// string into GoodVersion.PatchSuffix.
+
+if (size_t EndNumber = Segment.find_first_not_of("0123456789")) {
+ StringRef NumberStr = Segment.slice(0, EndNumber);
+ if (NumberStr.getAsInteger(10, Number) || Number < 0)
+return false;
+ OutStr = NumberStr;
+ GoodVersion.PatchSuffix = Segment.substr(EndNumber);
+ return true;
}
+return false;
+ };
+ auto HandleNumber = [](StringRef Segment, int &Number) -> bool {
+if (Segment.getAsInteger(10, Number) || Number < 0)
+ return false;
+return true;
+ };
+
+ if (MinorStr.empty()) {
+// If no minor string, major is the last segment
+if (!HandleLastNumber(MajorStr, GoodVersion.Major, GoodVersion.MajorStr))
+ return BadVersion;
+return GoodVersion;
+ } else {
+if (!HandleNumber(MajorStr, GoodVersion.Major))
+ return BadVersion;
+GoodVersion.MajorStr = MajorStr;
+ }
+ if (PatchStr.empty()) {
+// If no patch string, minor is the last segment
+if (!HandleLastNumber(MinorStr, GoodVersion.Minor, GoodVersion.MinorStr))
+
[clang] [clang] [Gnu] Improve GCCVersion parsing to match versions such as "10-win32" (PR #69079)
@@ -2007,45 +2007,71 @@ Generic_GCC::GCCVersion
Generic_GCC::GCCVersion::Parse(StringRef VersionText) {
std::pair First = VersionText.split('.');
std::pair Second = First.second.split('.');
- GCCVersion GoodVersion = {VersionText.str(), -1, -1, -1, "", "", ""};
- if (First.first.getAsInteger(10, GoodVersion.Major) || GoodVersion.Major < 0)
-return BadVersion;
- GoodVersion.MajorStr = First.first.str();
- if (First.second.empty())
-return GoodVersion;
+ StringRef MajorStr = First.first;
StringRef MinorStr = Second.first;
- if (Second.second.empty()) {
-if (size_t EndNumber = MinorStr.find_first_not_of("0123456789")) {
- GoodVersion.PatchSuffix = std::string(MinorStr.substr(EndNumber));
- MinorStr = MinorStr.slice(0, EndNumber);
-}
- }
- if (MinorStr.getAsInteger(10, GoodVersion.Minor) || GoodVersion.Minor < 0)
-return BadVersion;
- GoodVersion.MinorStr = MinorStr.str();
+ StringRef PatchStr = Second.second;
- // First look for a number prefix and parse that if present. Otherwise just
- // stash the entire patch string in the suffix, and leave the number
- // unspecified. This covers versions strings such as:
- // 5(handled above)
+ GCCVersion GoodVersion = {VersionText.str(), -1, -1, -1, "", "", ""};
+
+ // Parse version number strings such as:
+ // 5
// 4.4
// 4.4-patched
// 4.4.0
// 4.4.x
// 4.4.2-rc4
// 4.4.x-patched
- // And retains any patch number it finds.
- StringRef PatchText = Second.second;
- if (!PatchText.empty()) {
-if (size_t EndNumber = PatchText.find_first_not_of("0123456789")) {
- // Try to parse the number and any suffix.
- if (PatchText.slice(0, EndNumber).getAsInteger(10, GoodVersion.Patch) ||
- GoodVersion.Patch < 0)
-return BadVersion;
- GoodVersion.PatchSuffix = std::string(PatchText.substr(EndNumber));
+ // 10-win32
+ // Split on '.', handle 1, 2 or 3 such segments. Each segment must contain
+ // purely a number, except for the last one, where a non-number suffix
+ // is stored in PatchSuffix. The third segment is allowed to not contain
+ // a number at all.
+
+ auto HandleLastNumber = [&](StringRef Segment, int &Number,
+ std::string &OutStr) -> bool {
+// Look for a number prefix and parse that, and split out any trailing
+// string into GoodVersion.PatchSuffix.
+
+if (size_t EndNumber = Segment.find_first_not_of("0123456789")) {
+ StringRef NumberStr = Segment.slice(0, EndNumber);
+ if (NumberStr.getAsInteger(10, Number) || Number < 0)
+return false;
+ OutStr = NumberStr;
+ GoodVersion.PatchSuffix = Segment.substr(EndNumber);
+ return true;
}
+return false;
+ };
+ auto HandleNumber = [](StringRef Segment, int &Number) -> bool {
+if (Segment.getAsInteger(10, Number) || Number < 0)
+ return false;
+return true;
+ };
+
+ if (MinorStr.empty()) {
+// If no minor string, major is the last segment
+if (!HandleLastNumber(MajorStr, GoodVersion.Major, GoodVersion.MajorStr))
+ return BadVersion;
+return GoodVersion;
+ } else {
+if (!HandleNumber(MajorStr, GoodVersion.Major))
+ return BadVersion;
+GoodVersion.MajorStr = MajorStr;
+ }
+ if (PatchStr.empty()) {
+// If no patch string, minor is the last segment
+if (!HandleLastNumber(MinorStr, GoodVersion.Minor, GoodVersion.MinorStr))
+ return BadVersion;
+return GoodVersion;
+ } else {
+if (!HandleNumber(MinorStr, GoodVersion.Minor))
+ return BadVersion;
+GoodVersion.MinorStr = MinorStr;
}
+ // For the last segment, tolerate a missing number.
+ std::string DummyStr;
+ HandleLastNumber(PatchStr, GoodVersion.Patch, DummyStr);
mstorsjo wrote:
This implementation does parse `10-10` as `Major=10`, the rest left at -1, and
`PatchSuffix="-10"`.
I'm not sure exactly which bit gives you the impression that case wouldn't get
handled like that. The comment above ("For the last segment, tolerate a missing
number") only means that for the case `4.4.x-patched`, we don't return an error
even if the last bit is `x-patched`, but we return what we've parsed up to that
point.
https://github.com/llvm/llvm-project/pull/69079
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] [Gnu] Improve GCCVersion parsing to match versions such as "10-win32" (PR #69079)
@@ -2007,45 +2007,71 @@ Generic_GCC::GCCVersion
Generic_GCC::GCCVersion::Parse(StringRef VersionText) {
std::pair First = VersionText.split('.');
std::pair Second = First.second.split('.');
- GCCVersion GoodVersion = {VersionText.str(), -1, -1, -1, "", "", ""};
- if (First.first.getAsInteger(10, GoodVersion.Major) || GoodVersion.Major < 0)
-return BadVersion;
- GoodVersion.MajorStr = First.first.str();
- if (First.second.empty())
-return GoodVersion;
+ StringRef MajorStr = First.first;
StringRef MinorStr = Second.first;
- if (Second.second.empty()) {
-if (size_t EndNumber = MinorStr.find_first_not_of("0123456789")) {
- GoodVersion.PatchSuffix = std::string(MinorStr.substr(EndNumber));
- MinorStr = MinorStr.slice(0, EndNumber);
-}
- }
- if (MinorStr.getAsInteger(10, GoodVersion.Minor) || GoodVersion.Minor < 0)
-return BadVersion;
- GoodVersion.MinorStr = MinorStr.str();
+ StringRef PatchStr = Second.second;
- // First look for a number prefix and parse that if present. Otherwise just
- // stash the entire patch string in the suffix, and leave the number
- // unspecified. This covers versions strings such as:
- // 5(handled above)
+ GCCVersion GoodVersion = {VersionText.str(), -1, -1, -1, "", "", ""};
+
+ // Parse version number strings such as:
+ // 5
// 4.4
// 4.4-patched
// 4.4.0
// 4.4.x
// 4.4.2-rc4
// 4.4.x-patched
- // And retains any patch number it finds.
- StringRef PatchText = Second.second;
- if (!PatchText.empty()) {
-if (size_t EndNumber = PatchText.find_first_not_of("0123456789")) {
- // Try to parse the number and any suffix.
- if (PatchText.slice(0, EndNumber).getAsInteger(10, GoodVersion.Patch) ||
- GoodVersion.Patch < 0)
-return BadVersion;
- GoodVersion.PatchSuffix = std::string(PatchText.substr(EndNumber));
+ // 10-win32
+ // Split on '.', handle 1, 2 or 3 such segments. Each segment must contain
+ // purely a number, except for the last one, where a non-number suffix
+ // is stored in PatchSuffix. The third segment is allowed to not contain
+ // a number at all.
+
+ auto HandleLastNumber = [&](StringRef Segment, int &Number,
+ std::string &OutStr) -> bool {
+// Look for a number prefix and parse that, and split out any trailing
+// string into GoodVersion.PatchSuffix.
+
+if (size_t EndNumber = Segment.find_first_not_of("0123456789")) {
+ StringRef NumberStr = Segment.slice(0, EndNumber);
+ if (NumberStr.getAsInteger(10, Number) || Number < 0)
+return false;
+ OutStr = NumberStr;
+ GoodVersion.PatchSuffix = Segment.substr(EndNumber);
+ return true;
}
+return false;
+ };
+ auto HandleNumber = [](StringRef Segment, int &Number) -> bool {
mstorsjo wrote:
Thanks, will do.
https://github.com/llvm/llvm-project/pull/69079
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] [Gnu] Improve GCCVersion parsing to match versions such as "10-win32" (PR #69079)
mstorsjo wrote: Ping https://github.com/llvm/llvm-project/pull/69079 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] [unittest] Add a test for Generic_GCC::GCCVersion::Parse (PR #69078)
mstorsjo wrote: > ```c > #if !defined(LLVM_BUILD_SHARED_LIBS) > ``` > > is not great but is not too bad. `-DBUILD_SHARED_LIBS=on` modes are slow to > execute tests and are not used often for Release builds. I went ahead and relanded this now, in 538b7ba2aacd6e400ee63c4f9ff1c2543ae69a90, with `#if !defined(LLVM_BUILD_LLVM_DYLIB) && !defined(LLVM_BUILD_SHARED_LIBS)`. Not great, but probably the least bad compromise. https://github.com/llvm/llvm-project/pull/69078 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] 538b7ba - Reland [clang] [unittest] Add a test for Generic_GCC::GCCVersion::Parse (#69078)
Author: Martin Storsjö
Date: 2023-10-20T23:34:28+03:00
New Revision: 538b7ba2aacd6e400ee63c4f9ff1c2543ae69a90
URL:
https://github.com/llvm/llvm-project/commit/538b7ba2aacd6e400ee63c4f9ff1c2543ae69a90
DIFF:
https://github.com/llvm/llvm-project/commit/538b7ba2aacd6e400ee63c4f9ff1c2543ae69a90.diff
LOG: Reland [clang] [unittest] Add a test for Generic_GCC::GCCVersion::Parse
(#69078)
This adds actual test cases for all the cases that are listed in a code
comment in the implementation of this function; having such test
coverage eases doing further modifications to the function.
This relands b4b35a5d2b4ee26bf79b8a92715dd200f3f9cc49. This time,
the new test is excluded if building with dylibs or shared libraries
enabled, as the clang::toolchains::Generic_GCC class is marked
LLVM_LIBRARY_VISIBILITY, giving it hidden visibility in such builds,
making it unreferencable outside of the dylib/shared library.
Added:
clang/unittests/Driver/GCCVersionTest.cpp
Modified:
clang/unittests/Driver/CMakeLists.txt
Removed:
diff --git a/clang/unittests/Driver/CMakeLists.txt
b/clang/unittests/Driver/CMakeLists.txt
index e37c158d7137a88..752037f78fb147d 100644
--- a/clang/unittests/Driver/CMakeLists.txt
+++ b/clang/unittests/Driver/CMakeLists.txt
@@ -9,6 +9,7 @@ set(LLVM_LINK_COMPONENTS
add_clang_unittest(ClangDriverTests
DistroTest.cpp
DXCModeTest.cpp
+ GCCVersionTest.cpp
ToolChainTest.cpp
ModuleCacheTest.cpp
MultilibBuilderTest.cpp
diff --git a/clang/unittests/Driver/GCCVersionTest.cpp
b/clang/unittests/Driver/GCCVersionTest.cpp
new file mode 100644
index 000..88c26dfe814e3ff
--- /dev/null
+++ b/clang/unittests/Driver/GCCVersionTest.cpp
@@ -0,0 +1,59 @@
+//===- unittests/Driver/GCCVersionTest.cpp --- GCCVersion parser tests
===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// Unit tests for Generic_GCC::GCCVersion
+//
+//===--===//
+
+#include "../../lib/Driver/ToolChains/Gnu.h"
+#include "gtest/gtest.h"
+
+// The Generic_GCC class is hidden in dylib/shared library builds, so
+// this test can only be built if neither of those configurations are
+// enabled.
+#if !defined(LLVM_BUILD_LLVM_DYLIB) && !defined(LLVM_BUILD_SHARED_LIBS)
+
+using namespace clang;
+using namespace clang::driver;
+
+namespace {
+
+struct VersionParseTest {
+ std::string Text;
+
+ int Major, Minor, Patch;
+ std::string MajorStr, MinorStr, PatchSuffix;
+};
+
+const VersionParseTest TestCases[] = {
+{"5", 5, -1, -1, "5", "", ""},
+{"4.4", 4, 4, -1, "4", "4", ""},
+{"4.4-patched", 4, 4, -1, "4", "4", "-patched"},
+{"4.4.0", 4, 4, 0, "4", "4", ""},
+{"4.4.x", 4, 4, -1, "4", "4", ""},
+{"4.4.2-rc4", 4, 4, 2, "4", "4", "-rc4"},
+{"4.4.x-patched", 4, 4, -1, "4", "4", ""},
+{"not-a-version", -1, -1, -1, "", "", ""},
+};
+
+TEST(GCCVersionTest, Parse) {
+ for (const auto &TC : TestCases) {
+auto V = toolchains::Generic_GCC::GCCVersion::Parse(TC.Text);
+EXPECT_EQ(V.Text, TC.Text);
+EXPECT_EQ(V.Major, TC.Major);
+EXPECT_EQ(V.Minor, TC.Minor);
+EXPECT_EQ(V.Patch, TC.Patch);
+EXPECT_EQ(V.MajorStr, TC.MajorStr);
+EXPECT_EQ(V.MinorStr, TC.MinorStr);
+EXPECT_EQ(V.PatchSuffix, TC.PatchSuffix);
+ }
+}
+
+} // end anonymous namespace
+
+#endif
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] [unittest] Add a test for Generic_GCC::GCCVersion::Parse (PR #69078)
mstorsjo wrote: > I hope that we do not drop `LLVM_LIBRARY_VISIBILITY` arbitrarily from > `clang::driver::toolchains::*` classes, just because some unittests need to > reference the symbols in a shared object. That’s a reasonable point. > ```c > #if !defined(LLVM_BUILD_SHARED_LIBS) > ``` > > is not great but is not too bad. `-DBUILD_SHARED_LIBS=on` modes are slow to > execute tests and are not used often for Release builds. I guess that’s a reasonable tradeoff. We’d need to do the same for dylib mode as well, which probably is used a bit more. But the main point is that whoever is working on modifying that implementation can run the tests fairly easy, and that they get run somewhere in some configurations at least. https://github.com/llvm/llvm-project/pull/69078 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] [unittest] Add a test for Generic_GCC::GCCVersion::Parse (PR #69078)
mstorsjo wrote: > Perhaps this belongs in the ABI-breaking-checks build? Hmm, ideally I think it should be included in any build - let’s hope we don’t need to resort to that. @tstellar @MaskRay Do either of you happen to know about this; would it be ok ABI wise to remove `LLVM_LIBRARY_VISIBILITY` from `clang::driver::toolchains::Generic_GCC` in order to be able to access it from unit tests in shared builds? https://github.com/llvm/llvm-project/pull/69078 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] 1072b94 - Revert "[clang] [unittest] Add a test for Generic_GCC::GCCVersion::Parse (#69078)"
Author: Martin Storsjö
Date: 2023-10-18T15:42:18+03:00
New Revision: 1072b94ed8e5a051100557185cb384364850635a
URL:
https://github.com/llvm/llvm-project/commit/1072b94ed8e5a051100557185cb384364850635a
DIFF:
https://github.com/llvm/llvm-project/commit/1072b94ed8e5a051100557185cb384364850635a.diff
LOG: Revert "[clang] [unittest] Add a test for Generic_GCC::GCCVersion::Parse
(#69078)"
This reverts commit b4b35a5d2b4ee26bf79b8a92715dd200f3f9cc49.
That commit broke builds with -DBUILD_SHARED_LIBS=ON. The reason
is that clang::driver::toolchains::Generic_GCC::GCCVersion::Parse
isn't visible outside of the shared library, because
the Generic_GCC class is marked with LLVM_LIBRARY_VISIBILITY.
Added:
Modified:
clang/unittests/Driver/CMakeLists.txt
Removed:
clang/unittests/Driver/GCCVersionTest.cpp
diff --git a/clang/unittests/Driver/CMakeLists.txt
b/clang/unittests/Driver/CMakeLists.txt
index 752037f78fb147d..e37c158d7137a88 100644
--- a/clang/unittests/Driver/CMakeLists.txt
+++ b/clang/unittests/Driver/CMakeLists.txt
@@ -9,7 +9,6 @@ set(LLVM_LINK_COMPONENTS
add_clang_unittest(ClangDriverTests
DistroTest.cpp
DXCModeTest.cpp
- GCCVersionTest.cpp
ToolChainTest.cpp
ModuleCacheTest.cpp
MultilibBuilderTest.cpp
diff --git a/clang/unittests/Driver/GCCVersionTest.cpp
b/clang/unittests/Driver/GCCVersionTest.cpp
deleted file mode 100644
index 9ae335bca77dc12..000
--- a/clang/unittests/Driver/GCCVersionTest.cpp
+++ /dev/null
@@ -1,52 +0,0 @@
-//===- unittests/Driver/GCCVersionTest.cpp --- GCCVersion parser tests
===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===--===//
-//
-// Unit tests for Generic_GCC::GCCVersion
-//
-//===--===//
-
-#include "../../lib/Driver/ToolChains/Gnu.h"
-#include "gtest/gtest.h"
-
-using namespace clang;
-using namespace clang::driver;
-
-namespace {
-
-struct VersionParseTest {
- std::string Text;
-
- int Major, Minor, Patch;
- std::string MajorStr, MinorStr, PatchSuffix;
-};
-
-const VersionParseTest TestCases[] = {
-{"5", 5, -1, -1, "5", "", ""},
-{"4.4", 4, 4, -1, "4", "4", ""},
-{"4.4-patched", 4, 4, -1, "4", "4", "-patched"},
-{"4.4.0", 4, 4, 0, "4", "4", ""},
-{"4.4.x", 4, 4, -1, "4", "4", ""},
-{"4.4.2-rc4", 4, 4, 2, "4", "4", "-rc4"},
-{"4.4.x-patched", 4, 4, -1, "4", "4", ""},
-{"not-a-version", -1, -1, -1, "", "", ""},
-};
-
-TEST(GCCVersionTest, Parse) {
- for (const auto &TC : TestCases) {
-auto V = toolchains::Generic_GCC::GCCVersion::Parse(TC.Text);
-EXPECT_EQ(V.Text, TC.Text);
-EXPECT_EQ(V.Major, TC.Major);
-EXPECT_EQ(V.Minor, TC.Minor);
-EXPECT_EQ(V.Patch, TC.Patch);
-EXPECT_EQ(V.MajorStr, TC.MajorStr);
-EXPECT_EQ(V.MinorStr, TC.MinorStr);
-EXPECT_EQ(V.PatchSuffix, TC.PatchSuffix);
- }
-}
-
-} // end anonymous namespace
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] [unittest] Add a test for Generic_GCC::GCCVersion::Parse (PR #69078)
mstorsjo wrote: > @tbaederr Just came to report the same thing! > > @mstorsjo This broke builds that use `-DBUILD_SHARED_LIBS=True`. Thanks! That was my guess as well, I was running a build with that enabled to try to reproduce @tbaederr 's issue. > The problem seems to be that the `Generic_GCC` class has the > `LLVM_LIBRARY_VISIBILITY` attribute meaning the > `clang::driver::toolchains::Generic_GCC::GCCVersion::Parse(llvm::StringRef)` > symbol is hidden. Removing that attribute fixes the build. It would be good > for someone more familiar with this part of the codebase to confirm if that's > an acceptable fix however. Thanks for the analysis! I guess that sounds reasonable, although I wonder how these unit test, that test internals within libclang are meant to work in this configuration. The number of extra exported symbols by removing `LLVM_LIBRARY_VISIBILITY` probably isn't that bad, I wonder if it has other implications wrt ABI of the shared libs? I guess it's safest to revert this for now, so we can figure out the best path forward here without a rush? https://github.com/llvm/llvm-project/pull/69078 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] [Gnu] Improve GCCVersion parsing to match versions such as "10-win32" (PR #69079)
mstorsjo wrote: The prerequisite to this PR has been merged now. https://github.com/llvm/llvm-project/pull/69079 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] [Gnu] Improve GCCVersion parsing to match versions such as "10-win32" (PR #69079)
https://github.com/mstorsjo updated
https://github.com/llvm/llvm-project/pull/69079
From 468befbb3eeaa0a23b001141976108157608e11d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Martin=20Storsj=C3=B6?=
Date: Sat, 14 Oct 2023 00:55:18 +0300
Subject: [PATCH] [clang] [Gnu] Improve GCCVersion parsing to match versions
such as "10-win32"
In earlier GCC versions, the Debian/Ubuntu provided mingw toolchains
were packaged in /usr/lib/gcc/ with version strings such
as "5.3-win32", which were matched and found since
6afcd64eb65fca233a7b173f88cffb2c2c9c114c. However in recent versions,
they have stopped including the minor version number and only
have version strings such as "10-win32" and "10-posix".
Generalize the parsing code to tolerate the patch suffix to be
present on a version number with only a major number.
Refactor the string parsing code to highlight the overall structure
of the parsing. This implementation should yield the same result
as before, except for when there's only one segment and it has
trailing, non-number contents.
This allows Clang to find the GCC libraries and headers in
Debian/Ubuntu provided MinGW cross compilers.
---
clang/lib/Driver/ToolChains/Gnu.cpp | 82 +++
clang/unittests/Driver/GCCVersionTest.cpp | 1 +
2 files changed, 55 insertions(+), 28 deletions(-)
diff --git a/clang/lib/Driver/ToolChains/Gnu.cpp
b/clang/lib/Driver/ToolChains/Gnu.cpp
index cdd911af9a73361..e6f94836c4110a1 100644
--- a/clang/lib/Driver/ToolChains/Gnu.cpp
+++ b/clang/lib/Driver/ToolChains/Gnu.cpp
@@ -2007,45 +2007,71 @@ Generic_GCC::GCCVersion
Generic_GCC::GCCVersion::Parse(StringRef VersionText) {
std::pair First = VersionText.split('.');
std::pair Second = First.second.split('.');
- GCCVersion GoodVersion = {VersionText.str(), -1, -1, -1, "", "", ""};
- if (First.first.getAsInteger(10, GoodVersion.Major) || GoodVersion.Major < 0)
-return BadVersion;
- GoodVersion.MajorStr = First.first.str();
- if (First.second.empty())
-return GoodVersion;
+ StringRef MajorStr = First.first;
StringRef MinorStr = Second.first;
- if (Second.second.empty()) {
-if (size_t EndNumber = MinorStr.find_first_not_of("0123456789")) {
- GoodVersion.PatchSuffix = std::string(MinorStr.substr(EndNumber));
- MinorStr = MinorStr.slice(0, EndNumber);
-}
- }
- if (MinorStr.getAsInteger(10, GoodVersion.Minor) || GoodVersion.Minor < 0)
-return BadVersion;
- GoodVersion.MinorStr = MinorStr.str();
+ StringRef PatchStr = Second.second;
- // First look for a number prefix and parse that if present. Otherwise just
- // stash the entire patch string in the suffix, and leave the number
- // unspecified. This covers versions strings such as:
- // 5(handled above)
+ GCCVersion GoodVersion = {VersionText.str(), -1, -1, -1, "", "", ""};
+
+ // Parse version number strings such as:
+ // 5
// 4.4
// 4.4-patched
// 4.4.0
// 4.4.x
// 4.4.2-rc4
// 4.4.x-patched
- // And retains any patch number it finds.
- StringRef PatchText = Second.second;
- if (!PatchText.empty()) {
-if (size_t EndNumber = PatchText.find_first_not_of("0123456789")) {
- // Try to parse the number and any suffix.
- if (PatchText.slice(0, EndNumber).getAsInteger(10, GoodVersion.Patch) ||
- GoodVersion.Patch < 0)
-return BadVersion;
- GoodVersion.PatchSuffix = std::string(PatchText.substr(EndNumber));
+ // 10-win32
+ // Split on '.', handle 1, 2 or 3 such segments. Each segment must contain
+ // purely a number, except for the last one, where a non-number suffix
+ // is stored in PatchSuffix. The third segment is allowed to not contain
+ // a number at all.
+
+ auto HandleLastNumber = [&](StringRef Segment, int &Number,
+ std::string &OutStr) -> bool {
+// Look for a number prefix and parse that, and split out any trailing
+// string into GoodVersion.PatchSuffix.
+
+if (size_t EndNumber = Segment.find_first_not_of("0123456789")) {
+ StringRef NumberStr = Segment.slice(0, EndNumber);
+ if (NumberStr.getAsInteger(10, Number) || Number < 0)
+return false;
+ OutStr = NumberStr;
+ GoodVersion.PatchSuffix = Segment.substr(EndNumber);
+ return true;
}
+return false;
+ };
+ auto HandleNumber = [](StringRef Segment, int &Number) -> bool {
+if (Segment.getAsInteger(10, Number) || Number < 0)
+ return false;
+return true;
+ };
+
+ if (MinorStr.empty()) {
+// If no minor string, major is the last segment
+if (!HandleLastNumber(MajorStr, GoodVersion.Major, GoodVersion.MajorStr))
+ return BadVersion;
+return GoodVersion;
+ } else {
+if (!HandleNumber(MajorStr, GoodVersion.Major))
+ return BadVersion;
+GoodVersion.MajorStr = MajorStr;
+ }
+ if (PatchStr.empty()) {
+// If no patch string, minor is the last segment
+if (!HandleLastNumber(MinorStr, GoodVersion.Minor, GoodVersion.MinorStr))
+
[clang] [clang] [unittest] Add a test for Generic_GCC::GCCVersion::Parse (PR #69078)
https://github.com/mstorsjo closed https://github.com/llvm/llvm-project/pull/69078 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] [unittest] Add a test for Generic_GCC::GCCVersion::Parse (PR #69078)
mstorsjo wrote: Thanks, I applied the suggested changes - will push in a little while. https://github.com/llvm/llvm-project/pull/69078 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] [unittest] Add a test for Generic_GCC::GCCVersion::Parse (PR #69078)
https://github.com/mstorsjo updated
https://github.com/llvm/llvm-project/pull/69078
From 1aac071988f66ccab67c7a6179841bb272b5684a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Martin=20Storsj=C3=B6?=
Date: Sat, 14 Oct 2023 00:06:05 +0300
Subject: [PATCH] [clang] [unittest] Add a test for
Generic_GCC::GCCVersion::Parse
This adds actual test cases for all the cases that are listed in
a code comment in the implementation of this function; having such
test coverage eases doing further modifications to the function.
---
clang/unittests/Driver/CMakeLists.txt | 1 +
clang/unittests/Driver/GCCVersionTest.cpp | 52 +++
2 files changed, 53 insertions(+)
create mode 100644 clang/unittests/Driver/GCCVersionTest.cpp
diff --git a/clang/unittests/Driver/CMakeLists.txt
b/clang/unittests/Driver/CMakeLists.txt
index e37c158d7137a88..752037f78fb147d 100644
--- a/clang/unittests/Driver/CMakeLists.txt
+++ b/clang/unittests/Driver/CMakeLists.txt
@@ -9,6 +9,7 @@ set(LLVM_LINK_COMPONENTS
add_clang_unittest(ClangDriverTests
DistroTest.cpp
DXCModeTest.cpp
+ GCCVersionTest.cpp
ToolChainTest.cpp
ModuleCacheTest.cpp
MultilibBuilderTest.cpp
diff --git a/clang/unittests/Driver/GCCVersionTest.cpp
b/clang/unittests/Driver/GCCVersionTest.cpp
new file mode 100644
index 000..9ae335bca77dc12
--- /dev/null
+++ b/clang/unittests/Driver/GCCVersionTest.cpp
@@ -0,0 +1,52 @@
+//===- unittests/Driver/GCCVersionTest.cpp --- GCCVersion parser tests
===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// Unit tests for Generic_GCC::GCCVersion
+//
+//===--===//
+
+#include "../../lib/Driver/ToolChains/Gnu.h"
+#include "gtest/gtest.h"
+
+using namespace clang;
+using namespace clang::driver;
+
+namespace {
+
+struct VersionParseTest {
+ std::string Text;
+
+ int Major, Minor, Patch;
+ std::string MajorStr, MinorStr, PatchSuffix;
+};
+
+const VersionParseTest TestCases[] = {
+{"5", 5, -1, -1, "5", "", ""},
+{"4.4", 4, 4, -1, "4", "4", ""},
+{"4.4-patched", 4, 4, -1, "4", "4", "-patched"},
+{"4.4.0", 4, 4, 0, "4", "4", ""},
+{"4.4.x", 4, 4, -1, "4", "4", ""},
+{"4.4.2-rc4", 4, 4, 2, "4", "4", "-rc4"},
+{"4.4.x-patched", 4, 4, -1, "4", "4", ""},
+{"not-a-version", -1, -1, -1, "", "", ""},
+};
+
+TEST(GCCVersionTest, Parse) {
+ for (const auto &TC : TestCases) {
+auto V = toolchains::Generic_GCC::GCCVersion::Parse(TC.Text);
+EXPECT_EQ(V.Text, TC.Text);
+EXPECT_EQ(V.Major, TC.Major);
+EXPECT_EQ(V.Minor, TC.Minor);
+EXPECT_EQ(V.Patch, TC.Patch);
+EXPECT_EQ(V.MajorStr, TC.MajorStr);
+EXPECT_EQ(V.MinorStr, TC.MinorStr);
+EXPECT_EQ(V.PatchSuffix, TC.PatchSuffix);
+ }
+}
+
+} // end anonymous namespace
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] [Gnu] Improve GCCVersion parsing to match versions such as "10-win32" (PR #69079)
https://github.com/mstorsjo updated
https://github.com/llvm/llvm-project/pull/69079
From 2b127200dc7b7b7c60e3001c7acf49a33a22e2a5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Martin=20Storsj=C3=B6?=
Date: Sat, 14 Oct 2023 00:06:05 +0300
Subject: [PATCH 1/2] [clang] [unittest] Add a test for
Generic_GCC::GCCVersion::Parse
This adds actual test cases for all the cases that are listed in
a code comment in the implementation of this function; having such
test coverage eases doing further modifications to the function.
---
clang/unittests/Driver/CMakeLists.txt | 1 +
clang/unittests/Driver/GCCVersionTest.cpp | 48 +++
2 files changed, 49 insertions(+)
create mode 100644 clang/unittests/Driver/GCCVersionTest.cpp
diff --git a/clang/unittests/Driver/CMakeLists.txt
b/clang/unittests/Driver/CMakeLists.txt
index e37c158d7137a88..752037f78fb147d 100644
--- a/clang/unittests/Driver/CMakeLists.txt
+++ b/clang/unittests/Driver/CMakeLists.txt
@@ -9,6 +9,7 @@ set(LLVM_LINK_COMPONENTS
add_clang_unittest(ClangDriverTests
DistroTest.cpp
DXCModeTest.cpp
+ GCCVersionTest.cpp
ToolChainTest.cpp
ModuleCacheTest.cpp
MultilibBuilderTest.cpp
diff --git a/clang/unittests/Driver/GCCVersionTest.cpp
b/clang/unittests/Driver/GCCVersionTest.cpp
new file mode 100644
index 000..ef05a0b4fe734e5
--- /dev/null
+++ b/clang/unittests/Driver/GCCVersionTest.cpp
@@ -0,0 +1,48 @@
+//===- unittests/Driver/GCCVersionTest.cpp --- GCCVersion parser tests
===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// Unit tests for Generic_GCC::GCCVersion
+//
+//===--===//
+
+#include "../../lib/Driver/ToolChains/Gnu.h"
+#include "gtest/gtest.h"
+
+using namespace clang::driver;
+using namespace clang;
+
+struct VersionParseTest {
+ std::string Text;
+
+ int Major, Minor, Patch;
+ std::string MajorStr, MinorStr, PatchSuffix;
+};
+
+const VersionParseTest TestCases[] = {
+{"5", 5, -1, -1, "5", "", ""},
+{"4.4", 4, 4, -1, "4", "4", ""},
+{"4.4-patched", 4, 4, -1, "4", "4", "-patched"},
+{"4.4.0", 4, 4, 0, "4", "4", ""},
+{"4.4.x", 4, 4, -1, "4", "4", ""},
+{"4.4.2-rc4", 4, 4, 2, "4", "4", "-rc4"},
+{"4.4.x-patched", 4, 4, -1, "4", "4", ""},
+{"not-a-version", -1, -1, -1, "", "", ""},
+};
+
+TEST(GCCVersionTest, Parse) {
+ for (const auto &TC : TestCases) {
+auto V = toolchains::Generic_GCC::GCCVersion::Parse(TC.Text);
+ASSERT_EQ(V.Text, TC.Text);
+ASSERT_EQ(V.Major, TC.Major);
+ASSERT_EQ(V.Minor, TC.Minor);
+ASSERT_EQ(V.Patch, TC.Patch);
+ASSERT_EQ(V.MajorStr, TC.MajorStr);
+ASSERT_EQ(V.MinorStr, TC.MinorStr);
+ASSERT_EQ(V.PatchSuffix, TC.PatchSuffix);
+ }
+}
From 322d9e0626b3185533e1f7f83ab0ec2b64453ca8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Martin=20Storsj=C3=B6?=
Date: Sat, 14 Oct 2023 00:55:18 +0300
Subject: [PATCH 2/2] [clang] [Gnu] Improve GCCVersion parsing to match
versions such as "10-win32"
In earlier GCC versions, the Debian/Ubuntu provided mingw toolchains
were packaged in /usr/lib/gcc/ with version strings such
as "5.3-win32", which were matched and found since
6afcd64eb65fca233a7b173f88cffb2c2c9c114c. However in recent versions,
they have stopped including the minor version number and only
have version strings such as "10-win32" and "10-posix".
Generalize the parsing code to tolerate the patch suffix to be
present on a version number with only a major number.
Refactor the string parsing code to highlight the overall structure
of the parsing. This implementation should yield the same result
as before, except for when there's only one segment and it has
trailing, non-number contents.
This allows Clang to find the GCC libraries and headers in
Debian/Ubuntu provided MinGW cross compilers.
---
clang/lib/Driver/ToolChains/Gnu.cpp | 82 +++
clang/unittests/Driver/GCCVersionTest.cpp | 1 +
2 files changed, 55 insertions(+), 28 deletions(-)
diff --git a/clang/lib/Driver/ToolChains/Gnu.cpp
b/clang/lib/Driver/ToolChains/Gnu.cpp
index cdd911af9a73361..e6f94836c4110a1 100644
--- a/clang/lib/Driver/ToolChains/Gnu.cpp
+++ b/clang/lib/Driver/ToolChains/Gnu.cpp
@@ -2007,45 +2007,71 @@ Generic_GCC::GCCVersion
Generic_GCC::GCCVersion::Parse(StringRef VersionText) {
std::pair First = VersionText.split('.');
std::pair Second = First.second.split('.');
- GCCVersion GoodVersion = {VersionText.str(), -1, -1, -1, "", "", ""};
- if (First.first.getAsInteger(10, GoodVersion.Major) || GoodVersion.Major < 0)
-return BadVersion;
- GoodVersion.MajorStr = First.first.str();
- if (First.second.empty())
-return GoodVersion;
+ StringRef MajorStr = First.first;
StringRef Mino
[clang] [clang] [Gnu] Improve GCCVersion parsing to match versions such as "10-win32" (PR #69079)
mstorsjo wrote: This goes on top of #69078 - the first commit is reviewed there, thus within this PR, only review the second commit on its own. https://github.com/llvm/llvm-project/pull/69079 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] [Gnu] Improve GCCVersion parsing to match versions such as "10-win32" (PR #69079)
https://github.com/mstorsjo created
https://github.com/llvm/llvm-project/pull/69079
In earlier GCC versions, the Debian/Ubuntu provided mingw toolchains were
packaged in /usr/lib/gcc/ with version strings such as "5.3-win32",
which were matched and found since 6afcd64eb65fca233a7b173f88cffb2c2c9c114c.
However in recent versions, they have stopped including the minor version
number and only have version strings such as "10-win32" and "10-posix".
Generalize the parsing code to tolerate the patch suffix to be present on a
version number with only a major number.
Refactor the string parsing code to highlight the overall structure of the
parsing. This implementation should yield the same result as before, except for
when there's only one segment and it has trailing, non-number contents.
This allows Clang to find the GCC libraries and headers in Debian/Ubuntu
provided MinGW cross compilers.
From 2b127200dc7b7b7c60e3001c7acf49a33a22e2a5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Martin=20Storsj=C3=B6?=
Date: Sat, 14 Oct 2023 00:06:05 +0300
Subject: [PATCH 1/2] [clang] [unittest] Add a test for
Generic_GCC::GCCVersion::Parse
This adds actual test cases for all the cases that are listed in
a code comment in the implementation of this function; having such
test coverage eases doing further modifications to the function.
---
clang/unittests/Driver/CMakeLists.txt | 1 +
clang/unittests/Driver/GCCVersionTest.cpp | 48 +++
2 files changed, 49 insertions(+)
create mode 100644 clang/unittests/Driver/GCCVersionTest.cpp
diff --git a/clang/unittests/Driver/CMakeLists.txt
b/clang/unittests/Driver/CMakeLists.txt
index e37c158d7137a88..752037f78fb147d 100644
--- a/clang/unittests/Driver/CMakeLists.txt
+++ b/clang/unittests/Driver/CMakeLists.txt
@@ -9,6 +9,7 @@ set(LLVM_LINK_COMPONENTS
add_clang_unittest(ClangDriverTests
DistroTest.cpp
DXCModeTest.cpp
+ GCCVersionTest.cpp
ToolChainTest.cpp
ModuleCacheTest.cpp
MultilibBuilderTest.cpp
diff --git a/clang/unittests/Driver/GCCVersionTest.cpp
b/clang/unittests/Driver/GCCVersionTest.cpp
new file mode 100644
index 000..ef05a0b4fe734e5
--- /dev/null
+++ b/clang/unittests/Driver/GCCVersionTest.cpp
@@ -0,0 +1,48 @@
+//===- unittests/Driver/GCCVersionTest.cpp --- GCCVersion parser tests
===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// Unit tests for Generic_GCC::GCCVersion
+//
+//===--===//
+
+#include "../../lib/Driver/ToolChains/Gnu.h"
+#include "gtest/gtest.h"
+
+using namespace clang::driver;
+using namespace clang;
+
+struct VersionParseTest {
+ std::string Text;
+
+ int Major, Minor, Patch;
+ std::string MajorStr, MinorStr, PatchSuffix;
+};
+
+const VersionParseTest TestCases[] = {
+{"5", 5, -1, -1, "5", "", ""},
+{"4.4", 4, 4, -1, "4", "4", ""},
+{"4.4-patched", 4, 4, -1, "4", "4", "-patched"},
+{"4.4.0", 4, 4, 0, "4", "4", ""},
+{"4.4.x", 4, 4, -1, "4", "4", ""},
+{"4.4.2-rc4", 4, 4, 2, "4", "4", "-rc4"},
+{"4.4.x-patched", 4, 4, -1, "4", "4", ""},
+{"not-a-version", -1, -1, -1, "", "", ""},
+};
+
+TEST(GCCVersionTest, Parse) {
+ for (const auto &TC : TestCases) {
+auto V = toolchains::Generic_GCC::GCCVersion::Parse(TC.Text);
+ASSERT_EQ(V.Text, TC.Text);
+ASSERT_EQ(V.Major, TC.Major);
+ASSERT_EQ(V.Minor, TC.Minor);
+ASSERT_EQ(V.Patch, TC.Patch);
+ASSERT_EQ(V.MajorStr, TC.MajorStr);
+ASSERT_EQ(V.MinorStr, TC.MinorStr);
+ASSERT_EQ(V.PatchSuffix, TC.PatchSuffix);
+ }
+}
From 2c923927f2aaf58e0879fe88b573fd1bc80063a5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Martin=20Storsj=C3=B6?=
Date: Sat, 14 Oct 2023 00:55:18 +0300
Subject: [PATCH 2/2] [clang] [Gnu] Improve GCCVersion parsing to match
versions such as "10-win32"
In earlier GCC versions, the Debian/Ubuntu provided mingw toolchains
were packaged in /usr/lib/gcc/ with version strings such
as "5.3-win32", which were matched and found since
6afcd64eb65fca233a7b173f88cffb2c2c9c114c. However in recent versions,
they have stopped including the minor version number and only
have version strings such as "10-win32" and "10-posix".
Generalize the parsing code to tolerate the patch suffix to be
present on a version number with only a major number.
Refactor the string parsing code to highlight the overall structure
of the parsing. This implementation should yield the same result
as before, except for when there's only one segment and it has
trailing, non-number contents.
This allows Clang to find the GCC libraries and headers in
Debian/Ubuntu provided MinGW cross compilers.
---
clang/lib/Driver/ToolChains/Gnu.cpp | 82 +++
clang/unittests/Driver/G
[clang] [clang] [unittest] Add a test for Generic_GCC::GCCVersion::Parse (PR #69078)
https://github.com/mstorsjo created
https://github.com/llvm/llvm-project/pull/69078
This adds actual test cases for all the cases that are listed in a code comment
in the implementation of this function; having such test coverage eases doing
further modifications to the function.
From 2b127200dc7b7b7c60e3001c7acf49a33a22e2a5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Martin=20Storsj=C3=B6?=
Date: Sat, 14 Oct 2023 00:06:05 +0300
Subject: [PATCH] [clang] [unittest] Add a test for
Generic_GCC::GCCVersion::Parse
This adds actual test cases for all the cases that are listed in
a code comment in the implementation of this function; having such
test coverage eases doing further modifications to the function.
---
clang/unittests/Driver/CMakeLists.txt | 1 +
clang/unittests/Driver/GCCVersionTest.cpp | 48 +++
2 files changed, 49 insertions(+)
create mode 100644 clang/unittests/Driver/GCCVersionTest.cpp
diff --git a/clang/unittests/Driver/CMakeLists.txt
b/clang/unittests/Driver/CMakeLists.txt
index e37c158d7137a88..752037f78fb147d 100644
--- a/clang/unittests/Driver/CMakeLists.txt
+++ b/clang/unittests/Driver/CMakeLists.txt
@@ -9,6 +9,7 @@ set(LLVM_LINK_COMPONENTS
add_clang_unittest(ClangDriverTests
DistroTest.cpp
DXCModeTest.cpp
+ GCCVersionTest.cpp
ToolChainTest.cpp
ModuleCacheTest.cpp
MultilibBuilderTest.cpp
diff --git a/clang/unittests/Driver/GCCVersionTest.cpp
b/clang/unittests/Driver/GCCVersionTest.cpp
new file mode 100644
index 000..ef05a0b4fe734e5
--- /dev/null
+++ b/clang/unittests/Driver/GCCVersionTest.cpp
@@ -0,0 +1,48 @@
+//===- unittests/Driver/GCCVersionTest.cpp --- GCCVersion parser tests
===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// Unit tests for Generic_GCC::GCCVersion
+//
+//===--===//
+
+#include "../../lib/Driver/ToolChains/Gnu.h"
+#include "gtest/gtest.h"
+
+using namespace clang::driver;
+using namespace clang;
+
+struct VersionParseTest {
+ std::string Text;
+
+ int Major, Minor, Patch;
+ std::string MajorStr, MinorStr, PatchSuffix;
+};
+
+const VersionParseTest TestCases[] = {
+{"5", 5, -1, -1, "5", "", ""},
+{"4.4", 4, 4, -1, "4", "4", ""},
+{"4.4-patched", 4, 4, -1, "4", "4", "-patched"},
+{"4.4.0", 4, 4, 0, "4", "4", ""},
+{"4.4.x", 4, 4, -1, "4", "4", ""},
+{"4.4.2-rc4", 4, 4, 2, "4", "4", "-rc4"},
+{"4.4.x-patched", 4, 4, -1, "4", "4", ""},
+{"not-a-version", -1, -1, -1, "", "", ""},
+};
+
+TEST(GCCVersionTest, Parse) {
+ for (const auto &TC : TestCases) {
+auto V = toolchains::Generic_GCC::GCCVersion::Parse(TC.Text);
+ASSERT_EQ(V.Text, TC.Text);
+ASSERT_EQ(V.Major, TC.Major);
+ASSERT_EQ(V.Minor, TC.Minor);
+ASSERT_EQ(V.Patch, TC.Patch);
+ASSERT_EQ(V.MajorStr, TC.MajorStr);
+ASSERT_EQ(V.MinorStr, TC.MinorStr);
+ASSERT_EQ(V.PatchSuffix, TC.PatchSuffix);
+ }
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[libunwind] [libc++] Use -nostdlib++ on GCC unconditionally (PR #68832)
@@ -642,18 +642,8 @@ get_sanitizer_flags(SANITIZER_FLAGS
"${LLVM_USE_SANITIZER}")
# Link system libraries ===
function(cxx_link_system_libraries target)
-
-# In order to remove just libc++ from the link step
-# we need to use -nostdlib++ whenever it is supported.
-# Unfortunately this cannot be used universally because for example g++
supports
-# only -nodefaultlibs in which case all libraries will be removed and
-# all libraries but c++ have to be added in manually.
- if (CXX_SUPPORTS_NOSTDLIBXX_FLAG)
-target_add_link_flags_if_supported(${target} PRIVATE "-nostdlib++")
- else()
-target_add_link_flags_if_supported(${target} PRIVATE "-nodefaultlibs")
-target_add_compile_flags_if_supported(${target} PRIVATE "/Zl")
-target_add_link_flags_if_supported(${target} PRIVATE "/nodefaultlib")
mstorsjo wrote:
Yeah I think it's totally ok to remove. Whatever the setup was when those might
have been needed on MSVC-like setups, they're not needed (or usable) currently
- and our CI coverage is fairly good at the moment, I would say. So if this
patch works, just go ahead!
https://github.com/llvm/llvm-project/pull/68832
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [libc++] Use -nostdlib++ on GCC unconditionally (PR #68832)
@@ -642,18 +642,8 @@ get_sanitizer_flags(SANITIZER_FLAGS
"${LLVM_USE_SANITIZER}")
# Link system libraries ===
function(cxx_link_system_libraries target)
-
-# In order to remove just libc++ from the link step
-# we need to use -nostdlib++ whenever it is supported.
-# Unfortunately this cannot be used universally because for example g++
supports
-# only -nodefaultlibs in which case all libraries will be removed and
-# all libraries but c++ have to be added in manually.
- if (CXX_SUPPORTS_NOSTDLIBXX_FLAG)
-target_add_link_flags_if_supported(${target} PRIVATE "-nostdlib++")
- else()
-target_add_link_flags_if_supported(${target} PRIVATE "-nodefaultlibs")
-target_add_compile_flags_if_supported(${target} PRIVATE "/Zl")
-target_add_link_flags_if_supported(${target} PRIVATE "/nodefaultlib")
mstorsjo wrote:
Yeah I think it's totally ok to remove. Whatever the setup was when those might
have been needed on MSVC-like setups, they're not needed (or usable) currently
- and our CI coverage is fairly good at the moment, I would say. So if this
patch works, just go ahead!
https://github.com/llvm/llvm-project/pull/68832
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [libc++] Use -nostdlib++ on GCC unconditionally (PR #68832)
@@ -642,18 +642,8 @@ get_sanitizer_flags(SANITIZER_FLAGS
"${LLVM_USE_SANITIZER}")
# Link system libraries ===
function(cxx_link_system_libraries target)
-
-# In order to remove just libc++ from the link step
-# we need to use -nostdlib++ whenever it is supported.
-# Unfortunately this cannot be used universally because for example g++
supports
-# only -nodefaultlibs in which case all libraries will be removed and
-# all libraries but c++ have to be added in manually.
- if (CXX_SUPPORTS_NOSTDLIBXX_FLAG)
-target_add_link_flags_if_supported(${target} PRIVATE "-nostdlib++")
- else()
-target_add_link_flags_if_supported(${target} PRIVATE "-nodefaultlibs")
-target_add_compile_flags_if_supported(${target} PRIVATE "/Zl")
-target_add_link_flags_if_supported(${target} PRIVATE "/nodefaultlib")
mstorsjo wrote:
Interesting; FWIW, it seems like the test for `/Zl` fails because it skips
including the CRT glue code that takes the startup from the Windows specific
`mainCRTStartup()` to the regular `main()`. I wonder when/if this test passed
at some point.
As for how this works without those options: In MSVC build configs, the
standard libraries usually aren't injected by the compiler when doing the link
- on the contrary, in most cases, one doesn't actually do linking by invoking
the compiler, but the build usually calls `link.exe` or `lld-link` directly.
The info about what libraries should be linked in is conveyed via directives
embedded in the object files. So as long as we're not using any MS STL headers
that inject such directives, we don't automatically try to link against it.
Then secondly - in MSVC configurations, libc++ always builds on top of the MSVC
C++ runtime for the base ABI stuff, so we actually do need it linked - and we
pass the necessary libraries for that manually when we link our library.
https://github.com/llvm/llvm-project/pull/68832
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[libunwind] [libc++] Use -nostdlib++ on GCC unconditionally (PR #68832)
@@ -642,18 +642,8 @@ get_sanitizer_flags(SANITIZER_FLAGS
"${LLVM_USE_SANITIZER}")
# Link system libraries ===
function(cxx_link_system_libraries target)
-
-# In order to remove just libc++ from the link step
-# we need to use -nostdlib++ whenever it is supported.
-# Unfortunately this cannot be used universally because for example g++
supports
-# only -nodefaultlibs in which case all libraries will be removed and
-# all libraries but c++ have to be added in manually.
- if (CXX_SUPPORTS_NOSTDLIBXX_FLAG)
-target_add_link_flags_if_supported(${target} PRIVATE "-nostdlib++")
- else()
-target_add_link_flags_if_supported(${target} PRIVATE "-nodefaultlibs")
-target_add_compile_flags_if_supported(${target} PRIVATE "/Zl")
-target_add_link_flags_if_supported(${target} PRIVATE "/nodefaultlib")
mstorsjo wrote:
Interesting; FWIW, it seems like the test for `/Zl` fails because it skips
including the CRT glue code that takes the startup from the Windows specific
`mainCRTStartup()` to the regular `main()`. I wonder when/if this test passed
at some point.
As for how this works without those options: In MSVC build configs, the
standard libraries usually aren't injected by the compiler when doing the link
- on the contrary, in most cases, one doesn't actually do linking by invoking
the compiler, but the build usually calls `link.exe` or `lld-link` directly.
The info about what libraries should be linked in is conveyed via directives
embedded in the object files. So as long as we're not using any MS STL headers
that inject such directives, we don't automatically try to link against it.
Then secondly - in MSVC configurations, libc++ always builds on top of the MSVC
C++ runtime for the base ABI stuff, so we actually do need it linked - and we
pass the necessary libraries for that manually when we link our library.
https://github.com/llvm/llvm-project/pull/68832
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] [MinGW] Explicitly always pass the -fno-use-init-array (PR #68571)
https://github.com/mstorsjo closed https://github.com/llvm/llvm-project/pull/68571 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] [MinGW] Explicitly always pass the -fno-use-init-array (PR #68571)
mstorsjo wrote:
> Whatever we decide to do on the LLVM side, this seems fine for the clang side.
Yes, this bit should be fine in any case.
> It looks like clang uses the value of UseInitArray for some ObjC stuff, in
> addition to passing it to the backend, so we need the right value in clang in
> any case.
Indeed, having the correct picture is good, if there's logic that depends on
it. For the ObjC case, the code actually looks like this:
```cpp
if (CGM.getTriple().isOSBinFormatCOFF())
InitVar->setSection(".CRT$XCLz");
else
{
if (CGM.getCodeGenOpts().UseInitArray)
InitVar->setSection(".init_array");
else
InitVar->setSection(".ctors");
}
```
So in that sense, it doesn't really matter here. (The `.CRT$...` sections do
work on mingw too, even if `.ctors` normally is used.)
https://github.com/llvm/llvm-project/pull/68571
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] [MinGW] Explicitly always pass the -fno-use-init-array (PR #68571)
mstorsjo wrote:
> Right, so adding an `enum DefaultableBool { Default, False, True }` and
> changing the field to that value?
With such a definition, I wonder how to interpret it for MSVC targets;
`UseInitArray` would be false, as MSVC uses the `.CRT` sections, neither
`.ctors` not `.init_array`. But it shouldn’t be reversed, and false currently
means reversing it.
https://github.com/llvm/llvm-project/pull/68571
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] [MinGW] Explicitly always pass the -fno-use-init-array (PR #68571)
mstorsjo wrote:
> Doesn't UseInitArray default to false in LLVM?
The class `TargetOptions` itself initializes it to false indeed. But
`codegen::InitTargetOptionsFromCodeGenFlags` (which seems to be what e.g. `llc`
uses) explicitly sets it to `!getUseCtors()`, which only checks the state of
the `-use-ctors` option. And Clang sets it unconditionally based on the codegen
option with the same name, which defaults to true and is controlled by the
`-f[no-]use-init-array` option.
> Anyway, we already have ways to make booleans optional; it's just a matter of
> implementing a "default" state, which we already do for certain options like
> code models. I don't think there's any fundamental architectural work
> involved in doing that. (See various enums in TargetOptions.h)
Right, so adding an `enum DefaultableBool { Default, False, True }` and
changing the field to that value? That'd be an API break for all users of the
struct, but which should be allowed for us to do? All users of `TargetOptions`
would need to change from passing regular bools to passing values from this
enum - and at the same time, they get reminded that they can keep it set to
`Default` in case they don't really have a strong opinion on the matter. Is
that what you had in mind? Or is there a way to make the migration smoother for
users somehow?
It sounds like a somewhat disruptive change for downstreams; I know we're not
supposed to compromise upstream design too much around downstream concerns, but
it feels somewhat like making more of a fuss than I wanted...
https://github.com/llvm/llvm-project/pull/68571
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] [MinGW] Explicitly always pass the -fno-use-init-array (PR #68571)
mstorsjo wrote: > Hmm... > > Maybe the backend should have some notion of a default ctors section. So if > the frontend doesn't explicitly specify anything, the backend tries to pick > the right default. Yep, this is kinda the same issue for lots of target specific options that are set as bools..., while the ideal I guess either would be an `optional`, or to initialize the bools to the target specific actual value. In this case, I would maybe suggest merging both this and #68570 - that should both fix behaviours and make clang aware of the reality, and leave the root design issue for someone else to take on... WDYT? https://github.com/llvm/llvm-project/pull/68571 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] [MinGW] Explicitly always pass the -fno-use-init-array (PR #68571)
mstorsjo wrote: > We could go with a clang fix, but also make the backend report_fatal_error if > you try to use llvm.ctors with UseInitArray on mingw. That keeps everything > consistent while also making sure non-clang frontends don't miscompile. Hmm, interesting proposition... But wouldn't that essentially break every single invocation of `llc` with a mingw triple, unless they also explicitly pass the `-use-ctors` option? And for any other language frontend, I also feel that it would, somewhat abruptly, break every single current user of LLVM for mingw targets, unless they update their calling code to explicitly set `UseInitArray` to false for this target? https://github.com/llvm/llvm-project/pull/68571 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] [MinGW] Explicitly always pass the -fno-use-init-array (PR #68571)
mstorsjo wrote: This is an alternative to #68570. This has the upside that the `UseInitArray` flag is set to the correct value throughout the chain, for any other potential users of the field, in case it would affect code generation in other places. The downside is that if we only go with this, then any other language frontend, which might not know about the details about `.ctors`, will end up with the same bug unless they also add a similar fix. (We could of course go with both fixes as well.) https://github.com/llvm/llvm-project/pull/68571 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] [MinGW] Explicitly always pass the -fno-use-init-array (PR #68571)
https://github.com/mstorsjo created
https://github.com/llvm/llvm-project/pull/68571
On MinGW targets, the .ctors section is always used for constructors.
Make sure that all layers of code generation is aware of this, wherever it
matters, by passing the -fno-use-init-array option, setting the TargetOptions
field UseInitArray to false.
This fixes https://github.com/llvm/llvm-project/issues/55938.
From 89521ab0bfd12d7f45cacd3fa1d6c3f27ff54ab6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Martin=20Storsj=C3=B6?=
Date: Mon, 9 Oct 2023 13:13:28 +0300
Subject: [PATCH] [clang] [MinGW] Explicitly always pass the
-fno-use-init-array
On MinGW targets, the .ctors section is always used for constructors.
Make sure that all layers of code generation is aware of this,
wherever it matters, by passing the -fno-use-init-array option,
setting the TargetOptions field UseInitArray to false.
This fixes https://github.com/llvm/llvm-project/issues/55938.
---
clang/lib/Driver/ToolChains/MinGW.cpp | 2 ++
clang/test/Driver/mingw.cpp | 3 +++
2 files changed, 5 insertions(+)
diff --git a/clang/lib/Driver/ToolChains/MinGW.cpp
b/clang/lib/Driver/ToolChains/MinGW.cpp
index 5872e13bda358f8..d3d829a8ddbdb95 100644
--- a/clang/lib/Driver/ToolChains/MinGW.cpp
+++ b/clang/lib/Driver/ToolChains/MinGW.cpp
@@ -709,6 +709,8 @@ void toolchains::MinGW::addClangTargetOptions(
}
}
+ CC1Args.push_back("-fno-use-init-array");
+
for (auto Opt : {options::OPT_mthreads, options::OPT_mwindows,
options::OPT_mconsole, options::OPT_mdll}) {
if (Arg *A = DriverArgs.getLastArgNoClaim(Opt))
diff --git a/clang/test/Driver/mingw.cpp b/clang/test/Driver/mingw.cpp
index 0f276820d0fff22..bb22a0652b486e1 100644
--- a/clang/test/Driver/mingw.cpp
+++ b/clang/test/Driver/mingw.cpp
@@ -77,3 +77,6 @@
// CHECK_NO_SUBSYS-NOT: "--subsystem"
// CHECK_SUBSYS_CONSOLE: "--subsystem" "console"
// CHECK_SUBSYS_WINDOWS: "--subsystem" "windows"
+
+// RUN: %clang -target i686-windows-gnu -### %s 2>&1 | FileCheck
-check-prefix=CHECK_NO_INIT_ARRAY %s
+// CHECK_NO_INIT_ARRAY: "-fno-use-init-array"
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][driver] Prevent clang picking the system headers when started via a symlink (PR #68091)
mstorsjo wrote: > > ... I understand this is only a bug on MacOS? > > I'm using clang only to compile my binary packages on macOS (the Linux > binaries are compiled with gcc), so I got bitten by this bug only on macOS. > The other platforms/targets may or may not be affected, I don't know; but my > initial search for InstalledDir showed multiple references not paired with > references to Dir. This is one of the issues I have with mainly with the problem formulation - as there are two levels of logic involved here. The generic code which sets the install dir, and the target specific files in clang/lib/Driver/ToolChains, which do various things based on the install dir that has been detected elsewhere, plus target specific heuristics for falling back on other ways to detecting things that are needed. If the installation dir is detected as the wrong thing, this would be fatal for all targets, but the exact behaviour you'd see would be quite target dependent. That's why explanations like "silently using system headers from a different version is hard to debug here" are out of scope here; using the wrong install dir is fatal of course. If the darwin specific fallback handling could be improved, that'd of course be great. On the other hand - in principle I kinda agree that the fix itself, locating things based on the actual executable, sounds like the right thing to do in any case. We're early in the 18.x cycle, and if we could get the tests fixed to work despite that, we'd have lots of time to see if someone is affected negatively by it, before 18.x is released. https://github.com/llvm/llvm-project/pull/68091 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][driver] Prevent clang picking the system headers/libraries when started via a symlink (PR #68091)
mstorsjo wrote: > > it's quite possible that someone has ended up depending on the previous de > > facto behaviour. > > Then we have to be super creative and find a better solution. > > How about changing the logic, and where `Driver.InstalledDir` is used, if the > desired file/folder is not found (like header or library), search again in > `Driver.Dir` (which is the parent of ClangExecutable)? > > I don't know how complicated the implementation might be, but, at first > sight, it should maintain compatibility with curent use cases that create a > full toolchain folder and also prevent silently falling back to system > headers/libraries (like in my use case). Yes, something like that would be possible. I wonder if this would need to be somewhat target specific. Different targets have different logic for fallback when things don't exist in the most expected directory. But perhaps it would be enough to check for `/../lib/clang/` or something like that, to distinguish between the two locations - that's at least quite target independent. On the other hand; I agree that it's kinda implausible that this directory would exist at all anywhere else than next to the actual executable. In that case, perhaps those testcases are the only cases that would need to be handled after all? @nolange - in your efforts in https://github.com/mstorsjo/llvm-mingw/issues/362 - do you rely on making a symlink to the system installed clang executable and locating sysroots and the clang `/lib/clang` directory next to the symlink? https://github.com/llvm/llvm-project/pull/68091 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][driver] Use TheDriver.ClangExecutable in SetInstallDir to point to the actual install folder (PR #68091)
mstorsjo wrote: > Without any other reference, I checked the Apple clang, on my macOS: > > ``` > ilg@wksi ~ % /usr/bin/clang -v > Apple clang version 14.0.0 (clang-1400.0.29.202) > Target: x86_64-apple-darwin21.6.0 > Thread model: posix > InstalledDir: /Library/Developer/CommandLineTools/usr/bin > > > ilg@wksi ~ % mkdir tmp/a > ilg@wksi ~ % ln -s /usr/bin/clang tmp/a > > ilg@wksi ~ % tmp/a/clang -v > Apple clang version 14.0.0 (clang-1400.0.29.202) > Target: x86_64-apple-darwin21.6.0 > Thread model: posix > InstalledDir: /Library/Developer/CommandLineTools/usr/bin > ``` > > So Apple decided to **follow the links**, and, in my opinion, this is the > less ambiguous and expected behaviour. No, this is a side effect of something else. Note, `/usr/bin/clang` isn't a symlink to `/Library/Developer/CommandLineTools/usr/bin/clang`, but it is a wrapper executable that executes the latter (after setting some env variables, iirc, and after locating where it is installed - it can also be in an Xcode.app bundle somewhere). Let's recreate your experiment like this: ```console % mkdir -p tmp/a % ln -s /Library/Developer/CommandLineTools/usr/bin/clang tmp/a % tmp/a/clang -v Apple clang version 12.0.5 (clang-1205.0.22.9) Target: arm64-apple-darwin21.6.0 Thread model: posix InstalledDir: /Users/martin/tmp/a ``` There you see that Apple's clang also behaves exactly the same, if you disregard the external wrapper executable. > Another way to rephrase the question: the current implementation that > preserves the symlinks, was it an explicitly required feature, or rather a > side effect that came into existence by accident when implementing the > current tests? Regardless of whether it was intentional or not, due to Hyrum's law, it's quite possible that someone has ended up depending on the previous defacto behaviour. We did a similar cleanup for llvm-rc not long ago, and llvm-rc has much much fewer users than clang. In 8c6a0c8bf50bca6c3a0c4de1b84f21466ee31655 we changed llvm-rc to explicitly look up the executable path (instead of looking for what would be found in `$PATH`, essentially finding the source end of symlinks - just like what clang does right now). But this broke Google's internal uses of the tool, so in e4eb8d97e8afcb879dc5cd0da7a937dbb26fbf12 we reverted to checking both paths, essentially testing both directories. https://github.com/llvm/llvm-project/pull/68091 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][driver] Use platform specific calls to get the executable absolute path (PR #68091)
mstorsjo wrote: > I'm convinced that such links were occasionally used before, just that the > result was not necessarily an error, I would say that in most cases using the > system libraries is functional, and this explains why such cases were not > reported till now. Not necessarily; this depends entirely on what target you're using. If you're cross compiling, if you pick the wrong install dir it probably won't find any usable sysroot at all. The case you've run into, of finding and using the system installed toolchain transparently, is only one of many different outcomes in such a case. https://github.com/llvm/llvm-project/pull/68091 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][driver] Use platform specific calls to get the executable absolute path (PR #68091)
mstorsjo wrote: > > it makes me wonder if someone actually is relying on the current behaviour > > To rephrase your question, you ask if someone is using a configuration with a > folder where various custom libraries are and bin folder with a link to the > actual clang, and expects for clang to pick the custom libraries instead of > the distributed ones? I obviously can't tell, but I would say that the > chances are slim. To me, it actually sounds entirely possible for someone to want to set up a toolchain that way. (I even had a user look into setting up something similar to that recently; building user-local installed llvm-mingw toolchains around a clang binary provided by a linux distribution.) > And, in case someone wants such a configuration, the current patch, if > `-no-canonical-prefixes` is used, reverts to the previous behaviour, when the > symlink is not followed. > > > whatever the consequences are of using a different install dir is kinda of > > irrelevant here > > Well, given how much time I spent trying to understand some weird errors > caused by this, I would not call them irrelevant :-( I'm not diminishing your debugging effort - I'm just saying that how annoying something was to debug (until you realized that the detected installation dir was wrong) doesn't really affect whether clang should change its definition for how the install directory is determined. Yes, having it use the wrong install directory will of course make things not work as they should. > > there should be a well defined logic for how that's calculated > > Right, but I would say that the current logic based on the folder where the > clang executable is located worked fine till now, and I see no reasons to > change it, With well defined logic, I mean with respect to how symlinks are handled as well. Without symlinks involved, the logic for determining the clang install directory is near-trivial. And you make it sound like nobody might have used such symlinks before https://github.com/llvm/llvm-project/pull/68091 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][driver] Use platform specific calls to get the executable absolute path (PR #68091)
mstorsjo wrote: > > I haven't checked closely > > Hi Martin, please check the #66704 bug report, the current behaviour is > plainly wrong, I would kind of agree with that in general - resolving this to the actual clang binary would indeed seem like the right thing to do. But it makes me wonder if someone actually is relying on the current behaviour, or is this a case that hasn't been encountered before (symlinks primarily being used within the same bin directory)? > clang does not pick the correct libraries and silently uses the system > libraries, which leads to very subtle and hard to debug errors if the new > version is several versions apart from the system. Well whatever the consequences are of using a different install dir is kinda of irrelevant here IMO; there should be a well defined logic for how that's calculated (and I agree that it ideally should be based on the actual executable), and based on that, each target toolchain has various fallbacks for finding the headers to use if it's not colocated with the compiler. https://github.com/llvm/llvm-project/pull/68091 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
