[PATCH] D100944: [MC][ELF] Emit separate unique sections for different flags

[Tomas Matheson via Phabricator via llvm-commits]

tmatheson marked 9 inline comments as done.
tmatheson added inline comments.


================
Comment at: llvm/unittests/ExecutionEngine/Orc/RTDyldObjectLinkingLayerTest.cpp:33
                                  bool IsReadOnly) override {
-      if (SectionName == ".debug_str")
-        DebugSeen = true;
+      if (SectionName == ".note.GNU-stack")
+        NonAllocSeen = true;
----------------
lhames wrote:
> peter.smith wrote:
> > Worth a comment as to why .note.GNU-stack was chosen? Presumably an arbitrary non-alloc default section.
> Yes -- that would be a helpful comment. I think your presumption is correct though.
I've added a comment. `.not.GNU-stack` is the only non-alloc section in the module, at least at the point where `ProcessAllSections` is applied.


================
Comment at: llvm/unittests/ExecutionEngine/Orc/RTDyldObjectLinkingLayerTest.cpp:74
   Constant *StrConstant = ConstantDataArray::getString(Context, "forty-two");
   auto *GV =
       new GlobalVariable(*M, StrConstant->getType(), true,
----------------
lhames wrote:
> peter.smith wrote:
> > Now that GV->setSection(".debug_str") isn't being called where is it going? Is it a necessary part of the test?
> I guess that setSection(".debug_str") isn't meaningful any more. I was using it in this test because MC always treated ".debug_str" as non-alloc, but it looks like this patch changes that.
> 
> You might be able to remove StrConstant and GV entirely, but I don't know how MC treats empty modules -- do they still reliably generate `.note.GNU-stack` sections?
> 
> If removing StrConstant and GV works then that's great. If removing them causes problems for the test then they can be left as is (or you can substitute some other definition) with a comment that they're just there so that the module is non-empty.
> 
I've added a comment. I had tried to remove them but the test failed much earlier with an empty module.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100944