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

[Aaron Hill via Phabricator via llvm-commits]

Aaron1011 added a comment.

> 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.

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 main concern with option 3 is that it doesn't fix the underlying issue with COMDAT-related optimizations. If COMDATs can't be relied on to keep their group members together, their only possible use is for de-duplication purposes.

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.


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

https://reviews.llvm.org/D53234