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

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

Thanks very much for working on this. I agree with the approach of putting out a separate section when the flags don't match as this matches the non-lto behaviour when the files are compiled separately.

I've left a few thoughts on the RTDyldObjectLinkingLayerTest.cpp test as I think that there may be some assertions that need updating. I think it is reasonable to use a different non-alloc section assuming the purpose of the test is to check that ProcessAllSections controls whether non alloc sections are considered.

@MaskRay do you have any thoughts?
@bd1976llvm did the changes to explicit-section-mergeable.ll match your expectations?

I checked what GCC (10.1.1) does for the LTO case. Looking at the output of --save-temps the GCC LTO code-generator/assembler has merged the section flags prior to passing to the linker. So the linker sees a RW foo with both var1 and var2. This is definitely better than a RO foo which is incorrect.



================
Comment at: llvm/test/CodeGen/Generic/elf-unique-sections-by-flags.ll:84
+
+; Explicitly placed in .rodata. The writable section should be uniqued, not the default ro section, even if it comes first.
+ at rw_rodata = global [2 x i32] zeroinitializer, section ".rodata", align 4
----------------
A small suggestion to emphasise that .rodata has been given an unconventional flag.
; Explicitly placed in a .rodata with writeable set. The writable section should be uniqued, not the default ro section, even if it comes first.


================
Comment at: llvm/test/CodeGen/Generic/elf-unique-sections-by-flags.ll:104
+
+; Multiple .text sections are emitted
+ at rw_text = global i32 10, section ".text", align 4
----------------
Suggest
; Multiple .text sections are emitted for read-only and read-write sections using .text name.


================
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;
----------------
Worth a comment as to why .note.GNU-stack was chosen? Presumably an arbitrary non-alloc default section.


================
Comment at: llvm/unittests/ExecutionEngine/Orc/RTDyldObjectLinkingLayerTest.cpp:74
   Constant *StrConstant = ConstantDataArray::getString(Context, "forty-two");
   auto *GV =
       new GlobalVariable(*M, StrConstant->getType(), true,
----------------
Now that GV->setSection(".debug_str") isn't being called where is it going? Is it a necessary part of the test?


================
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))
----------------
I think this should be non alloc section instead of Debug. Possibly use .note.GNU-stack instead of non alloc.


================
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";
 }
----------------
Same here, debug -> non alloc or even .note.GNU-stack


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100944