This is an archive of the discontinued LLVM Phabricator instance.

Start support for linking object files with split stacks
ClosedPublic

Authored by saugustine on May 9 2018, 11:34 AM.

Details

Summary

Implement linking object files compiled with -fsplit-stack, including tests.

This revision supports x86_64 (in both 32-bit and 64-bit variants). Support for other targets is simply a matter
of adding prologue adjustments.

Event Timeline

saugustine created this revision.May 9 2018, 11:34 AM
ruiu added a comment.May 10 2018, 10:59 AM

If you have other patches in the series, could you also send it for review? It would be easier for me to review individual patch once I get a whole idea about how you are going to support -fsplit-stack.

ELF/InputFiles.cpp
658

remove //

675–676

else { if -> else if

ELF/InputFiles.h
213–214

I'd think you don't need to add this comment because that's true to some other members too.

218

nit: add a blank line just like the above.

grimar added a subscriber: grimar.May 11 2018, 3:12 AM

If you have other patches in the series, could you also send it for review? It would be easier for me to review individual patch once I get a whole idea about how you are going to support -fsplit-stack.

+1. Please let us see the whole implementation plan.

ELF/InputFiles.cpp
663

I'm not an expert in licenses. I do not know if we can quote the text like that. It probably belongs to a different code license. I think we usually write own comments and just add a link if we need. Can you do the same here?

675–676

No need to do 'else' at all, the first branch ends with the return.

saugustine marked 6 inline comments as done.

Implement split stack support and tests.

saugustine edited the summary of this revision. (Show Details)May 30 2018, 2:53 PM
saugustine added inline comments.May 30 2018, 2:55 PM
ELF/InputFiles.cpp
663

I've just kept the cross reference and deleted everything else.

pcc added a subscriber: pcc.May 30 2018, 2:58 PM
pcc added inline comments.May 30 2018, 2:58 PM
ELF/InputFiles.cpp
657

I don't think Rust actually uses segmented stacks any more.
https://mail.mozilla.org/pipermail/rust-dev/2013-November/006314.html

ruiu added inline comments.May 30 2018, 3:19 PM
ELF/Arch/X86_64.cpp
482

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.

ELF/InputFiles.cpp
663

I believe quoting that amount of text should be fine, but if you are cautious, please write a comment with your own words instead of deleting the comment entirely. That was a helpful comment.

ELF/InputSection.cpp
825

What does this function do? Please write a function comment.

This function seems a bit too long. Please consider splitting.

852

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.

mgrang added inline comments.May 30 2018, 6:37 PM
ELF/InputSection.cpp
873
879

Same here.

saugustine marked 6 inline comments as done.

I've addressed all the comments. Have another look if you would.

ruiu added a comment.May 31 2018, 7:55 PM

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.

ELF/Arch/X86_64.cpp
478

This is inconsistent in style with other functions in the same file. Please remove the blank line.

483–486

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;
}
490

Style issue: put a blank line before comment.

ELF/InputSection.cpp
222

Style issue.

227

Style issue. You should use clang-format to fix format issues.

872

This function looks too dense because of the lack of any blank lines. Please make your new code look the same as other code in style.

pcc added inline comments.Jun 5 2018, 11:43 AM
ELF/Arch/X86_64.cpp
490

I think this should say %r10d, not %r10. Similarly %r11d below.

MaskRay added a subscriber: MaskRay.Jun 8 2018, 9:20 PM
saugustine marked 4 inline comments as done.Jun 15 2018, 3:06 PM

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.

ELF/Arch/X86_64.cpp
482

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.

483–486

There are two styles for this in the file. This function had followed Retpoline<ELFT>::writePltHeader (and those around it).

But done.

ELF/InputSection.cpp
852

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.
  1. 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.

  1. 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?

saugustine marked 3 inline comments as done.

Respond to comments by rewriting parts, and reformatting.

ruiu added a comment.Jun 27 2018, 1:28 AM

It has too many style issue which distracted me from actually reviewing code. Can you format your patch with clang-format-diff and upload again?

ELF/Arch/X86_64.cpp
479

I don't think we really need to support a split-stack for the x32 ABI because IIUC split-stack is used virtually only by Go, and Go doesn't support the x32.

482

Yes, it requires hand-coded if-else-if expressions, but that's in my opinion easier to read.

ELF/InputSection.cpp
222

Can you fix?

227

Ditto

842

L and R

846

Generally we use shorter names for variable name and function name. MorestackCallIter -> It

851

Please use pre-++ instead of post-++.

864

style

875

Style issue.

876

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.

881

Style

885

Style issue

889

Style issue.

902–903

Style

ELF/Target.h
66

Please follow the local convention -- all other functions that fail by default are in the .cpp file and uses llvm_unreachable to report an (impossible) error.

saugustine marked 15 inline comments as done.

