[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 ↗

(On Diff #50128)

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

[ruiu]

ELF/InputFiles.cpp

179 ↗

(On Diff #50128)

I wouldn't add it.

250 ↗

(On Diff #50128)

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.

lld/trunk/ELF/InputFiles.cpp

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;
 
+  if (Name == ".note.GNU-split-stack")
+    error("Objects using splitstacks are not supported");
+
   // 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

lld/trunk/test/ELF/splitstacks.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t1.o
+
+# RUN: not ld.lld %t1.o -o %t 2>&1 | FileCheck %s
+# CHECK: Objects using splitstacks are not supported
+
+.globl _start
+_start:
+ nop
+
+.section .note.GNU-split-stack,"",@progbits