In D35639#816341, @tejohnson wrote:

In D35639#816132, @tejohnson wrote:

Actually, I have a variant of this that I prefer. Instead of setting VisibleToRegularObj in each linker, we could handle it similar to the way some COFF specific behavior is handled. I.e. when building the symbol table, if isOSBinFormatELF, and the symbol has a C identifier section name, then set a (new) flag on the symbol indicating as such (e.g. ELFHasCIdentifierSectionName). Then in LTO::addModuleToGlobalRes, we add a check of this flag to the conditions under which VisibleOutsideSummary is set. That has the advantage of avoiding the need to add checking to each ELF linker, and also doesn't overload the VisibleToRegularObj flag with a condition that isn't being visible to a regular object, but rather visible outside the summary in a different way. Sound ok?

Note in addModuleToGlobalRes it is also added to the set of conditions where we need to set the Partition to External (similar to the places where we currently check things like VisibleToRegularObj. I have this working for a small test case, testing with my full internal test case. If that works I will clean up and send for review. I like this since it is ELF specific but doesn't require any linker changes.

In D35639#815587, @tejohnson wrote:

In D35639#815579, @pcc wrote:

In D35639#815578, @tejohnson wrote:

In D35639#815234, @pcc wrote:

Another possibility is to handle this in the client (i.e. the linker) instead. The client has the best idea of whether a given section is a GC root; ELF linkers can set VisibleToRegularObj on any symbol in a section named like a C identifier, and other linkers can do whatever is appropriate for their object format.

I don't see how to get the presence of a section or the section name for a symbol when walking the symbols? I.e. either in gold-plugin via the symbol info from the linker, or in LTO::addModuleToGlobalRes via the InputFile:Symbol?

We'd need to add that information to the irsymtab and expose it via InputFile::Symbol.

That's what I was afraid of. =( That seems quite a bit more extensive than simply checking whether the GV has a section and blocking internalization.
If we were to go the route you are suggesting, it looks like currently we don't iterate through the InputFile until we invoke the LTO API, when adding a module. I guess we would have to then iterate through those and update the global resolutions earlier, e.g. in the gold-plugin? I.e. in order to make it ELF specific.

In D35639#815578, @tejohnson wrote:

In D35639#815234, @pcc wrote:

Another possibility is to handle this in the client (i.e. the linker) instead. The client has the best idea of whether a given section is a GC root; ELF linkers can set VisibleToRegularObj on any symbol in a section named like a C identifier, and other linkers can do whatever is appropriate for their object format.

I don't see how to get the presence of a section or the section name for a symbol when walking the symbols? I.e. either in gold-plugin via the symbol info from the linker, or in LTO::addModuleToGlobalRes via the InputFile:Symbol?

LGTM

• Address review comments

Another possibility is to handle this in the client (i.e. the linker) instead. The client has the best idea of whether a given section is a GC root; ELF linkers can set VisibleToRegularObj on any symbol in a section named like a C identifier, and other linkers can do whatever is appropriate for their object format.

Please add a test that shows that the thinlink files can be read by llvm-lto2.

the global VST (for the global values we will have in the index) does not.

LGTM

FYI, I don't think HEAD on its own will work because it is not necessarily updated on every commit, see discussion on D31985.

Simon should probably review this change.

LGTM

• Address review comments

• Address review comments

In D34644#792470, @fjricci wrote:

As far as I'm aware, the feature exists both to save time building and testing sanitizers you don't need, and to get around differing build requirements across the sanitizers (it's possible, especially when cross-compiling, that some sanitizers build in a given configuration and others do not).

I don't understand why we have this COMPILER_RT_SANITIZERS_TO_BUILD feature, but given that we have it, this seems reasonable to me.

Using LLVM_APPEND_VC_REV seems fine to me, but we probably want to update the documentation and most likely change the default because right now the VC revision affects ThinLTO cache invalidation, and we shouldn't silently use stale cache entries when clang is upgraded.

Depends on D34546

In D34459#786952, @evgeny777 wrote:

Peter, I tried doing this, but it forces dynamic reloc to .Label, which can reside in RO section.

I think @eugenis fixed this in r305584.

LGTM

Still LGTM

Please confirm that we can still self host with full LTO now that D33922 has landed.