Fix for new objdump issues. Finally figure out style issues. Update for other comments.

grimar added inline comments.Jun 30 2018, 12:54 AM
ELF/InputSection.cpp
223

When can this happen? That code is perhaps dead.

864

This line never executes when running the test cases.

883

This 'continue' is untested by a test case. The code is "dead" because of that.

888

This 'continue' is untested by a test case it seems.

901

The same - untested.

908

Untested.

910

Untested. enclosingPrologueAdjusted always returns false when running tests.

918

This error is untested (because of the broken tests first of all I believe).

test/ELF/Inputs/x86-64-split-stack-main.s
2

Excessive empty line.

13

Excessive empty line.

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

/usr/local/google/home/saugustine/binutils/build/gold/ld-new -> ld.lld here and below

11

Looks like a mistype, you need to change "|&" to "|" as currently, it triggers "shell parser error" when trying to run tests under windows.

14

I do not know where these strings are coming from, I think you need to test the "Function call at ..." error message here
and these strings are coming from gold probably? (because of /usr/local/google/home/saugustine/binutils/build/gold/ld-new).

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

You need to use llvm-objdump and not GNU objdump.

ruiu added inline comments.Jul 2 2018, 2:26 AM
ELF/Arch/X86_64.cpp
505

Isn't this function the same as the function which this function overrides? Can you remove?

ELF/InputSection.cpp
227–228

if (A && B) if (C && D) → if (A && B && C && D)

891

What else symbol can start with __morestack? Is this documented?

MaskRay added inline comments.Jul 2 2018, 11:09 AM
ELF/InputSection.cpp
891
% 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.

901

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?

grimar added inline comments.Jul 3 2018, 12:24 AM
ELF/InputSection.cpp
901

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.

ruiu added a comment.Jul 11 2018, 4:39 PM

We do not need to aim for 100% test coverage for new code, just like the existing code is not covered 100% by the existing tests. I'd say we should aim for reasonable test coverage though.

saugustine marked 9 inline comments as done.

Much better code coverage in tests. Fix various style issues.

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.

ELF/Arch/X86_64.cpp
505

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.

ELF/InputSection.cpp
223

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.

876

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?

883

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.

888

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.

891

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.

901

I have added code to test this case.

908

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.

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

Of course. This was waiting for https://reviews.llvm.org/D48554 to land. And now that it has, I have switched it.

ruiu added inline comments.Jul 12 2018, 1:55 PM
ELF/InputSection.cpp
876

Oh sorry I should have said DenseSet. That was just a typo.

915

Error messages should start with a lowercase letter and shouldn't end with a period.

grimar added inline comments.Jul 13 2018, 3:18 AM
ELF/InputSection.cpp
223

I do not think it is possible to call this function for synthetic sections for your case.

getLocation already have the check and your new code does not seem is able to call it for synthetics,
because I believe they have Relocations empty, so this piece is dead.
I would just remove it, assert would be excessive as code anyways would crash.

883

The following test triggers this continue.

# REQUIRES: x86
# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t1.o
# RUN: ld.lld %t1.o -o %t

.local foo
foo:

.section .text,"ax",@progbits
 .quad foo
	
.section	.note.GNU-stack,"",@progbits
.section	.note.GNU-split-stack,"",@progbits

You could merge it with x86-64-split-stack-prologue-adjust-success.s I think.

888

The lines are still there.

I was able to trigger this case after adding the call foo@plt and -shared
to your x86-64-split-stack-prologue-adjust-success.s test:

# RUN: ld.lld --defsym __morestack=0x100 --defsym __morestack_non_split=0x200 %t1.o %t2.o -shared -o %t
...

.size	split,. - split

call foo@plt
	
.section	.note.GNU-stack,"",@progbits
.section	.note.GNU-split-stack,"",@progbits
saugustine marked an inline comment as done.

Cleanup error message. Small amounts of additional coverage.

saugustine added inline comments.Jul 16 2018, 5:13 PM
ELF/InputSection.cpp
888

Using "-shared" creates problems because llvm-objdump doesn't support .plt sections very well.

https://bugs.llvm.org/show_bug.cgi?id=38186

Any check using both -shared and llvm-objdump will likely be effectively a bitwise comparison.

I would like not to block this change on getting that bug fixed, just so that we can have a single extra line of coverage, in near trivial code.

Can we just not have coverage for this single line?

I have no more comments here.

ELF/InputSection.cpp
888

Go ahead, I'll handle this after your commit.

I have committed this change as r337332.

saugustine accepted this revision.Jul 17 2018, 4:24 PM

Ruiu is out until next week, but his last round of comments were just with error message formatting, and I have addressed them. Committed as r337332.

This revision is now accepted and ready to land.Jul 17 2018, 4:24 PM
saugustine closed this revision.Jul 17 2018, 4:24 PM
test/ELF/Inputs/x86-64-split-stack-main.s