Looks ok to me.

--warn-backrefs-exclude='*' disables all warnings. We may need --no-warn-backrefs if it is more like the option users want to use. If you are happy to try out, I'd happy to know whether ChromeOS packages have no warnings if --warn-backrefs is added (--fatal-warnings upgrade warnings to errors).

@MaskRay Is there a global option to disable e.g --no-warn-backrefs . It is not easy for us to add -warn-backrefs-exclude=<glob> individually to all packages where we need to suppress the error.

D80491 has been breaking our ToT builds for a week now. Can this be submitted soon. If not please consider reverting D80491.

@nikic Thanks for the work.

In D76848#2008278, @efriedma wrote:

@manojgupta can you give a build recipe? None of the buildbots are failing.

In D76848#2007875, @vhscampos wrote:

Hi @manojgupta. I am not familiar enough with this piece of code to understand whether it could be fixed by simply using any other general-purpose register instead of r7.

Can you request it to someone more accustomed to compiler-rt?

@vhscampos This change is more restrictive than GCC. e.g. https://godbolt.org/z/wP8_ic where clang errors out but GCC does not error out.

I believe chromium's version is a copy of compiler-rt's implementation of clear_cache [1] .
@vhscampos Can you please fix compiler-rt version as well?

I opened https://github.com/ClangBuiltLinux/linux/issues/979 to see if we can fix this in Linux kernel.

Yes, it'd be nice if all of the FORTIFY handling can be improved. For a simple call like memcpy of 8 bytes in the example, there is no reason to emit all these stack/range checks since they'd degrade memcpy performance.

Copying an email reply to phab thread from @EricWF , not sure why it did not make it here.

Unfortunately, cherry-picking the kernel patches didn't work including latest memcpy for x86 (https://github.com/torvalds/linux/commit/170d13ca3a2fdaaa0283399247631b76b441cca2 and https://github.com/torvalds/linux/commit/c228d294f2040c3a5f5965ff04d4947d0bf6e7da ).
Also tried ToT Linux kernel but still the same problem.

I was able to reduce to following:

Sure, I am trying to root cause the issue. Will report back hopefully soon.

