It's also worth asking whether -nodefaultlibs would work for this use case as @filcab suggested on D64547.

LGTM

LGTM

Sorry, just realized this. If I do

clang++ -c -flto a.cpp # "split"
clang++ -c -flto=thin b.cpp -fwhole-program-vtables # non-split
clang++ a.o b.o

this should fail, right? If I'm not mistaken, this patch series will cause this to succeed. I think we need to change this patch so that it always sets EnableSplitLTOUnit to 1 for regular LTO, then you can drop the other patch.

Unfortunately it looks like this part of the condition had no test coverage before I refactored it in r366494, which is how the mistake slipped through.

• Add comment

LGTM

The instrumentation for globals is coming in a separate patch.

IIUC the reason was to work around the lack of analysis availability from module passes, which is no longer a concern with the new PM. I would personally prefer to see the other sanitizers re-architected this way.

Using /*/ as the ignore pattern would work for me. My main concern was that it would conflict with my use of /* in the exclude file to ignore files in the root directory, but it appears that they don't conflict. If @jyknight is happy with this then I'll update this review as suggested.

As I see it it's not too different from e.g. internalizing or hiding symbols or propagating DSO-local, which is, in the end, also overriding a compile-time decision. I think that taking this sort of stance in general makes it hard to say where to draw the line on what kind of optimizations are permitted. If we do switch to the compile-time relocation model, should the backend then be prevented from generating jump tables with absolute addresses when linking without -pie or -shared because it could cause problems if the binary happens to end up being loaded at the wrong address? That doesn't seem like a good idea to me. It seems more practical to say, at least in this case, that the binary that you get from a non-PIC link is, in fact, position-dependent, and it is an error to use it position-independently.

In D48680#1587967, @ldionne wrote:

@pcc In your reproducer, what is ~/l3/ra-cmake/bin/clang++?

I think it would be up to the libc++ maintainers to approve the patch.

In D64711#1586814, @phosek wrote:

In D64711#1586738, @pcc wrote:

In D64711#1586696, @phosek wrote:

In D64711#1584916, @pcc wrote:

I'd like to understand the motivation behind this change a little better because the relaxation of the code model from PIC to static is technically an "optimization" that the linker should have license to perform and in fact is already performing in some ways (e.g. emitting addresses into the binary instead of RELATIVE/RELR relocations for statically known addresses).

In the case of our kernel, we need the generated code to be position independent for our KASLR scheme, but we're not doing traditional PIE link. When compiling individual translation units, we use -fpie, but when linking we use --no-pie. Changing the code model from PIC to static in this case results in an invalid kernel image because the generated code is not relocatable. I think it's a matter of principle that it shouldn't be the linker's responsibility to decide how to combine our code-generation choices with our linking choices. I'm open to suggestions for other ways to solve this issue though as we'd really like to be able to use ThinLTO for our kernel.

Have you considered linking the kernel as a PIE image when KASLR is enabled? This will fix not only this problem but also others (e.g. initializers containing global pointers, which might not just be explicit in the source code but could also be implicitly generated by the compiler, are broken with this scheme).

This is what the Linux kernel does (it uses -shared -Bsymbolic instead of -pie in KASLR mode, but they are almost equivalent):
https://github.com/torvalds/linux/blob/fec88ab0af9706b2201e5daf377c5031c62d11f7/arch/arm64/Makefile#L21

It uses assembly to self relocate to avoid depending on relative relocations during early startup:
https://github.com/torvalds/linux/blob/fec88ab0af9706b2201e5daf377c5031c62d11f7/arch/arm64/kernel/head.S#L822

We're considering it but it requires some non-trivial changes and so won't be ready anytime soon.

• Add test

In D64711#1586696, @phosek wrote:

In D64711#1584916, @pcc wrote:

I'd like to understand the motivation behind this change a little better because the relaxation of the code model from PIC to static is technically an "optimization" that the linker should have license to perform and in fact is already performing in some ways (e.g. emitting addresses into the binary instead of RELATIVE/RELR relocations for statically known addresses).

In the case of our kernel, we need the generated code to be position independent for our KASLR scheme, but we're not doing traditional PIE link. When compiling individual translation units, we use -fpie, but when linking we use --no-pie. Changing the code model from PIC to static in this case results in an invalid kernel image because the generated code is not relocatable. I think it's a matter of principle that it shouldn't be the linker's responsibility to decide how to combine our code-generation choices with our linking choices. I'm open to suggestions for other ways to solve this issue though as we'd really like to be able to use ThinLTO for our kernel.

I think it's a little unfortunate that we're continuing to go down the road of letting users pass more flags to the ThinLTO backend action, but I won't stand in the way here.

I'd like to understand the motivation behind this change a little better because the relaxation of the code model from PIC to static is technically an "optimization" that the linker should have license to perform and in fact is already performing in some ways (e.g. emitting addresses into the binary instead of RELATIVE/RELR relocations for statically known addresses).

Ah, I missed that.

I would prefer to keep this the way it is with an array so that it is obvious what the size/alignment are.

This change broke the fuzzer libc++ build on multiple sanitizer buildbots, e.g.:
http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux/builds/22287
http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap/builds/13081

In D64597#1582944, @hubert.reinterpretcast wrote:

In D64597#1581605, @pcc wrote:

The problem with 0xaaaaaaaa on 32-bit is that it is likely to be a valid address.

When I discussed this with JF I proposed a pointer initialization of 0xffffffff which he agreed to. This value is very likely to trap when accessed (due to accesses likely wrapping to zero) and also has the benefit of being the same pattern as for floats.

The value is very likely not to trap on some systems (due to accesses likely wrapping to zero). I am not sure I have a better magic value to offer though.

The problem with 0xaaaaaaaa on 32-bit is that it is likely to be a valid address.

FWIW, I did something similar: https://github.com/pcc/llvm-test-suite/tree/android
but never got time to clean it up and send it for review.

Hi Oliver, thanks for working on this. This is something that I've always wanted to do with the type metadata but never had time to work on. I'll try to take a more in depth look later this week.

In D64456#1577531, @MaskRay wrote:

Might be worth adding another test to aarch64-fpic-adr_prel_pg_hi21.s

• Address review comments

Do we really need to support clang-cl syntax here? Can't the user of -fthinlto-index= use the regular clang driver?