[ELF] - Issue an error if trying to link object that uses splitstacks.

[grimar]

Summary

Each object file compiled in split stack mode will have an empty
section with a special name: .note.GNU-split-stack

We don't support this in linker now, so patch just adds an error out for that.

[grimar]

You want to also keep ".note.GNU-no-split-stack", no?

Thank you,
Filipe

This patch is just about producing an error (when linking with -r) if one object
has ".note.GNU-split-stack?" and the other does not.
If file have ".note.GNU-no-split-stack" then it will also have ".note.GNU-split-stack",
so implemented logic is correct here I believe.

Generally I am not sure how much splitstack feature is important, so I just want to stop generating currupted
output. And if we will want to have a full support for splitstacks, thats is for another patches anyways.

George.

[grimar]

You want to also keep ".note.GNU-no-split-stack", no?

Thank you,

Filipe

I think I got what you mean, I am not proccessing the ".note.GNU-no-split-stack" at all. So it should not be discarded now then anyways.

George.

[ruiu]

This patch covers a tricky use case. Is this actually needed? I mean if you want to do something for the split stack, why don't you support it in the linker instead of rejecting it?

Even if you want reject it, this patch seems a bit too much. There is a simpler way of doing the same thing. InputFiles and Sections can be agnostic of the split stack. Instead, it can be done using a few non-member function as shown below.

template <class ELFT>
static bool hasSplitStack(const std::unique_ptr<ObjectFile<ELFT>> &File) {
  for (InputSectionBase<ELFT> *Sec : File->getSections())
    if (auto *S = dyn_cast_or_null<InputSection<ELFT>>(Sec))
      if (S->getSectionName() == ".note.GNU-split-stack")
        return true;
  return false;
}

template <class ELFT>
static void checkSplitSections(SymbolTable<ELFT> &Symtab) {
  if (!Config->Relocatable)
    return;
  const std::vector<std::unique_ptr<ObjectFile<ELFT>>> &Files
      = Symtab.getObjectFiles();
  bool HasSplitStack = hasSplitStack<ELFT>(Files.front());
  auto Fn = [=](const std::unique_ptr<ObjectFile<ELFT>> &Obj) {
    return hasSplitStack<ELFT>(Obj) == HasSplitStack;
  };
  if (!std::all_of(Files.begin(), Files.end(), Fn))
    error("You cannot mix object files compiled with -fsplit-stack"
          " and -fno-split-stack");
}

(I'm not arguing that this is what I want because I'm not sure if this feature should be in, but if it is, it can be simplified.)

[grimar]

In D17918#368733, @ruiu wrote:

This patch covers a tricky use case. Is this actually needed? I mean if you want to do something for the split stack, why don't you support it in the linker instead of rejecting it?

I am not sure how many people are using splitstacks (feature itself looks interesting for me though).
If you think it is reasonable to support it in lld - I can try to do that (and that patch is anyways will be required for -r then).

Leaving behind known silently broken output is anyways bad.
But you right that use case is a but tricky, so in case we are not planning to support splitstacks we probably can simplify the solution:
just error out if we see any splitstack note in files.

Actually main point of this patch was that I prepared a serie of patches last days that had aim to improve the -r support.
That was the last one possible major issue I knew about about -r.
It is mentioned in "implementation steps" for -r here https://gcc.gnu.org/wiki/SplitStacks.

George.

[filcab]

In D17918#368701, @grimar wrote:

You want to also keep ".note.GNU-no-split-stack", no?

Thank you,

Filipe

I think I got what you mean, I am not proccessing the ".note.GNU-no-split-stack" at all. So it should not be discarded now then anyways.

George.

It should still get the same type of handling as GNU-split-stack, though. I see no reason to handle one (even if "handle" is discarding if the output is not relocatable) but not the other.

But I think the latter messages have a better idea: Simply rejecting split-stack objects (unless you plan on adding the rest of the support, including changing function prologues) seems like the most appropriate thing to do right now.

[grimar]

It should still get the same type of handling as GNU-split-stack, though. I see no reason to handle one (even if "handle" is discarding if the output is not relocatable) but not the other.

Thats because -r does not need to handle it. We should handle the other one for completeness, but that is not relative to this patch, so should be done separatelly.