We believe this change (relanded as https://reviews.llvm.org/rGd437fba8ef626b6d8b7928540f630163a9b04021) is causing a mis-compile in Linux kernel 4.4 builds resulting in local test failures.
Chrome OS bug: https://bugs.chromium.org/p/chromium/issues/detail?id=1066638

Noticed a few typos. Rest lgtm but deferring to other reviewers for now for approval.

Thanks for the quick fix, verified that the crash is fixed on trunk.

Still causing a crash using a previously supplied test https://bugs.chromium.org/p/chromium/issues/detail?id=1061533#c7. Any reason this was not tested with a previous repro?

Thanks a lot!

Friendly ping @pcc and @ldionne . We have been carrying this patch I think for too long now.
Also, we have not discovered any LTO issues elsewhere so can't tell from our side if there are other places in libc++ that need visibility annotations.

Can you also test on the reduced test cases in https://bugs.llvm.org/show_bug.cgi?id=45181 ?

I see another crash with this change when building gdb.

Thanks, Verified that this fixes the kernel warnings in my local builds with https://gist.github.com/nathanchance/767cccf4d093c1342e1994083518815e!

Thanks, should this be cherry picked to 11.0 release as well?

Opened PR43518 with repro instructions.

Heads up: This is causing an error when building Chrome on ARM32 (Thumb2):

Submitted as https://reviews.llvm.org/rL371785.
Thanks for the patch!

Thanks for working on it. Have you tested if Linux perf can symbolize the perf traces using the split debug files especially with large binaries?

Hi Fangrui,

Adding a few more reviewers since I am not very familiar with this part of clang.
Please also update the patch description as suggested by @thakis

In D52524#1630767, @thakis wrote:

Wouldn't those projects just move to also disabling the warning by passing -Wno-poison-system-directories? If there are projects that are actively adding -I/usr/include, that means they're consciously fighting the build system and you've kind of already lost, no? Can't you tell them to not use -I/usr/include?

In D52524#1621468, @thakis wrote:

Couldn't cross build users just pass -nostdsysteminc to tell clang to not look in system header locations?

@pcc can you please submit this patch if there are no objections?

@dankm are you still working on this patch?

@ldionne Does Peter's example answer your questions?

Hi Peter and Marshall,

In D60472#1461351, @peter.smith wrote:

In D60472#1461351, @peter.smith wrote:

The motivation for this change is to make "crypto" setting an additive option e.g. like "-mavx" used in many media packages. Some packages in Chrome want to enable crypto conditionally for a few files to allow crypto feature to be used based on runtime cpu detection. They set "-march=armv8+crypto" flag but it gets overridden by the global "-march=armv8a" flag set by the build system in Chrome OS because the target cpu does not support crypto causing compile-time errors.
Ability to specify "-mcrypto" standalone makes it an additive option and ensures that it it is not lost. i.e. this will help in decoupling "-mcrypto" from "-march" so that they could be set independently. The current additive alternate is '-Xclang -target-feature -Xclang "+crypto" ' which is ugly.

In D53608#1423692, @LuoYuanke wrote:

Hi
What's the status for __float128 support? Has it already been finished?

In D54487#1298688, @JohnReagan wrote:

Yes, but that is somewhat different. The "-g" option seems to just append the command line to the producer string, yes? It is harder to parse out later and it can be stripped out. gcc has both -g and -f forms.

Doesn't clang already has -grecord-gcc-switches option (https://clang.llvm.org/docs/ClangCommandLineReference.html#debug-information-flags) ?

Took another look and seems like long double is hardcoded in many of the builtins. So I think the current patch needs to rename a lot of places using long double to __float128 type.

I am also getting these tests failures because of missing trunctfxf2 and extendxftf2. These are provided by libgcc but compiler-rt does not seem to have an implementation for them.

Added checked for defined(FLOAT128) || defined(SIZEOF_FLOAT128)

lgtm. But someone more familiar with these code paths should approve.

As per https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80868, I thought GCC also emits this error but only with -pedantic. So probably should keep this error but under -Wextra or another appropriate group?

In D51713#1232497, @srhines wrote:

In D51713#1232414, @manojgupta wrote:

What is the call generated with -pg for AMR32, gnu_mcount_nc or _mount? gnu_mcount_nc with "-pg" is known to be broken ( https://bugs.llvm.org/show_bug.cgi?id=33845)

I CCed myself on that issue as we are trying to do a better job of supporting code coverage, but I don't think this bug is relevant for this patch.

Unless there are any additional reviews, I will likely submit this patch in a few hours.

What is the call generated with -pg for AMR32, gnu_mcount_nc or _mount? gnu_mcount_nc with "-pg" is known to be broken ( https://bugs.llvm.org/show_bug.cgi?id=33845)

In D51440#1217910, @mstorsjo wrote:

In D51440#1217839, @manojgupta wrote:

Just a minor comment regarding test cases: Since you are adding both -L/path/ and -l<libname>, the test cases should be updated to check for the -L/path/ argument as well.

I guess I could do that, although we don't know the path in the test, so we can only check for -L.*.

Just a minor comment regarding test cases: Since you are adding both -L/path/ and -l<libname>, the test cases should be updated to check for the -L/path/ argument as well.

In D50000#1180760, @lebedev.ri wrote:

Congrats on D50000 :)

Random uneducated thought - how does it work with multiple translation units, where not all of them have/don't have that flag set?

In D47895#1175164, @a.elovikov wrote:

Note that, I am expecting that functions with alwaysinline attribute should still get inlined. If that happens and caller does not have this attribute, then optimizer is free to remove the checks.

Not that I actually hit that, but it was questionable when reading the code - why that way? From LangRef: alwaysinline "indicates that the inliner should attempt to inline this function into callers whenever possible, ignoring any active inlining size threshold for this caller." So there is some freedom by not doing the inlining, even in the presence of the attribute.

Wouldn't it be better not to inline or to add the attribute to the caller?

Thanks for the quick review!

Thanks Piotr for the testcase.

What clang version is that? Works fine for me.

@efriedma Maybe not relevant to the patch here but our kernel devs were looking into preserve_all but it does not seem to work for AArch64.

Added helper text, updated tests.