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.
Differential D46653
Start support for linking object files with split stacks saugustine on May 9 2018, 11:34 AM. Authored by
Details
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
Diff Detail
Event TimelineComment Actions 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.
Comment Actions +1. Please let us see the whole implementation plan.
Comment Actions 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.
Comment Actions 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 Actions 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?
Comment Actions Fix for new objdump issues. Finally figure out style issues. Update for other comments.
Comment Actions 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. Comment Actions 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 Actions I have no more comments here.
Comment Actions 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 is inconsistent in style with other functions in the same file. Please remove the blank line.