But I think the latter messages have a better idea: Simply rejecting split-stack objects (unless you plan on adding the rest of the support, including changing function prologues) seems like the >most appropriate thing to do right now.

So if we are not going to support splitstacks, I would also vote for this. (I can just update this patch, will cut excessive logic and update the tests, probably will also add few testcases).

But if it is reasonable to have splitstacks, then I can implement this feature I think.

Rui, Rafael, what do you think about this ?

George.

[grimar]

• Removed most of logic, lld should just produce error if splitstacks are used.

ELF/InputFiles.cpp

179

That's not actually needed for this patch to work, but it seems reasonable to have ?

[ruiu]

ELF/InputFiles.cpp

179

I wouldn't add it.

248

I don't think this comment describes anything beyond what the code does, so please remove it.

test/ELF/splitstacks.s

5–6 ↗

(On Diff #50128)

You can write in one line. Remove --check-prefix=ERR.

7 ↗

(On Diff #50128)

ERR -> CHECK

[grimar]

• Addressed review comments.

[ruiu]

LGTM

test/ELF/splitstacks.s

4 ↗

(On Diff #50154)

I'd remove this comment because it's obvious.

ELF/InputFiles.h

public:
   // used to create the relocatable object and required to support
   // R_MIPS_GPREL16 / R_MIPS_GPREL32 relocations.
   uint32_t getMipsGp0() const;
 
   // The number is the offset in the string table. It will be used as the
   // st_name of the symbol.
   std::vector<std::pair<const Elf_Sym *, unsigned>> KeptLocalSyms;
 
+  // If an object file compiled in split stack mode.
+  bool SplitStacks = false;
+
 private:
   void initializeSections(llvm::DenseSet<StringRef> &ComdatGroups);
   void initializeSymbols();
   InputSectionBase<ELFT> *createInputSection(const Elf_Shdr &Sec);
 
   SymbolBody *createSymbolBody(const Elf_Sym *Sym);
 
   // List of all sections defined by this file.

ELF/InputFiles.cpp

 void elf::ObjectFile<ELFT>::initializeSections(
     DenseSet<StringRef> &ComdatGroups) {
   uint64_t Size = this->ELFObj.getNumSections();
   Sections.resize(Size);
   unsigned I = -1;
   const ELFFile<ELFT> &Obj = this->ELFObj;
   for (const Elf_Shdr &Sec : Obj.sections()) {
     ++I;
     if (Sections[I] == InputSection<ELFT>::Discarded)
       continue;
 
     switch (Sec.sh_type) {
     case SHT_GROUP:
       Sections[I] = InputSection<ELFT>::Discarded;
       if (ComdatGroups.insert(getShtGroupSignature(Sec)).second)
         continue;
       for (uint32_t SecIndex : getShtGroupEntries(Sec)) {
elf::ObjectFile<ELFT>::createInputSection(const Elf_Shdr &Sec) {
 
   // .note.GNU-stack is a marker section to control the presence of
   // PT_GNU_STACK segment in outputs. Since the presence of the segment
   // is controlled only by the command line option (-z execstack) in LLD,
   // .note.GNU-stack is ignored.
   if (Name == ".note.GNU-stack")
     return InputSection<ELFT>::Discarded;
 
+  // .note.GNU-split-stack is a marker section that used if object file compiled
+  // in split stack mode. If we are not generating relocatable output, then
+  // such section can be discarded.
+  if (Name == ".note.GNU-split-stack") {
+    SplitStacks = true;
+    if (!Config->Relocatable)
+      return InputSection<ELFT>::Discarded;
+  }
+
   // A MIPS object file has a special section that contains register
   // usage info, which needs to be handled by the linker specially.
   if (Config->EMachine == EM_MIPS && Name == ".reginfo") {
     MipsReginfo = new (Alloc) MipsReginfoInputSection<ELFT>(this, &Sec);
     return MipsReginfo;
   }
 
   // We dont need special handling of .eh_frame sections if relocatable

ELF/SymbolTable.cpp

 using namespace llvm::ELF;
 
 using namespace lld;
 using namespace lld::elf;
 
 // All input object files must be for the same architecture
 // (e.g. it does not make sense to link x86 object files with
 // MIPS object files.) This function checks for that error.
-template <class ELFT> static bool isCompatible(InputFile *FileP) {
+template <class ELFT> static bool isCompatibleArch(InputFile *FileP) {
   auto *F = dyn_cast<ELFFileBase<ELFT>>(FileP);
   if (!F)
     return true;
   if (F->getELFKind() == Config->EKind && F->getEMachine() == Config->EMachine)
     return true;
   StringRef A = F->getName();
   StringRef B = Config->Emulation;
   if (B.empty())
     B = Config->FirstElf->getName();
   error(A + " is incompatible with " + B);
   return false;
 }
 
+// If relocatable output (-r) is used to link an object with the split-stack
+// note with an object without the split-stack note, issue an error.
+// https://gcc.gnu.org/wiki/SplitStacks
+template <class ELFT, class ObjList>
+static bool isCompatibleObj(ObjectFile<ELFT> *O, ObjList &Files) {
+  if (!Config->Relocatable)
+    return true;
+  bool Comp = Files.front()->SplitStacks == O->SplitStacks;
+  if (!Comp)
+    error("when using -r, cannot mix split-stack vs non-split-stack: " +
+          Files.front()->getName() + " " + O->getName());
+  return Comp;
+}
+
 // Add symbols in File to the symbol table.
 template <class ELFT>
 void SymbolTable<ELFT>::addFile(std::unique_ptr<InputFile> File) {
   InputFile *FileP = File.get();
-  if (!isCompatible<ELFT>(FileP))
+  if (!isCompatibleArch<ELFT>(FileP))
     return;
 
   // .a file
   if (auto *F = dyn_cast<ArchiveFile>(FileP)) {
     ArchiveFiles.emplace_back(cast<ArchiveFile>(File.release()));
     F->parse();
     for (Lazy &Sym : F->getLazySymbols())
       addLazy(&Sym);
for (SymbolBody *B : F->getSymbols())
       resolve(B);
     return;
   }
 
   // .o file
   auto *F = cast<ObjectFile<ELFT>>(FileP);
   ObjectFiles.emplace_back(cast<ObjectFile<ELFT>>(File.release()));
   F->parse(ComdatGroups);
+  if (!isCompatibleObj<ELFT>(F, ObjectFiles))
+    return;
   for (SymbolBody *B : F->getSymbols())
     resolve(B);
 }
 
 // Codegen the module M and returns the resulting InputFile.
 template <class ELFT>
 std::unique_ptr<InputFile> SymbolTable<ELFT>::codegen(Module &M) {
   StringRef TripleStr = M.getTargetTriple();

test/ELF/relocatable-splitstacks.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t1.o
+# RUN: echo "" > %t.s
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %t.s -o %t2.o
+
+## Check that when generating relocatable output we should not be
+## able to mix the files with different splitstacks mode.
+# RUN: not ld.lld -r %t1.o %t2.o -o %t 2>&1 | \
+# RUN:    FileCheck --check-prefix=MIX %s
+# MIX: when using -r, cannot mix split-stack vs non-split-stack:
+
+## Check that we are able to generate relocatable output when
+## linking files with the same splitstacks mode.
+# RUN: ld.lld -r %t2.o %t2.o -o %t2 2>&1 
+# RUN: llvm-readobj %t2 > /dev/null
+
+## Check that .note.GNU-split-stack is not discarded when using -r
+# RUN: ld.lld -r %t1.o -o %t3 
+# RUN: llvm-readobj -s %t3 | FileCheck %s --check-prefix=KEEPNOTE
+# KEEPNOTE: Sections [
+# KEEPNOTE: Name: .note.GNU-split-stack
+
+## Check that .note.GNU-split-stack is discarded when not using -r
+# RUN: ld.lld %t1.o -o %t4
+# RUN: llvm-readobj -s %t4 | FileCheck %s --check-prefix=REMOVENOTE
+# REMOVENOTE:     Sections [
+# REMOVENOTE-NOT: Name: .note.GNU-split-stack
+
+.globl _start
+_start:
+ nop
+
+.section .note.GNU-split-stack,"",@progbits