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

[Sterling Augustine via Phabricator via llvm-commits]

saugustine marked 4 inline comments as done.
saugustine added a comment.

In https://reviews.llvm.org/D46653#1118480, @ruiu wrote:

> Could you read other functions in the same file and other files in the same directory and make your code look the same as others in coding style? Consistency is really important, and it is hard for me to read some code in this patch because of lack of consistency.


There is a variety of styles already in the file. I had followed Retpoline<ELFT>::writePltHeader for the format of the byte arrays and such. But I have no strong preference here.

So done.



================
Comment at: ELF/Arch/X86_64.cpp:474-477
+  const uint8_t P0[] = {0x64, 0x3b, 0x24, 0x25};
+  const uint8_t R0[] = {0xf9, 0x0f, 0x1f, 0x84, 0x00, 0x00, 0x00, 0x00};
+  if (Loc + sizeof(P0) < End && memcmp(P0, Loc, sizeof(P0)) == 0 &&
+      Loc + sizeof(R0) < End) {
----------------
ruiu wrote:
> Please write more directly and concise as we do in other places in the same file.
> 
>   if (Loc + 8 < End && memcmp(Loc, "\x64\x3b\x24\x25", 4)) {
>     memcpy(Loc, "\xf9\x0f\x1f\x84\x00\x00\x00\x00", 8);
>     return true;
>   }
There are two styles for this in the file.  This function had followed  Retpoline<ELFT>::writePltHeader (and those around it).

But done.


================
Comment at: ELF/Arch/X86_64.cpp:474
+    &X86_64<ELF32LE>::getPrologueAdjustments() const {
+  static llvm::SmallVector<SplitStackPrologueAdjustment, 4> PAs = {
+      SplitStackPrologueAdjustment(
----------------
ruiu wrote:
> We generally don't use the table-driven coding pattern that much in lld, especially when compared to other LLVM subprojects. I prefer less abstracted code. I'd write this as a (boring but simple) sequence of "if" and "else if" that calls memcmp() and memcpy() to replace a matched pattern.
That means every single target that implements split stack also has to hand code the if-the-else routine for each replacement. That's a lot of boilerplate, but done.


================
Comment at: ELF/InputSection.cpp:820
+      // fall through to the reporting below.
+      if (Defined *F = getEnclosingFunction<ELFT>(Rel.Offset)) {
+        // Nothing to be done if this prologue was already adjusted.
----------------
ruiu wrote:
> This function seems too slow to call for each relocation. The number of relocation can be an order of tens of millions, so per-relocation processing needs to be really fast.
Not sure if you are referring to getEnclosingFunction, or adjustCrossSplitFunctionPrologues. Either way:

1. This processing only happens if the input section was an executable split-stack section.

2. This loop does several very cheap checks on each relocation before it does the heavy lifting, and therefore skips most of them . I have rewritten it a bit to show that more accurately.

Also, I have added an additional filter that checks if the relocation references a function, which should limit the heavy work even more.

3. getEnclosingFunction (which is the really expensive part) is only called when the relocation crosses a split-stack to non-split-stack boundary. There are likely only very few of those. Nevertheless, I have updated the code to check if the enclosing prologue was already found. This should reduce the heavy lifting quite a bit more.

If performance is still too slow, making this fast probably means building a table of functions and their sizes per executable section, or possibly building it lazily and caching the results. What is your preferred solution?


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D46653