[PATCH] D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/include/llvm/MC/MCContext.h:335
+    // sections (e.g. via _attribute_((section("myname")))).
+    std::set<StringRef> ELFSeenGenericMergeableSections;
+
----------------
bd1976llvm wrote:
> MaskRay wrote:
> > MaskRay wrote:
> > > DenseSet
> > MCContext::renameELFSection is only used by GNU-style .zdebug_* (GNU-style is obsoleted by SHF_COMPRESSED).
> > 
> > I think we should not add code just to make .zdebug_* happy. Can we simplify code if .zdebug_str is not a big issue?
> Thanks for pointing this out. Sorry for the slow response - I had COVID-19 and zlib problems to contend with before I could get back to you about this :)
> 
> .debug_str is mergeable and MC can produce .zdebug_str.
> 
> I'm not sure what reasonable behaviour should be with GNU style debug section compression. Two cases I have thought about are:
> 
> 1. If a compatible symbol has a n explicit section of .debug_str.
> 2. If a compatible symbol has an explicit section of .zdebug_str.
> 
> I think 1. probably should result in the symbol appearing in .zdebug_str in the output since the section compression can be thought of as a post processing step. I think that 2. should probably be an error.
> 
> I don't really want to add extra logic and more LIT testing to support this if this is a corner case that no one will actually ever hit.
> 
> I would be happy to remove the code from MCContext::renameELFSection and leave the interaction with GNU-style debug section compression as a later enhancement to be added if needed.
> 
> What do you think?
> I don't really want to add extra logic and more LIT testing to support this if this is a corner case that no one will actually ever hit.

Agree. That was why I asked whether we can simplify code and tests if .zdebug_* support can be dropped.

.zdebug* is an obsolete feature that may even be deleted in a future lld..


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

https://reviews.llvm.org/D72194