Page MenuHomePhabricator

[ELF] Fix handling of FDE negative relative PC addr
ClosedPublic

Authored by andrewng on Jul 19 2018, 10:25 AM.

Details

Summary

Signed values for the FDE PC addr were not correctly handled in
readFdeAddr(). If the value is negative and the type of the value is
smaller than 64 bits, the FDE PC addr overflow error would be
incorrectly triggered.

Fixed readFdeAddr() to properly handle signed values by sign extending
where appropriate.

Diff Detail

Repository
rL LLVM

Event Timeline

andrewng created this revision.Jul 19 2018, 10:25 AM
ruiu added a comment.Jul 19 2018, 11:25 AM

Thanks! How did yoy find this?

Nice catch! Few comments/suggestions are below.

test/ELF/eh-frame-negative-relative-pcaddr.s
1 ↗(On Diff #156307)

Should this test case be in a linkerscript sub-folder? Since it relies on LS.

4 ↗(On Diff #156307)

Maybe it worth to check the .eh_frame_hdr section content like eh-frame-value-format8.s/eh-frame-value-format7.s
tests do? To demonstrate that we encoded the address correctly and were able to build a valid eh_frame_hdr entry.

10 ↗(On Diff #156307)

I think this can be a single line:

SECTIONS { .text : { *(.text) } .eh_frame : { *(.eh_frame) } }

Also, what about adding 3 test cases? A one for each new type you added (DW_EH_PE_sdata2, DW_EH_PE_sdata4, DW_EH_PE_sdata8).

andrewng added a comment.EditedJul 20 2018, 2:46 AM

Thanks! How did yoy find this?

Our LLD port uses linker scripts where .eh_frame is placed after .text, so a number of our test cases started failing with the "PC address is too large" error from commit r337382. That led me to finding this issue.

Also, what about adding 3 test cases? A one for each new type you added (DW_EH_PE_sdata2, DW_EH_PE_sdata4, DW_EH_PE_sdata8).

Yes, that's a good idea, although having had a quick look, I'm not too sure we have test coverage for all the other types either. Also I think some of the .eh_frame value format tests are using invalid augmentation strings, but these issues aren't related to this particular patch.

andrewng updated this revision to Diff 156505.Jul 20 2018, 8:47 AM

Updated testing as per review comments.

andrewng marked 2 inline comments as done.Jul 20 2018, 8:49 AM
andrewng updated this revision to Diff 156531.Jul 20 2018, 10:18 AM

Update to the eh-frame-negative-pcrel-sdata8.s test.

ruiu accepted this revision.Jul 20 2018, 10:27 AM

LGTM

ELF/SyntheticSections.cpp
524–525 ↗(On Diff #156531)

nit: move this above udata4 so that they are sorted by size.

This revision is now accepted and ready to land.Jul 20 2018, 10:27 AM
This revision was automatically updated to reflect the committed changes.