We are noticing a clang crash after this commit: https://github.com/llvm/llvm-project/issues/55510

Thanks Christopher,

We can close the discussion as Fangrui has uploaded https://reviews.llvm.org/D125074 to keep older behavior.

Thanks fangrui.

This change is breaking rust embedded use cases where NOLOAD in a linker script is no longer being honored. So we had to revert it (locally) in ChromeOS.

Hmm, the commit message says that Wno-error should work but this is not really the case :(.

Following behavior is also surprising:

Tried locally but I still see the warning with -fno-knr-functions. It also says that the argument is unused.

We are finding a lot of failures in our ToT builds with this change. here is an example for a configure script:

Unless I probably mis-interpreted something, -fno-knr-functions does not suppress the warning: https://godbolt.org/z/rbEfbbb33

Basically, I'm wondering if you'd be able to enable -fno-knr-function?

Thanks. this looks promising. Any ideas when -fno-knr-function support was added?

Some of our users are not very happy with the churn probably caused by this change where the declaration has the "void" argument but the later definition does not have explicit "void".

Thanks, this also fixes one of the test failures we were seeing on AArch64 boards.

We noticed a new crash that still repros at head.

Replaced uses of /dev/stdin by /dev/null and added asserts.

Added tests for stat64 and fstatat64

In D121652#3381363, @vitalybuka wrote:

to avoid crash you need:
INIT_STAT64;
INIT_LSTAT64;
around sanitizer_common_interceptors.inc:10554

can you please add corresponding tests similar to
llvm-project/compiler-rt/test/sanitizer_common/TestCases/Posix/lstat.cpp
llvm-project/compiler-rt/test/msan/fstat.cpp

Fixed crash and added suggested tests.

I am noticing a unit tests error with this change for glob_altdirfunc.cpp file but not sure how to resolve it.

MemorySanitizer:DEADLYSIGNAL
==3854677==ERROR: MemorySanitizer: SEGV on unknown address 0x000000000000 (pc 0x000000000000 bp 0x7ffeeab8eb60 sp 0x7ffeeab8eb28 T3854677)
==3854677==Hint: pc points to the zero page.
==3854677==The signal is caused by a READ memory access.
==3854677==Hint: address points to the zero page.

For the background, we had hit this in Chrome OS when building bluetooth code.

In D118376#3360001, @manojgupta wrote:

We are noticing some failures in internal unit tests with this change.

Tests pass at 588f121ada6d541 but start failing at b4b97ec813a0258.

We are noticing some failures in internal unit tests with this change.

We noticed another crash with this change that still repros on ToT: https://github.com/llvm/llvm-project/issues/54190

In D120831#3354933, @ldionne wrote:

I attempted in Feb 2019 here: https://reviews.llvm.org/D57688.

I ended up dropping the effort after the discussion at https://discourse.llvm.org/t/removing-deprecated-ext-hash-set-ext-hash-map-and-ext-hash/51068.

FWIW, I still support this removal, but I'd like to get a better understanding of what's broken by it before moving forward.

Running readelf shows following in our shipping shared libs.

We had never defined LIBCXX_ENABLE_ASSERTIONS and _LIBCPP_DEBUG explicitly in our cmake config for Chrome OS.

I am fine with this change but Nick should review.

I have removed the usage of this flag from Chrome OS so fine for us as well.

testing out Chrome OS change at: https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/3308214

Will it also fix the bug https://bugs.llvm.org/show_bug.cgi?id=49294 related to m32?

Thanks for adding me. This is going to break Chrome OS 32-bit builds so I'll need to check if I can remove uses of LIBCXX_BUILD_32_BITS with something more suitable.

More subjective: for most users this whole -march business is abstracted away in build systems, so they won't have to deal with this, that's why this isn't so much of an improvement.

Chrome OS bug: https://bugs.chromium.org/p/chromium/issues/detail?id=1233594. Seems like it was tcl that was miscompiled, not LUA.

Late comment but this broke LUA (could be a mis-compile ) which further broke building sqlite. This commit was reverted in Chrome OS (https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/3066844) but seems like the reporter forgot to mention it upstream.

Matt, do you think you can review this?

Yes, the current approach of "-march=<isa>+feature" is terrible and does not work with developers who want flexibility of features. This being pitched as a feature imo is akin to promoting a design bug as a feature.
Any additive or subtractive alternative is welcome.

Assuming CMAKE_SYSTEM_NAME implies the machine, clang is being built on,
it may also break baremetal targets since clang does not currently handle per ABI runtime dirs for baremetal targets
e.g. see the problem in https://reviews.llvm.org/D98452

Sure, see D113357

Yes, you can check for COMPILER_RT_BAREMETAL_BUILD in cmake to keep the older naming for baremetal. The baremetal targets have very different driver searching mechanisms for libs, not sure why.

Sorry for replying since this patch has been there for a while. But this has broken the installs for the baremetal targets eg armv7m-none-eabi.

@compnerd can you please check again.

@kcc do you have any concerns?

I'll send the cache and other details by email.

This is also causing build failures in our ToT builds.

thanks, I can verify that it fixes the crash we were seeing.

I am noticing a clang crash with ToT after this change.

Unfortunately, explicit CXXFLAGS is not enough. If we add CXXFLAGS, it will make all of llvm including clang to be built with CET which is not what we want.

I tried to use it locally and ran into following issues:

Thanks Daniel. We still have a problem with pthread_cancel but this resolves the immediate crash.

We have been trying to debug 1 issue on ARM32 Chrome OS devices related to pthread_cancel.
After some debugging, I found that glibc uses char[8] for the exception class [1] but libunwind is using uin64_t.
This causes crashes because of ABI mismatch when pthread_cancel calls Unwind routines.

This is a local patch we have been carrying in Chrome OS.
What we do to cross compile lldb-server:

1. Create host build to build tools like llvm-tblgen, clang-tblgen, lldb-tblgen
2. Cross compile llvm and clang libraries using the just build *tblgen.
3. Cross compile lldb by setting the path to pre-cross compiled libraries and *tblgen binaries from step 1.

In D107560#2932805, @MaskRay wrote:

Wait. Can't the user just specify -fcf-protection=full in CMAKE_CXX_FLAGS?

Please also see https://bugs.llvm.org/show_bug.cgi?id=51416

I'd rather not be in a situation where a program built with a normal production toolchain appears to work, but then randomly crashes because the unwinder clashes with libgcc. Unless Chrome OS actually refuses to load binaries built with a normal Linux toolchain, we should ensure those binaries work.

If there's crosstalk between libgcc.a and libunwind.so, binaries built on systems with true libgcc.a will break on systems with libunwind.so installed as libgcc_s.so. Messing with the names is just covering up the issue.

@pcc @cferris Would you be able to review this libunwind patch?

An earlier version did check for library directories [1]. I am not exactly sure why was it removed, maybe it didn't work. So if anyone is willing to test that, please apply the diff and try.

Thanks for the quick turn around, I can verify that https://reviews.llvm.org/rGee8a5e4bc2c986b8e6c07e81fb58dc1e5a5c2d17 fixes the issue.

@cjdb please pick the fix once available to chrome os.

@avl Did you test this change? It has broken setuid permission preservation with sudo.

@MaskRay I have verified that Chrome OS builds are not affected by this change.

Thanks for the clarification. I do not have any objections but I feel that am not the right person to approve this change.
@tstellar can you please review it?

Another concern is people generally want clang to work out of box without any special arguments. As a user, after installing clang's distro package or building from source, I expect that basic compilation should work out-of-box which includes gcc detection.
i.e. "clang foo.cpp -o foo" should just work in most cases.

Thanks for explaining that it only affects "-B". While I believe that this change won't affect us in Chrome OS, I think it should be reviewed and approved by a few Linux distro contributors since there is already known reliance e.g. Android on the current behavior.

thanks, this is much needed documentation.

I am not sure of the rationale or upside of this change and why do we want to drop gcc detection? GCC does not need to do the GCC detection because it has the needed information at configure time.

With the special rule, it is: if --gcc-toolchain is $sysroot/usr, suppress sysroot GCC detection as well.

Clang -B and --gcc-toolchain have some weird behaviors. Hope you can share your opinions on https://lists.llvm.org/pipermail/cfe-dev/2021-March/067827.html

In Chrome OS, we currently both "-B" and "--prefix". "-B" points to binutils bin directory and the "--prefix" has the binutils directory + target suffix. Should we drop "-B" and still get the same behavior?

Thanks @MaskRay for this clean up. I can't speak for all of Gentoo but please give me a couple of days to at least test this on Chrome OS.