This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by saugustine on Jul 27 2018, 12:02 PM.

Event Timeline

saugustine created this revision.Jul 27 2018, 12:02 PM

ruiu, do you have any thoughts on this?

ruiu added inline comments.Jul 30 2018, 2:11 PM
ELF/Arch/X86_64.cpp
501

nit: remove the empty line.

ELF/InputSection.cpp
902

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 added a subscriber: grimar.Jul 31 2018, 1:07 AM
grimar added inline comments.
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 marked 4 inline comments as done.

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.

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

grimar added inline comments.Aug 1 2018, 1:07 AM
ELF/InputSection.cpp
854

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.

928

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 marked 4 inline comments as done.Aug 1 2018, 11:55 AM

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 marked an inline comment as done.

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 added a comment.Aug 1 2018, 4:00 PM

Overall looking good.

ELF/Arch/X86_64.cpp
490

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

saugustine updated this revision to Diff 158660.Aug 1 2018, 4:26 PM
saugustine marked an inline comment as done.

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.

Thanks.

ELF/Arch/X86_64.cpp
490

Miscount from when I changed it from sizeof.

ruiu accepted this revision.Aug 1 2018, 4:38 PM

LGTM

This revision is now accepted and ready to land.Aug 1 2018, 4:38 PM
grimar accepted this revision.Aug 1 2018, 11:00 PM

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.

This revision was automatically updated to reflect the committed changes.