Update split stack support to handle more generic prologues. Improve error handling. Add test file for better code-coverage. Update tests to be more complete.

[saugustine]

Summary

Address various shortcomings of current split-stack implementation.

[saugustine]

ruiu, do you have any thoughts on this?

[ruiu]

ELF/Arch/X86_64.cpp

501

nit: remove the empty line.

ELF/InputSection.cpp

890

Instead of adding another Set, you can just add a symbol with an error as if there was no error occurred to suppress further errors, no?

[grimar]

ELF/Arch/X86_64.cpp

490

It seems now you can do check early:

if (Loc + 8 >= End)
  return;

// Replace "cmp %fs:0x70,%rsp" and subsequent branch
// with "stc, nopl 0x0(%rax,%rax,1)"
...

test/ELF/x86-64-split-stack-prologue-adjust-fail.s

9

Could you check the whole error message here? (including filenames and symbol names you are printing).

[saugustine]

Update for comments.

Updating D49926: Update split stack support to handle more generic prologues.

Improve error handling.
Add test file for better code-coverage. Update tests to be more
complete.

[saugustine]

Thanks for the reviews. I think I've addressed them.

[grimar]

ELF/InputSection.cpp

843

Something is wrong here. You are reporting __more_stack_nonsplit symbol name (and in the test cases you are using the same name),
but trying to find the __morestack_non_split in the symbol table.

911

I think we usually write like this: auto *IS =

test/ELF/x86-64-split-stack-prologue-adjust-fail.s

9

I think we do not rely on the test file name and usually use regexp for checking, example:

# CHECK: error: {{.*}}.o:(.zdebug_str): decompress failed:

14

Do you need the --defsym here?

I would also rename UNDEFINED_MORE_STACK_NON_SPLIT so something a bit shorter. Maybe just ERROR?

[saugustine]

Thanks again. Hopefully this is the last round, but happy to make other changes.

test/ELF/x86-64-split-stack-prologue-adjust-fail.s

14

Don't need the defsym, because this test is testing what happens when it has been omitted.

Renamed to just ERROR.

[saugustine]

Yet another round.

Updating D49926: Update split stack support to handle more generic prologues.

Improve error handling.
Add test file for better code-coverage. Update tests to be more
complete.

[ruiu]

Overall looking good.

ELF/Arch/X86_64.cpp

490

This looks odd -- you passed a 5 byte string and then specify 4.

[saugustine]

Fix a typo.

Updating D49926: Update split stack support to handle more generic prologues.

Improve error handling.
Add test file for better code-coverage. Update tests to be more
complete.

[saugustine]

Thanks.

ELF/Arch/X86_64.cpp

490

Miscount from when I changed it from sizeof.

[ruiu]

LGTM

[grimar]

LGTM.

test/ELF/x86-64-split-stack-prologue-adjust-fail.s

14

I was mean you probably do not need passing --defsym __morestack=0x100 --defsym _start=0x300 here.

