[DNM] Revert "IRGen: Use llvm.compiler.used instead of llvm.used on ELF"

[kateinoigakukun]

This reverts commit 2bbea54 to see what multiple sections for each global broke.

The use of llvm.compiler.used as a replacement for llvm.used changes the semantics of symbol requirements at link time, and it loses SHF_GNU_RETAIN for generated swift5 metadata sections.

I'm not sure why multiple same name sections were problem here 🤔

[kateinoigakukun]

@swift-ci Please test

[aschwaighofer]

The problem is/was that you end up with multiple section of the same name.

The runtime (CC @mikeash) and tools look for said section and (presumably) can't deal with the presence of multiple same named sections in one binary on linux and therefore you see failures.

Speculating ..., to address this, maybe the runtime could be modified? I am assuming that this means something like: if you have seen one section of (e.g) swift5_protocol_conformances continue looking at potentially other sections of the same name.

[compnerd]

The section data should be coalesced, so it seems like we might be missing something.

[aschwaighofer]

The fact that they are not coalesced seems to be intentional feature of SHF_GNU_RETAIN. https://reviews.llvm.org/D97448

For front ends which do not expect to see multiple sections of the same name,
consider emitting @llvm.compiler.used instead of @llvm.used.

[kateinoigakukun]

IIUC, multiple same-name sections are OK in object files, but the problem here is linker does not coalesce those same-name sections appearing in a single object file, but just pick a first one. I will have a closer look again.

[kateinoigakukun]

Okay, the problem was that:

1. gold linker creates separate sections for same name input sections with different section flags. source
2. After reverting 9ce1baf, each swift5 section emitted by compiler has SHF_GNU_RETAIN, but swiftrt.o's sections didn't.
3. swiftrt.o is placed before Swift object files in gold linker command arguments, so gold creates encapsulation symbols (start/stop) for the swiftrt.o's empty sections.

The 1. behavior is inconsistent between gold and lld (lld merges those sections into one), so it was not a problem with lld.

I applied 0e410da to add SHF_GNU_RETAIN flags to the swiftrt.o's empty sections to have consistent section flags with compiler-emitted object files.

I tested stdlib test suites with the following env var and it works on my end.

SWIFT_DRIVER_TEST_OPTIONS=" -Xlinker --gc-sections -use-ld=lld"
SWIFT_DRIVER_TEST_OPTIONS=" -Xlinker --gc-sections -use-ld=gold"

[kateinoigakukun]

@swift-ci Please test Linux platform

[kateinoigakukun]

I saw some of the reflection tests failed because some of reflection sections like swift5_typeref and swift5_reflstr are not flagged with SHF_GNU_RETAIN, but swiftrt.o has empty sections with the flag. 05e2fc1 fixes part of the problem by removing the flag for the reflection sections.

However, some of reflection sections like swift5_fieldmd are usually retained but not retained when using -reflection-metadata-for-debugger-only or -disable-reflection-metadata.

https://github.com/apple/swift/blob/aa5b505014b4f622d5f1a9846f1a48d3ba7118fc/lib/IRGen/GenReflection.cpp#L724-L729

This conditional section flag decision makes it difficult to have consistent section flags between object files, and it results in multiple reflection sections in the final image.

I think one of the solutions would be adding retained versions of those conditionally retained sections swift5_fieldmd_retain, but it adds 5 more swift5 sections.

What do you think about this solution? and do you have any other good ideas? @compnerd @aschwaighofer

[compnerd]

Could we initialize the section appropriately so that the attributes are applied to that section into which we emit the symbols? We can change the behaviour conditionally based on the object file format that we are emitting into as well if that is necessary.

[kateinoigakukun]

@compnerd Do you mean not to have SHF_GNU_RETAIN section flag for those reflection metadata sections consistently and control retainability by something else? Or do you mean always to mark those reflection metadata as "retained" on ELF object files and not supporting reflection metadata stripping on ELF?

[compnerd]

I was thinking more the latter. We can emit them if reflection is enabled and not strip them.

[al45tair]

The problem is/was that you end up with multiple section of the same name.

The runtime (CC @mikeash) and tools look for said section and (presumably) can't deal with the presence of multiple same named sections in one binary on linux and therefore you see failures.

Speculating ..., to address this, maybe the runtime could be modified? I am assuming that this means something like: if you have seen one section of (e.g) swift5_protocol_conformances continue looking at potentially other sections of the same name.

There are a couple of different problems here. The first is that for ELF images it's impossible to locate sections at runtime by looking through the ELF data structures (the section headers are not mapped into the process's address space and indeed can be stripped from the executable image). The upshot of that is that we're using the linker-generated __start and __stop symbols to locate the sections.

