[PATCH] D46653: Start support for linking object files with split stacks

[Sterling Augustine via Phabricator via llvm-commits]

saugustine added a comment.

Finally figured out the source of my clang-format problems (/usr/bin/clang-format was giving different results that $build_dir/bin/clang-format, so hopefully all the style issues are solved now.

Updated the test cases for changes to llvm-objdump, fixed other issues.

Also, thanks for all the code coverage comments. I have added several bits of code to the test-cases to ensure additional paths are taken, but I haven't managed to get all of them.

If the goal is 100% code coverage, the code will have to be much less conservative in its error checking.



================
Comment at: ELF/Arch/X86_64.cpp:505
+                                                       uint8_t *End) const {
+  llvm_unreachable("Target doesn't support split stacks.");
+}
----------------
ruiu wrote:
> Isn't this function the same as the function which this function overrides? Can you remove?
I get link errors if I remove the entire implementation (because the template <class ELFT> class X86_64 declares adjustPrologueForCrossSplitStack), so there needs to be some implementation for ELF32. I think the only way around that would be to add another layer of inheritance, which seems overkill.

Deleting the implementation also gives up feature parity with gnu gold, which does support 32-bit here. That's not important to me, but worth making an explicit decision over.


================
Comment at: ELF/InputSection.cpp:842
+  if (!getFile<ELFT>()->SplitStack) return;
+  std::set<Defined *> AdjustedPrologues;
+  std::vector<Relocation *> MorestackCalls;
----------------
ruiu wrote:
> We generally avoid `std::set` because it is an ordered map and slower than `llvm::DenseMap`. If you don't need an ordered map, please use DenseMap instead.
This is a pure set (keys but no values), so done with DenseSet instead of DenseMap.

If you would rather DenseMap, should I just duplicate the key for the value?


================
Comment at: ELF/InputSection.cpp:223
+  if (getFile<ELFT>() == nullptr)
+    return nullptr;
+
----------------
grimar wrote:
> When can this happen? That code is perhaps dead.
This function was refactored out of getLocation, and the check was added there for https://reviews.llvm.org/D30889.  getLocation still does the check.

Any synthetic section won't have a backing file. I have added a comment to that effect.

I don't think an assert would be any better for a client than returning a nullptr here, but I'll switch it if we want 100% code coverage. Up to you.


================
Comment at: ELF/InputSection.cpp:882
+    if (Rel.Sym->isLocal())
+      continue;
+    Defined *D = dyn_cast_or_null<Defined>(Rel.Sym);
----------------
grimar wrote:
> This 'continue' is untested by a test case. The code is "dead" because of that.
I've spent quite a bit of time trying to write a test case that triggers executes the continue, but I haven't managed to figure one out.

So I'm not sufficiently clever to write a test case to trigger this line, but I'm not convinced it can never happen.


================
Comment at: ELF/InputSection.cpp:887
+    if (!D)
+      continue;
+
----------------
grimar wrote:
> This 'continue' is untested by a test case it seems.
I've tried to write test-cases to execute this continue, but have been unable to do so. They protect against an erroneous or strange input.

I've just deleted these two lines, if someone manages to trigger this case, lld will just crash.


================
Comment at: ELF/InputSection.cpp:890
+    // Ignore calls into the split-stack api.
+    if (D->getName().startswith("__morestack")) {
+      if (D->getName().equals("__morestack"))
----------------
MaskRay wrote:
> ruiu wrote:
> > What else symbol can start with `__morestack`? Is this documented?
> ```
> % ar x /usr/lib/gcc/x86_64-linux-gnu/8/libgcc.a morestack.o 
> % llvm-nm -U morestack.o | grep morestack
> 0000000000000037 T __morestack
> 0000000000000159 T __morestack_get_guard
> 000000000000011c T __morestack_large_model
> 000000000000016d T __morestack_make_guard
> 0000000000000000 T __morestack_non_split
> 0000000000000163 T __morestack_set_guard
> ```
> 
> `generic-morestack.o` in `libgcc.a` also defines some `__morestack_*` functions. I'm not sure whether they are related.
They are related. 

I don't know of any formal documentation of the other functions, but we really don't want to mess around with any of them.


================
Comment at: ELF/InputSection.cpp:900
+    if (D->Type != STT_FUNC)
+      continue;
+    InputSection *IS = dyn_cast_or_null<InputSection>(D->Section);
----------------
grimar wrote:
> MaskRay wrote:
> > grimar wrote:
> > > The same - untested.
> > Not related to the revision. Are you using `-fprofile-arcs -ftest-coverage` + `llvm-cov` (http://lists.llvm.org/pipermail/llvm-dev/2018-April/122782.html) to collect coverage information?
> Yes. 
> 
> But in simple cases (for writing/reviewing the patches, for example) just placing `assert(false)` or putting a breakpoint
> is sufficient to check possible dead places.
I have added code to test this case.


================
Comment at: ELF/InputSection.cpp:907
+    if (IS && IS->getFile<ELFT>()->SplitStack)
+      continue;
+    if (enclosingPrologueAdjusted(Rel.Offset, AdjustedPrologues))
----------------
grimar wrote:
> Untested.
I have deleted these lines. Again, this seems somewhat risky to me, but I have been unable to write a test case where it would happen.


================
Comment at: test/ELF/x86-64-split-stack-prologue-adjust-silent.s:6
+# RUN: /usr/local/google/home/saugustine/binutils/build/gold/ld-new --defsym __morestack=0x100 %t1.o %t2.o -o %t
+# RUN: objdump -d %t | FileCheck %s
+
----------------
grimar wrote:
> You need to use llvm-objdump and not GNU objdump.
Of course. This was waiting for https://reviews.llvm.org/D48554 to land. And now that it has, I have switched it.



Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D46653