[PATCH] D53234: Don't remove COMDATs when internalizing global objects

[ben via Phabricator via llvm-commits]

bd1976llvm added a comment.

In D53234#1330686 <https://reviews.llvm.org/D53234#1330686>, @Aaron1011 wrote:

> > We need to resolve this point of disagreement. IMO that optimization is only invalid in regular compilation. I think that for LTO this is a valid optimization. LTO has optimizations that would normally be invalid - for example the Internalize pass.
>
> I think the issue here is that an LLVM COMDAT has two different purposes:
>
> 1. De-duplicate section groups
> 2. Ensure that section group members are kept/removed as a unit
>
>   In ELF files, you can have non-COMDAT section groups, which only accomplish purpose 2.


Actually, I don't think that ELF has non-COMDAT groups. AFAIK COMDATs are the only, currently, legal type of group.

> The way that COMDATS are currently used within LLVM seems to suggest that keeping group members together is an important purpose.
>  For example, AddressSanitizer <https://github.com/llvm-mirror/llvm/blob/ee2b007283006695772056191277b331717a08d3/lib/Transforms/Instrumentation/AddressSanitizer.cpp#L1831-L1832> uses COMDATs to keep groups together, and explicitly tries to //prevent// the COMDAT name from duplicating another name. In this case, the de-duplication feature of COMDATS is completely irrelevant - a COMDAT is only being used to keep an instrumented function and its associated global together.
> 
>> AFAIK !associated metadata sections are not required to be in a COMDAT
> 
> The language ref <https://llvm.org/docs/LangRef.html#associated-metadata> states that globals with !associated metadata should be in a COMDAT "for best compatibility".
>  While that's not the same as requiring it, all LLVM passes which insert !associated metadata (e.g. AddressSanitizer, SanitizerCoverage) always create/use a COMDAT.

My reading of "for best compatibility" that is simply that system linker support for "associated" sections might not be implemented (actually, I think that this advice is becoming increasingly irrelevant). We shouldn't rely on this"best compatibility" guideline for correctness.

> My main concern with option 3 is that it doesn't fix the underlying issue with COMDAT-related optimizations.

I don't think that there are any other problems with COMDAT-related optimizations; or, at least I don't think that there are any that can't be fixed without changing the current handling of COMDATs.

> If COMDATs can't be relied on to keep their group members together, their only possible use is for de-duplication purposes.

They can be relied on to keep group memebers together "for as long as necessary". They also provide for group de-duplication.

> Given the existing uses in AddressSanitizer and CoversageSanitizer, I think it makes the most sense for LLVM to never delete COMDATS in optimization passes. In the worst case, we miss an (ambiguous) optimization opportunity - i.e. we don't delete an individual COMDAT member when it might have been legal to do so. However, the current behavior (deleting individual COMDAT members) leads to behavior that is at best surprising, and at worst a violation of the ELF spec.

While this strategy works, it would go against the mental model of developers working on LLVM - I expect LTO optimizations to be able to remove COMDATs. Given the current use cases I don't think that it is a problem to implement option 3. Option 3 also has the benefit that we can add specific fixes to the code that is creating the "associated metadata" (with appropriate comments) which is easy for a developer to understand. Adjusting the global behavior of LLVM w.r.t COMDATs to work around issues with "associated metadata" is much more complicated.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53234/new/

https://reviews.llvm.org/D53234