Unfortunately those symbols aren't regarded as GC roots, so when section GC is enabled, the linker will drop some of the metadata sections as there are no pointers into them.

This is then compounded by the fact that gold does not coalesce sections whose flags differ, but I think incorrectly includes SHF_GNU_RETAIN in the list of flags it considers when doing so. IMO it would be better to ignore that flag for coalescing purposes (although it should clearly set the flag on the coalesced section if any section that was merged into it had it set). A great irony here is that gold actually allows coalescing of read-write and read-only sections, or of executable and non-executable sections, which is suspect, but won't coalesce retained and not-retained sections which is much less questionable.

(To clarify, because the sections are not coalesced, and because there can be only one __start and one __stop symbol, we have no way to find the additional sections.)

Even if we were to get a fix into gold, we have to cope with the fact that users likely have an old version of gold, without such a fix, on their system courtesy of their Linux distribution.

[al45tair]

Or do you mean always to mark those reflection metadata as "retained" on ELF object files and not supporting reflection metadata stripping on ELF?

I think this is probably the way forward in the short term. WDYT @aschwaighofer @mikeash? Also, I guess @kubamracek might have some opinions on this?

[al45tair]

(I could do with having this resolved for my static Linux SDK work — see #71841, which is one of the changes I had on my branch, and which, of course, doesn't work in CI because of this problem.)

[al45tair]

FWIW, I just filed a bug against gold, here: https://sourceware.org/bugzilla/show_bug.cgi?id=31415.

That'll fix the issue in the longer term, but doesn't help us now, of course.

[keith]

This is then compounded by the fact that gold does not coalesce sections whose flags differ

My assumption was that we could make it so the flags never differ for these sections. Presumably by making a similar change to #71841 wherever that is in the code, is that not the case?

[al45tair]

My assumption was that we could make it so the flags never differ for these sections. Presumably by making a similar change to #71841 wherever that is in the code, is that not the case?

There is the issue @kateinoigakukun mentions above, where the reflection metadata is marked either used or not used depending on a flag, which complicates matters slightly.

One other option that nobody has mentioned so far is for the runtime to support two sections (with different names, hence different __start and __stop symbols) for every one where this is a problem, then have the compiler put things into one or other of them depending on the setting of the flag. That's doable and might be worth exploring.

[kubamracek]

Or do you mean always to mark those reflection metadata as "retained" on ELF object files and not supporting reflection metadata stripping on ELF?

I think this is probably the way forward in the short term. WDYT @aschwaighofer @mikeash? Also, I guess @kubamracek might have some opinions on this?

I'm okay if we for now declare that reflection metadata stripping on ELF won't work. It's only needed for the -experimental-hermetic-seal-at-link / WME / VFE features, which don't (sadly) work properly on ELF today anyway.

[keith]

There is the issue @kateinoigakukun mentions above, where the reflection metadata is marked either used or not used depending on a flag, which complicates matters slightly.

I'm okay if we for now declare that reflection metadata stripping on ELF won't work. It's only needed for the -experimental-hermetic-seal-at-link / WME / VFE features, which don't (sadly) work properly on ELF today anyway.

Is the section not being there the important part? I guess I assumed that if the section was there, but empty, that would be "good enough" for that disabling as well (even if it's not ideal)

[kubamracek]

Not sure which exact part you're asking about. The -experimental-hermetic-seal-at-link feature can work with reflection metadata being completely off, but that regresses debugging because without reflection metadata, LLDB doesn't know how to inspect variables and understand their types. Runtime execution will still work, though.

[keith]

Sorry I was just talking about the section presence part. If the section is there, but empty, I assume that would be ok?

[al45tair]

I've been looking at this some more. The runtime only cares about the section if it's retained, so swiftrt.o only needs to include declarations for the retained section. We don't need to turn off start-stop GC. We also don't want to use @llvm.compiler.used, because we really do want the retain flag.

The reflection code is where we might care about the unretained sections, so readELFSections() in ReflectionContext.h needs changing to look for both retained and unretained sections. The easiest solution here seems to be to make the compiler generate a different section name for the non-retained sections, which will mean that all the linkers will behave the same way if you link retained and unretained reflection metadata into the same program. Then we can just make readELFSections() add an extra ReflectionInfo if it finds unretained sections.

That should fix everything I think. I've got a changeset for all of this and I'm currently trying the test suite with

SWIFT_DRIVER_TEST_OPTIONS=" -Xlinker --gc-sections -use-ld=lld"
SWIFT_DRIVER_TEST_OPTIONS=" -Xlinker --gc-sections -use-ld=gold"

as @kateinoigakukun did previously to make sure I haven't broken anything.

[al45tair]

I have a new PR that deals with all of this: #72061.

[kateinoigakukun]

Great!