[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 marked 5 inline comments as done.
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:
> 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?


================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:605
+    Name.push_back('.');
+    TM.getNameWithPrefix(Name, GO, Mang, true /*MayAlwaysUsePrivate*/);
+  }
----------------
MaskRay wrote:
> MaskRay wrote:
> > `/*MayAlwaysUsePrivate=*/true`
> Not done yet.
Sorry - lost the change rebasing somehow,


================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:737
 
-  SmallString<128> Name;
-  if (Kind.isMergeableCString()) {
-    // We also need alignment here.
-    // FIXME: this is getting the alignment of the character, not the
-    // alignment of the global!
-    unsigned Align = GO->getParent()->getDataLayout().getPreferredAlignment(
-        cast<GlobalVariable>(GO));
-
-    std::string SizeSpec = ".rodata.str" + utostr(EntrySize) + ".";
-    Name = SizeSpec + utostr(Align);
-  } else if (Kind.isMergeableConst()) {
-    Name = ".rodata.cst";
-    Name += utostr(EntrySize);
-  } else {
-    Name = getSectionPrefixForGlobal(Kind);
-  }
-
-  if (const auto *F = dyn_cast<Function>(GO)) {
-    const auto &OptionalPrefix = F->getSectionPrefix();
-    if (OptionalPrefix)
-      Name += *OptionalPrefix;
-  }
-
-  unsigned UniqueID = MCContext::GenericSectionID;
-  if (EmitUniqueSection) {
-    if (TM.getUniqueSectionNames()) {
-      Name.push_back('.');
-      TM.getNameWithPrefix(Name, GO, Mang, true /*MayAlwaysUsePrivate*/);
-    } else {
-      UniqueID = *NextUniqueID;
-      (*NextUniqueID)++;
+    bool UniqueSectionName = false;
+    unsigned UniqueID = MCContext::GenericSectionID;
----------------
MaskRay wrote:
> Accidentally indented.
Thanks!


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

https://reviews.llvm.org/D72194