ELF/Arch/X86_64.cpp

 
 // A split-stack prologue starts by checking the amount of stack remaining
 // in one of two ways:
 // A) Comparing of the stack pointer to a field in the tcb.
 // B) Or a load of a stack pointer offset with an lea to r10 or r11.
 template <>
 bool X86_64<ELF64LE>::adjustPrologueForCrossSplitStack(uint8_t *Loc,
                                                        uint8_t *End) const {
+  if (Loc + 8 >= End)
+    return false;
+
   // Replace "cmp %fs:0x70,%rsp" and subsequent branch
   // with "stc, nopl 0x0(%rax,%rax,1)"
-  if (Loc + 8 < End && memcmp(Loc, "\x64\x48\x3b\x24\x25", 4) == 0) {
+  if (memcmp(Loc, "\x64\x48\x3b\x24\x25", 5) == 0) {
     memcpy(Loc, "\xf9\x0f\x1f\x84\x00\x00\x00\x00", 8);
     return true;
   }
 
-  // Adjust "lea -0x200(%rsp),%r10" to lea "-0x4200(%rsp),%r10"
-  if (Loc + 7 < End && memcmp(Loc, "\x4c\x8d\x94\x24\x00\xfe\xff", 7) == 0) {
-    memcpy(Loc, "\x4c\x8d\x94\x24\x00\xbe\xff", 7);
-    return true;
-  }
-
-  // Adjust "lea -0x200(%rsp),%r11" to lea "-0x4200(%rsp),%r11"
-  if (Loc + 7 < End && memcmp(Loc, "\x4c\x8d\x9c\x24\x00\xfe\xff", 7) == 0) {
-    memcpy(Loc, "\x4c\x8d\x9c\x24\x00\xbe\xff", 7);
+  // Adjust "lea X(%rsp),%rYY" to lea "(X - 0x4000)(%rsp),%rYY" where rYY could
+  // be r10 or r11. The lea instruction feeds a subsequent compare which checks
+  // if there is X available stack space. Making X larger effectively reserves
+  // that much additional space. The stack grows downward so subtract the value.
+  if (memcmp(Loc, "\x4c\x8d\x94\x24", 4) == 0 ||
+      memcmp(Loc, "\x4c\x8d\x9c\x24", 4) == 0) {
+    // The offset bytes are encoded four bytes after the start of the
+    // instruction.
+    write32le(Loc + 4, read32le(Loc + 4) - 0x4000);
     return true;
   }
   return false;
 }
 
 template <>
 bool X86_64<ELF32LE>::adjustPrologueForCrossSplitStack(uint8_t *Loc,
                                                        uint8_t *End) const {

ELF/InputSection.cpp

 // and replace them with calls to __morestack_non_split.
 static void switchMorestackCallsToMorestackNonSplit(
     DenseSet<Defined *> &Prologues, std::vector<Relocation *> &MorestackCalls) {
 
   // If the target adjusted a function's prologue, all calls to
   // __morestack inside that function should be switched to
   // __morestack_non_split.
   Symbol *MoreStackNonSplit = Symtab->find("__morestack_non_split");
+  if (!MoreStackNonSplit) {
+    error("Mixing split-stack objects requires a definition of "
+          "__morestack_non_split");
+    return;
+  }
 
   // Sort both collections to compare addresses efficiently.
   llvm::sort(MorestackCalls.begin(), MorestackCalls.end(),
              [](const Relocation *L, const Relocation *R) {
                return L->Offset < R->Offset;
              });
   std::vector<Defined *> Functions(Prologues.begin(), Prologues.end());
   llvm::sort(
       Functions.begin(), Functions.end(),
       [](const Defined *L, const Defined *R) { return L->Value < R->Value; });
 
   auto It = MorestackCalls.begin();
   for (Defined *F : Functions) {
     // Find the first call to __morestack within the function.
     while (It != MorestackCalls.end() && (*It)->Offset < F->Value)
       ++It;
     // Adjust all calls inside the function.
     while (It != MorestackCalls.end() && (*It)->Offset < F->Value + F->Size) {
       (*It)->Sym = MoreStackNonSplit;
       ++It;
     }
   }
 }
 
-static bool enclosingPrologueAdjusted(uint64_t Offset,
+static bool enclosingPrologueAttempted(uint64_t Offset,
                                        const DenseSet<Defined *> &Prologues) {
   for (Defined *F : Prologues)
     if (F->Value <= Offset && Offset < F->Value + F->Size)
       return true;
   return false;
 }
 
 // If a function compiled for split stack calls a function not
 // compiled for split stack, then the caller needs its prologue
 // adjusted to ensure that the called function will have enough stack
 // available. Find those functions, and adjust their prologues.
 template <class ELFT>
 void InputSectionBase::adjustSplitStackFunctionPrologues(uint8_t *Buf,
                                                          uint8_t *End) {
   if (!getFile<ELFT>()->SplitStack)
     return;
-  DenseSet<Defined *> AdjustedPrologues;
+  DenseSet<Defined *> Prologues;
   std::vector<Relocation *> MorestackCalls;
 
   for (Relocation &Rel : Relocations) {
     // Local symbols can't possibly be cross-calls, and should have been
     // resolved long before this line.
     if (Rel.Sym->isLocal())
       continue;
 
     // Ignore calls into the split-stack api.
     Defined *D = cast<Defined>(Rel.Sym);
     if (D->getName().startswith("__morestack")) {
       if (D->getName().equals("__morestack"))
         MorestackCalls.push_back(&Rel);
       continue;
     }
 
     // A relocation to non-function isn't relevant. Sometimes
     // __morestack is not marked as a function, so this check comes
     // after the name check.
     if (D->Type != STT_FUNC)
       continue;
 
-    if (enclosingPrologueAdjusted(Rel.Offset, AdjustedPrologues))
+    // If the callee's-file was compiled with split stack, nothing to do.
+    auto *IS = cast_or_null<InputSection>(D->Section);
+    if (!IS || IS->getFile<ELFT>()->SplitStack)
+      continue;
+
+    if (enclosingPrologueAttempted(Rel.Offset, Prologues))
       continue;
 
     if (Defined *F = getEnclosingFunction<ELFT>(Rel.Offset)) {
-      if (Target->adjustPrologueForCrossSplitStack(Buf + F->Value, End)) {
-        AdjustedPrologues.insert(F);
+      Prologues.insert(F);
+      if (Target->adjustPrologueForCrossSplitStack(Buf + getOffset(F->Value),
+                                                   End))
         continue;
-      }
-    }
       if (!getFile<ELFT>()->SomeNoSplitStack)
-      error("function call at " + getErrorLocation(Buf + Rel.Offset) +
-            "crosses a split-stack boundary, but unable " +
-            "to adjust the enclosing function's prologue");
+        error(lld::toString(this) + ": " + F->getName() +
+              " (with -fsplit-stack) calls " + D->getName() +
+              " (without -fsplit-stack), but couldn't adjust its prologue");
+    }
   }
-  switchMorestackCallsToMorestackNonSplit(AdjustedPrologues, MorestackCalls);
+  switchMorestackCallsToMorestackNonSplit(Prologues, MorestackCalls);
 }
 
 template <class ELFT> void InputSection::writeTo(uint8_t *Buf) {
   if (Type == SHT_NOBITS)
     return;
 
   if (auto *S = dyn_cast<SyntheticSection>(this)) {
     S->writeTo(Buf + OutSecOff);

test/ELF/Inputs/x86-64-split-stack-extra.s

+# This file is split out to provide better code coverage.
+  .global split
+  .type split,@function
+split:
+  retq
+
+  .size split,. - split
+
+  .section  .note.GNU-stack,"",@progbits
+  .section  .note.GNU-split-stack,"",@progbits

test/ELF/x86-64-split-stack-prologue-adjust-fail.s

 # REQUIRES: x86
 # RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t1.o
-# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %p/Inputs/x86-64-split-stack-main.s -o %t2.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %p/Inputs/x86-64-split-stack-extra.s -o %t2.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %p/Inputs/x86-64-split-stack-main.s -o %t3.o
 
-# RUN: not ld.lld --defsym __morestack=0x100 %t1.o %t2.o -o %t 2>&1 | FileCheck %s
+# RUN: not ld.lld --defsym __morestack=0x100 --defsym __more_stack_nonsplit=0x200 %t1.o %t2.o %t3.o -o %t 2>&1 | FileCheck %s
 
 # An unknown prologue gives a match failure
-# CHECK: unable to adjust the enclosing function's
+# CHECK: error: {{.*}}.o:(.text): unknown_prologue (with -fsplit-stack) calls non_split (without -fsplit-stack), but couldn't adjust its prologue
 
 # RUN: not ld.lld -r --defsym __morestack=0x100 %t1.o %t2.o -o %t 2>&1 | FileCheck %s -check-prefix=RELOCATABLE
 # RELOCATABLE: Cannot mix split-stack and non-split-stack in a relocatable link
 
+# RUN: not ld.lld --defsym __morestack=0x100 --defsym _start=0x300 %t1.o %t2.o %t3.o -o %t 2>&1 | FileCheck %s -check-prefix=ERROR
+# ERROR: Mixing split-stack objects requires a definition of __morestack_non_split
+
   .text
 
   .global unknown_prologue
   .type unknown_prologue,@function
 unknown_prologue:
   push  %rbp
   mov %rsp,%rbp
   cmp %fs:0x70,%rsp

test/ELF/x86-64-split-stack-prologue-adjust-silent.s

 # REQUIRES: x86
 # RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t1.o
 # RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %p/Inputs/x86-64-split-stack-main.s -o %t2.o
 
-# RUN: ld.lld --defsym __morestack=0x100 %t1.o %t2.o -o %t
+# RUN: ld.lld --defsym __morestack=0x100 --defsym __morestack_non_split=0x200 %t1.o %t2.o -o %t
 # RUN: llvm-objdump -d %t 2>&1 | FileCheck %s
 
 # An unknown prologue ordinarily gives a match failure, except that this
 # object file includes a .note.GNU-no-split-stack section, which tells the
 # linker to expect such prologues, and therefore not error.
 
 # CHECK: __morestack
 

test/ELF/x86-64-split-stack-prologue-adjust-success.s

 # REQUIRES: x86
 # RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t1.o
-# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %p/Inputs/x86-64-split-stack-main.s -o %t2.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %p/Inputs/x86-64-split-stack-extra.s -o %t2.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %p/Inputs/x86-64-split-stack-main.s -o %t3.o
 
-# RUN: ld.lld --defsym __morestack=0x100 --defsym __morestack_non_split=0x200 %t1.o %t2.o -o %t -z notext
+# RUN: ld.lld --defsym __morestack=0x100 --defsym __morestack_non_split=0x200 %t1.o %t2.o %t3.o -o %t -z notext
 # RUN: llvm-objdump -d %t | FileCheck %s
 
 # Avoid duplicating the prologue for every test via macros.
 
 .macro prologue1 function_to_call
   .global prologue1_calls_\function_to_call
   .type prologue1_calls_\function_to_call,@function
 prologue1_calls_\function_to_call:
   cmp %fs:0x70,%rsp
   jae 1f
   callq __morestack
   retq
 1:
   # Various and duplicate calls to ensure every code path is taken.
   callq \function_to_call
   callq \function_to_call
   callq 1b
   callq non_function_text_symbol
   retq
   .size  prologue1_calls_\function_to_call,. - prologue1_calls_\function_to_call
 .endm
 
-.macro prologue2 function_to_call register
+.macro prologue2 function_to_call register compare_amount
   .global prologue2_calls_\function_to_call\register
   .type prologue2_calls_\function_to_call\register,@function
 prologue2_calls_\function_to_call\register:
-  lea  -0x200(%rsp),%\register
+  lea  -\compare_amount(%rsp),%\register
   cmp %fs:0x70,%\register
   jae 1f
   callq __morestack
   retq
 1:
   # Various and duplicate calls to ensure every code path is taken.
   callq \function_to_call
   callq \function_to_call

 # CHECK-NEXT: callq{{.*}}<__morestack>
 
 prologue1 split
 
 # For split-stack code calling split-stack code, ensure prologue v2 still
 # calls plain __morestack, that any raw bytes written to the prologue
 # make sense, and that the register number is preserved.
 # CHECK: prologue2_calls_splitr10:
-# CHECK-NEXT: lea{{.*}} -{{[0-9]+}}(%rsp),{{.*}}%r10
+# CHECK-NEXT: lea{{.*}} -512(%rsp),{{.*}}%r10
 # CHECK: cmp{{.*}}%fs:{{[^,]*}},{{.*}}%r{{[0-9]+}}
 # CHECK: jae{{.*}}
 # CHECK-NEXT: callq{{.*}}<__morestack>
 
-prologue2 split r10
+prologue2 split r10 0x200
 
 # CHECK: prologue2_calls_splitr11:
-# CHECK-NEXT: lea{{.*}} -{{[0-9]+}}(%rsp),{{.*}}%r11
+# CHECK-NEXT: lea{{.*}} -256(%rsp),{{.*}}%r11
 # CHECK: cmp{{.*}}%fs:{{[^,]*}},{{.*}}%r{{[0-9]+}}
 # CHECK: jae{{.*}}
 # CHECK-NEXT: callq{{.*}}<__morestack>
 
-prologue2 split r11
+prologue2 split r11 0x100
 
 # For split-stack code calling non-split-stack code, ensure prologue v1
 # calls __morestack_non_split, and that any raw bytes written to the prologue
 # make sense.
 # CHECK: prologue1_calls_non_split:
 # CHECK-NEXT: stc{{.*$}}
 # CHECK-NEXT: nopl{{.*$}}
 # CHECK: jae{{.*$}}
 # CHECK-NEXT: callq{{.*}}<__morestack_non_split>
 
 prologue1 non_split
 
 # For split-stack code calling non-split-stack code, ensure prologue v2
 # calls __morestack_non_split, that any raw bytes written to the prologue
 # make sense, and that the register number is preserved
 # CHECK: prologue2_calls_non_splitr10:
-# CHECK-NEXT: lea{{.*$}}
+# CHECK-NEXT: lea{{.*}} -16640(%rsp),{{.*}}%r10
 # CHECK: cmp{{.*}}%fs:{{[^,]*}},{{.*}}%r10
 # CHECK: jae{{.*$}}
 # CHECK-NEXT: callq{{.*}}<__morestack_non_split>
 
-prologue2 non_split r10
+prologue2 non_split r10 0x100
 
 # CHECK: prologue2_calls_non_splitr11:
-# CHECK-NEXT: lea{{.*$}}
+# CHECK-NEXT: lea{{.*}} -16896(%rsp),{{.*}}%r11
 # CHECK: cmp{{.*}}%fs:{{[^,]*}},{{.*}}%r11
 # CHECK: jae{{.*$}}
 # CHECK-NEXT: callq{{.*}}<__morestack_non_split>
 
-prologue2 non_split r11
-# call foo@plt  # for code-coverage.
-
-
-
-  .global split
-  .type split,@function
-split:
-  retq
-
-  .size split,. - split
+prologue2 non_split r11 0x200
 
   .section  .note.GNU-stack,"",@progbits
   .section  .note.GNU-split-stack,"",@progbits