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

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added a comment.

Thanks for the extensive tests.  I might triple check the entity sizes to make sure they're correct, but the tests look pretty exhaustive to me and make sense.



================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:623
+  assert (((Section->getEntrySize() == 0) ||
+      (Section->getEntrySize() == getEntrySizeForKind(Kind))));
+
----------------
rjmccall wrote:
> Is this assertion intentionally allowing `Section` to be non-mergeable?  I feel like this probably should guarantee to return a mergeable section if that's what was requested.
Right, shouldn't this be:
```
if (Flags & ELF::SHF_MERGE) {
  assert((Section->getFlags() & ELF::SHF_MERGE) &&
        "Merge-able section requested but not produced");
  assert(Section->getEntrySize() == getEntrySizeForKind(Kind) &&
        "Mismatched merge-able section entity sizes");
}
```
?

Also, it might be nice to hoist the multiple calls to `getEntrySizeForKind(Kind)` into one variable rather than have 3 calls at different scopes.


================
Comment at: llvm/unittests/ExecutionEngine/Orc/RTDyldObjectLinkingLayerTest.cpp:81
+  GV->setUnnamedAddr(GlobalValue::UnnamedAddr::Global);
+  GV->setAlignment(1);
 
----------------
Are the ORC test changes related to this change?


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

https://reviews.llvm.org/D72194