This is firing even in checked length codes, is that expected?

In D136765#4205989, @philnik wrote:

In D136765#4205962, @hans wrote:

In D136765#4203247, @ldionne wrote:

I do agree however that if someone (in this case Chrome) is using a not-yet-released snapshot of trunk that happens to have _LIBCPP_CLANG_VER >= 1600 yet isn't something we've released as LLVM 16, then it's probably not reasonable for us to try to support that. Imagine coming up with that policy, I think it would mean that we can't ever rely on _LIBCPP_CLANG_VER >= 1600 during the libc++ 16 release, which would be pretty unfortunate.

(FWIW it's ChromeOS, not Chrome, that's using an older Clang version. And if necessary they can probably patch in the needed compiler-rt changes. Also, https://libcxx.llvm.org/ technically says that clang "16-git" is supported, which is what they are using ;)

It's "16-git" as-in current trunk, not some random version from a few months ago.

I'll stop harping on about this, but I think gating functionality on _LIBCPP_CLANG_VER isn't great for the reason we've seen here. (And Eric predicted it in his "It smells weird, and it looks weird, and I think it'll lead to bugs." comment). I'm pleased to see that it's currently only used in two places in libc++, one of which is for a deprecation warning. As mentioned above, the best would be if compiler-rt exposed some kind of feature check for this, but if we can't have that, I think >= 17 is the way to go.

I don't think >= 17 is an option. You are basically asking us to keep the code around for another release because someone used some wacky unsupported configuration. Either live at head or use releases. They exist for a reason.

In D136765#4178089, @hans wrote:

In D136765#4173661, @ayzhao wrote:

FYI this is causing (yet another) bug on Chrome/Chrome OS: https://crbug.com/1422033

We've root caused this to ChromeOS using a toolchain based on llvmorg-16-init-6711-g11897708c022, which is before the ASan runtime gained support for unaligned container memory in llvmorg-16-init-9006-gdd1b7b797a11, which this patch depends on. (https://crbug.com/1422033#c11 has a reproducer).

I see that the patch is already trying to handle this with the #if _LIBCPP_CLANG_VER >= 1600 check. However, that doesn't quite work since there were many revisions with that version number before the ASan change.

Should it be checking _LIBCPP_CLANG_VER >= 1700 instead, or is there some more precise way to handle this? (Does the ASan runtime have some way to check if it supports this for example?)

At least for ChromeOS, as long as this change is limited to clang not making calls to it and function is not removed from compiler-rt, it works for us . We ship a libgcc_s.so using compiler-rt sources so we need so compatibility there so that older binaries keep working.

The change looks simple enough and do not see any issues raised.

The patch looks simple enough to me. But I do not know this code patch well enough to accept it. Agree that Inlining cost estimation work should not block this.

If ChromeOS needs time for migration, I think -Xcompiler can be temporarily ignored.

Here are a few instances of Xcompiler usage for a non-exhaustive search (I can't look inside package tarballs if they are using it ):

Without -Xcompiler, ChromeOS code will break. It may not be supported by GCC but it is supported in some other compilers like Cuda and a few others if you search. Also being supported by libtool makes it more important to keep it working.

Xlinker still works. Xcompiler is failing.

We use -Xcompiler and -Xlinker which are passed in programs and they raise error now.

The removal is also breaking ChromeOS builds which use -Xpattern in some cases.

Updated test.

Restore back to C++ only.

restore back to C++ only

@tomhughes has more details on this but if we do not define it in clang itself, we'll need to define it for every package manually at build time since the usage goes deep inside newlib headers.

I am not sure who is a good reviewer for this. Starting with @MaskRay and @efriedma

thanks, I assume @MaskRay is a good reviewer for it.

Removed c++ limitation.

Abandon in favor of D136708

address comments and check CI.

friendly ping for review.

Thanks for the patch. Can you please post a full diff (git diff -U9999).

Going back to older handlesTarget() style in Baremetal.
Apparently RISCV can be both Baremetal or RISCVToolchain for the
same tuple. I do not know of the nuances so trying not to disturb that for
now.

Fix clang-format complains.

Address maskray comments.

Address more style lints.

Address style nits.

Moved defs to Triple.cpp

Moved Baremetal triple related code to Triple.h.

Thanks, I can verify that this fixes the crash I reported.

Still crashes on trunk.

Heads up: I am seeing a clang crash on arm with this commit:

If you can't wait until Wednesday or Thursday you can always revert the diff downstream.

Will submit after CI results come back.

Will submit after CI results.

@philnik @ldionne Can this patch please be reverted? There are clear test cases that it breaks.

The following code now longer works after this patch:

This seems like same as the problem that was fixed by https://reviews.llvm.org/rGe6a39f00e8d0cd3684df54fb03d288efe2969202 and re-introduced by this patch.

Thanks for clarifying that ELF is not affected, I just wanted someone from ARM to verify.

@danielkiss can you please review?

Daniel, can you please check if this removal is ok. I am particularly worried about pthread_cancel which is often hard to debug.

Yes, LLVM_ENABLE_RUNTIMES and LLVM_ENABLE_PER_TARGET_RUNTIME_DIR are two separate things.

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.