- User Since
- Dec 28 2012, 2:34 PM (342 w, 2 d)
Fri, Jul 19
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.
Thu, Jul 18
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.
Wed, Jul 17
- Add comment
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.
Tue, Jul 16
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.
I think it would be up to the libc++ maintainers to approve the patch.
Mon, Jul 15
- Add test
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.
Sun, Jul 14
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).
Fri, Jul 12
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.:
Thu, Jul 11
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.
Wed, Jul 10
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.
Tue, Jul 9
- 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?