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

> Having thought about it I am happy with this being the full fix

Me too. Please amend the patch description above to say so, rather than *partial fix*.  Also, please address the two comments I've "bumped."



================
Comment at: llvm/include/llvm/CodeGen/TargetLoweringObjectFileImpl.h:17
+
+#include "llvm/BinaryFormat/ELF.h"
 #include "llvm/BinaryFormat/XCOFF.h"
----------------
nickdesaulniers wrote:
> probably should include `llvm/ADT/StringRef.h`, too. IWYU.
bump.


================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:623
+  assert (((Section->getEntrySize() == 0) ||
+      (Section->getEntrySize() == getEntrySizeForKind(Kind))));
+
----------------
nickdesaulniers wrote:
> 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.
bump


================
Comment at: llvm/unittests/ExecutionEngine/Orc/RTDyldObjectLinkingLayerTest.cpp:81
+  GV->setUnnamedAddr(GlobalValue::UnnamedAddr::Global);
+  GV->setAlignment(1);
 
----------------
bd1976llvm wrote:
> nickdesaulniers wrote:
> > Are the ORC test changes related to this change?
> Yes. In the original test the user has explicitly assigned a 32bit integer into the .debug_str section (GV->setSection(".debug_str")). The .debug_str section is a special section reserved for the system with fixed properties. MC creates a .debug_str section internally before looking at any of the globals as a mergeable section containing ASCII strings. The attempt to assign the integer foo to .debug_str ends up creating two .debug_str sections with the second one being a mergeable section with its entsize compatible with 32bit integers with the patch.
> 
> I traced the history of the test and I believe that using an integer is simply a mistake and that the original unit test author should have put strings into .debug_str. So, I updated the unit tests to do that.
Thanks for the context, please add it to the patch description above so that others have context when looking at `git log`. It may also be worthwhile to subscribe the tests' authors to this change, which I've done (@lhames ).


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

https://reviews.llvm.org/D72194