[ELF] Merge sections with different access attributes

[evgeny777]

Summary

There was a long story here of merging output sections when linker script is used and reverting it back to original scheme with multiple output sections having the same name. Now when we returned to original scheme, I'm getting troubles linking boot loader, where all code and data should be packed to a single section and then extracted from result image using objcopy.

To deal with this problem, I suggest dropping section access attributes from output section key and merging them in addSection(). This solves my problem and doesn't break unit tests, except repsection-va.s, which is part of linker script processor test suite.

I'd like to add that such behavior conforms to GNU linkers. Both gold and ld merge section attributes in such case.

[ruiu]

As you know we had lots of discussions whether we should merge them or separate them. After several and forth commits, we are currently not merging sections with different attributes. When the last patch was submitted, the justification of doing it was to handle mergeable sections and other sections properly and also because keeping section writable/executable properties is a good thing. This patch revert that behavior. How would you handle the problems addressed by the last commit?

[evgeny777]

AFAIK, the main problem was linker script putting mergeable and non-mergeable to the same output section. This is really bad and should be avoided. However we're not living in ideal world and linker scripts putting RX and RW input sections to the same output section do exist. Things get worse when you use assignments:

.mysec : {
   *(.text)
   *(.data)
   . = ALIGN(0x1000);
}

Needless to say that lld will align second .mysec section, leaving the first one as is. Currently the only way lld allows putting code and data together is using PHDRS and single PT_LOAD for both. However nothing can be done to handle the case above correctly.

May be it makes sense to enable R/W/X merging only for linker script case for now?

[evgeny777]

Done here:
https://reviews.llvm.org/D24576

ELF/OutputSections.h

public:
   uintX_t getFileOff() const { return Header.sh_offset; }
   uintX_t getAlignment() const { return Header.sh_addralign; }
   uint32_t getType() const { return Header.sh_type; }
 
   void updateAlignment(uintX_t Alignment) {
     if (Alignment > Header.sh_addralign)
       Header.sh_addralign = Alignment;
   }
+  void updateFlags(InputSectionBase<ELFT> *C);
 
   // If true, this section will be page aligned on disk.
   // Typically the first section of each PT_LOAD segment has this flag.
   bool PageAlign = false;
 
   virtual void finalize() {}
   virtual void finalizePieces() {}
   virtual void assignOffsets() {}

ELF/OutputSections.cpp

 }
 
 template <class ELFT>
 void OutputSectionBase<ELFT>::writeHeaderTo(Elf_Shdr *Shdr) {
   *Shdr = Header;
 }
 
 template <class ELFT>
+void OutputSectionBase<ELFT>::updateFlags(InputSectionBase<ELFT> *C) {
+  this->Header.sh_flags |=
+      C->getSectionHdr()->sh_flags & (SHF_EXECINSTR | SHF_WRITE);
+}
+
+template <class ELFT>
 GotPltSection<ELFT>::GotPltSection()
     : OutputSectionBase<ELFT>(".got.plt", SHT_PROGBITS, SHF_ALLOC | SHF_WRITE) {
   this->Header.sh_addralign = Target->GotPltEntrySize;
 }
 
 template <class ELFT> void GotPltSection<ELFT>::addEntry(SymbolBody &Sym) {
   Sym.GotPltIndex = Target->GotPltHeaderEntriesNum + Entries.size();
   Entries.push_back(&Sym);

 
 template <class ELFT>
 void OutputSection<ELFT>::addSection(InputSectionBase<ELFT> *C) {
   assert(C->Live);
   auto *S = cast<InputSection<ELFT>>(C);
   Sections.push_back(S);
   S->OutSec = this;
   this->updateAlignment(S->Alignment);
+  this->updateFlags(C);
 }
 
 // If an input string is in the form of "foo.N" where N is a number,
 // return N. Otherwise, returns 65536, which is one greater than the
 // lowest priority.
 static int getPriority(StringRef S) {
   size_t Pos = S.rfind('.');
   if (Pos == StringRef::npos)
static StringRef toStringRef(ArrayRef<uint8_t> A) {
   return {(const char *)A.data(), A.size()};
 }
 
 template <class ELFT>
 void MergeOutputSection<ELFT>::addSection(InputSectionBase<ELFT> *C) {
   auto *Sec = cast<MergeInputSection<ELFT>>(C);
   Sec->OutSec = this;
   this->updateAlignment(Sec->Alignment);
+  this->updateFlags(C);
   this->Header.sh_entsize = Sec->getSectionHdr()->sh_entsize;
   Sections.push_back(Sec);
 
   bool IsString = this->Header.sh_flags & SHF_STRINGS;
 
   for (SectionPiece &Piece : Sec->Pieces) {
     if (!Piece.Live)
       continue;
OutputSectionFactory<ELFT>::create(InputSectionBase<ELFT> *C,
   return {Sec, true};
 }
 
 template <class ELFT>
 SectionKey<ELFT::Is64Bits>
 OutputSectionFactory<ELFT>::createKey(InputSectionBase<ELFT> *C,
                                       StringRef OutsecName) {
   const Elf_Shdr *H = C->getSectionHdr();
-  uintX_t Flags = H->sh_flags & ~SHF_GROUP & ~SHF_COMPRESSED;
+  uintX_t Flags =
+      H->sh_flags & ~(SHF_GROUP | SHF_COMPRESSED | SHF_EXECINSTR | SHF_WRITE);
 
   // For SHF_MERGE we create different output sections for each alignment.
   // This makes each output section simple and keeps a single level mapping from
   // input to output.
   uintX_t Alignment = 0;
   if (isa<MergeInputSection<ELFT>>(C))
     Alignment = std::max(H->sh_addralign, H->sh_entsize);
 

test/ELF/linkerscript/repsection-va.s

 # REQUIRES: x86
 # RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t
 
 # RUN: echo "SECTIONS {.foo : {*(.foo.*)} }" > %t.script
 # RUN: ld.lld -o %t1 --script %t.script %t
 # RUN: llvm-objdump -section-headers %t1 | FileCheck %s
 # CHECK:      Sections:
 # CHECK-NEXT: Idx Name          Size      Address          Type
 # CHECK-NEXT:   0               00000000 0000000000000000
-# CHECK-NEXT:   1 .foo          00000004 0000000000000158 DATA
-# CHECK-NEXT:   2 .foo          00000004 000000000000015c DATA
-# CHECK-NEXT:   3 .text         00000001 0000000000000160 TEXT DATA
+# CHECK-NEXT:   1 .foo          00000008 0000000000000158 DATA
+# CHECK-NEXT:   2 .text         00000001 0000000000000160 TEXT DATA
 
 .global _start
 _start:
  nop
 
 .section .foo.1,"a"
 foo1:
  .long 0
 
 .section .foo.2,"aw"
 foo2:
  .long 0