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

[Lang Hames via Phabricator via llvm-commits]

lhames 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;
----------------
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.


================
Comment at: llvm/unittests/ExecutionEngine/Orc/RTDyldObjectLinkingLayerTest.cpp:74
   Constant *StrConstant = ConstantDataArray::getString(Context, "forty-two");
   auto *GV =
       new GlobalVariable(*M, StrConstant->getType(), true,
----------------
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.



================
Comment at: llvm/unittests/ExecutionEngine/Orc/RTDyldObjectLinkingLayerTest.cpp:92
       MemoryBuffer::getMemBufferCopy(Obj->getBuffer()), false))
       << "Debug section seen despite ProcessAllSections being false";
   EXPECT_TRUE(testSetProcessAllSections(std::move(Obj), true))
----------------
peter.smith wrote:
> I think this should be non alloc section instead of Debug. Possibly use .note.GNU-stack instead of non alloc.
Agreed.


================
Comment at: llvm/unittests/ExecutionEngine/Orc/RTDyldObjectLinkingLayerTest.cpp:94
   EXPECT_TRUE(testSetProcessAllSections(std::move(Obj), true))
       << "Expected to see debug section when ProcessAllSections is true";
 }
----------------
peter.smith wrote:
> Same here, debug -> non alloc or even .note.GNU-stack
Agreed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100944