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

[Lang Hames via Phabricator via llvm-commits]

lhames added inline comments.


================
Comment at: llvm/unittests/ExecutionEngine/Orc/RTDyldObjectLinkingLayerTest.cpp:81
+  GV->setUnnamedAddr(GlobalValue::UnnamedAddr::Global);
+  GV->setAlignment(1);
 
----------------
nickdesaulniers wrote:
> 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 ).
This test just needed to put something into a debug section so that the section would be forced though the JIT memory manager. No care was taken with types. ;)

This fix looks good to me.


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

https://reviews.llvm.org/D72194