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.

@RKSimon We identified that this caused a clang crash on our ToT builds https://bugs.chromium.org/p/chromium/issues/detail?id=1182316.

Thanks a lot Fangrui.

In D96292#2563966, @MaskRay wrote:

In D96292#2549683, @manojgupta wrote:

This change make further deviation from GNU tools. Lets wait for resolution of D93881. This will not be needed if D93881 is merged.

This change actually makes our behavior consistent with current GNU strip/objcopy. GNU strip/objcopy does not copy S_ISUID and S_ISGID bits.

chmod u+s,g+s,o+x a
sudo llvm-strip a -o b

Currently b has S_ISUID and S_ISGID bits.

This change make further deviation from GNU tools. Lets wait for resolution of D93881. This will not be needed if D93881 is merged.

Thanks Fangrui for working on this!

This CL has caused two issues in Chrome OS :
Compilation fail with FORTIFY: https://bugs.chromium.org/p/chromium/issues/detail?id=1168199
Runtime failures: https://bugs.chromium.org/p/chromium/issues/detail?id=1167504

Could you just update the build instructions for these few packages?

I see over 600 uses of of fowners in Gentoo and over 55 in Chrome OS (public). Not all uses are related to binary files but asking to fix the build system instead of fixing the incompatibility is a non-trivial and a no-go.

[1] https://github.com/gentoo/gentoo/search?q=fowners&type=code
[2] https://source.chromium.org/search?q=fowners&sq=&ss=chromiumos

OK. If that is the case, I am happy that llvm-objcopy adopts the GNU objcopy behavior.

But I think we should do better: instead of: (1) create a temp file (2) open it by name again (3) check it is a regular file with one hard link & it is writable (S_IWUSR) (4) fstat (5) fchown,
we should just modify the destination in place (for objcopy file and strip file). If the file has been mapped readonly, on many platforms it may be unwritable. We have to accept this.

The various security checks is to prevent vulnerability.

Thanks, we will work on an improved patch with the added checks.

In D93881#2483542, @MaskRay wrote:

GNU objcopy and strip preserve the ownership of a file when it's overwritten, e.g. while "sudo objcopy foo -o bar" creates a new file bar with root access, "sudo objcopy foo" will keep the owner and group of foo unchanged. Currently llvm-objcopy and llvm-strip behave differently, always changing the ownership to root.

objcopy foo -o bar => objcopy foo bar

I am unsure that we want to adopt the GNU behavior because

(1) there were security vulnerabilities
(2) the code is subtle and untestable
(3) the behavior is self inconsistent. The behaviors of sudo objcopy foo and sudo -u mail objcopy foo (if user mail has write permission to the directory) are different.

In D93881#2483309, @nickdesaulniers wrote:

It sounds like portage should be using something like setuidgid to drop privileges for parts of the build.

In D93881#2483184, @MaskRay wrote:

I don't think we want to introduce a filename based chmod API. Unless it is essential (very unlikely), it is easy for developers to make time-of-check time-of-use (TOCTOU) race condition allowing bypass of protection mechanism due to symlink attacks.

binutils 2.36 will fix the issue https://sourceware.org/bugzilla/show_bug.cgi?id=26945

Hi Daniel,

lgtm but not an expert in this area.

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.