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

[ben via Phabricator via llvm-commits]

bd1976llvm added inline comments.


================
Comment at: llvm/include/llvm/MC/MCContext.h:335
+    // sections (e.g. via _attribute_((section("myname")))).
+    std::set<StringRef> ELFSeenGenericMergeableSections;
+
----------------
MaskRay wrote:
> 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..
I have removed the code. No tests simplification as the changes to MCContext::renameELFSection were not being exercised.


================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:587
+  return ".data.rel.ro";
+}
+
----------------
nickdesaulniers wrote:
> is this section prefixing specific to ELF, or common between all object file formats? Should the function name contain `ELF` if specific to ELF?
> 
> Is there a more specific guard we can use for `".data.rel.ro"`, returning an empty string otherwise?
> is this section prefixing specific to ELF, or common between all object file formats? Should the function name contain ELF if specific to ELF?

This function is also used by web assembly which is why it doesn't have ELF in the name.

> Is there a more specific guard we can use for ".data.rel.ro", returning an empty string otherwise?

An llvm_unreachable would make this code clearer. I'll add one.




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

https://reviews.llvm.org/D72194