This is an archive of the discontinued LLVM Phabricator instance.

[libunwind] Handle .ARM.exidx tables without sentinel last entry
ClosedPublic

Authored by chill on Jul 11 2017, 9:29 AM.

Details

Summary

UnwindCursor<A, R>::getInfoFromEHABISection assumes the last
entry in the index table never corresponds to a real function.
Indeed, GNU ld always inserts an EXIDX_CANTUNWIND entry,
containing the end of the .text section. However, the EHABI specification
(http://infocenter.arm.com/help/topic/com.arm.doc.ihi0038b/IHI0038B_ehabi.pdf)
does not seem to contain text that requires the presence of a sentinel entry.
In that sense the libunwind implementation isn't compliant with the specification.

This patch makes getInfoFromEHABISection examine the last entry in the index
table if upper_bound returns the end iterator.

Diff Detail

Event Timeline

chill created this revision.Jul 11 2017, 9:29 AM
emaste added a subscriber: emaste.Jul 17 2017, 6:08 PM

I assume that this is a fix for https://bugs.llvm.org/show_bug.cgi?id=31091 ? Given that the exception being thrown from the last table is a special case I think it is worth a comment, along with a comment on why the value of nextPC has been chosen in this case. I've also made a few subjective stylistic suggestions, I'm not a contributor to libunwind so please give these little weight.

I think that this fix is worth making as any linker implementing the ARM EHABI as per the standard may not put in the sentinel and will be at risk of not catching an exception thrown through the function with the highest address in the program if using libunwind, this could lead to unexpected program termination. Alternatively if it is felt that libunwind requires a linker to put in a sentinel section, it should be made clear with at least a comment in the source and or the documentation.

src/UnwindCursor.hpp
762

I've not seen multiple comma separated declarations in libunwind, instead I've seen each on a separate line. May be worth staying consistent.

764

I think it is worth a comment explaining the special case that it is possible that an exception is thrown in the last table entry due to a sentinel not being mandatory, and upper_bound won't catch this. When this occurs an arbitrary choice of nextPC has to be chosen.

769

In DwarfParser.hpp I've seen std::numeric_limits<pint_t>::max used, personally I prefer this to ~(0).

772

This is by far the most common case, is it worth rewriting if (itNextPC ==end) as if (itNextPC != end) and making this appear first.

compnerd added inline comments.Jul 18 2017, 9:48 PM
src/UnwindCursor.hpp
699

Isn't this equivalent to --end()?

751

Can't the second clause be lifted above the upper_bound?

chill marked 5 inline comments as done.Jul 19 2017, 8:41 AM
chill added inline comments.
src/UnwindCursor.hpp
699

Indeed, it is. Fixed.

772

The iterator class does not implement operator!=, I think it's not worth it just because of a stylistic change.

chill updated this revision to Diff 107311.Jul 19 2017, 8:42 AM
chill marked an inline comment as done.

Addressed the comments above.

I'm happy my comments have been addressed, no further comments.

chill added a comment.EditedJul 19 2017, 8:50 AM

AFAICT, GCC's libgcc will happily accept an exidx table without a sentinel at the end, and DTRT: https://github.com/gcc-mirror/gcc/blob/master/libgcc/unwind-arm-common.inc#L156

compnerd accepted this revision.Jul 21 2017, 5:42 PM

I wish that we could add a test for this. Unfortunately, libunwind as it stands doesn't do a very good job with tests :-(. Thanks for cleaning up the change and fixing this.

This revision is now accepted and ready to land.Jul 21 2017, 5:42 PM
This revision was automatically updated to reflect the committed changes.