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

[chill]

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.

[chill]

Ping.

[peter.smith]

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

757 ↗

(On Diff #106043)

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

759 ↗

(On Diff #106043)

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.

764 ↗

(On Diff #106043)

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

767 ↗

(On Diff #106043)

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]

src/UnwindCursor.hpp

699 ↗

(On Diff #106043)

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

754 ↗

(On Diff #106043)

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

[chill]

src/UnwindCursor.hpp

699 ↗

(On Diff #106043)

Indeed, it is. Fixed.

767 ↗

(On Diff #106043)

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

[chill]

Addressed the comments above.

[peter.smith]

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

[chill]

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]

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.

libunwind/trunk/src/UnwindCursor.hpp

 template <typename A, typename R>
 bool UnwindCursor<A, R>::getInfoFromEHABISection(
     pint_t pc,
     const UnwindInfoSections &sects) {
   EHABISectionIterator<A> begin =
       EHABISectionIterator<A>::begin(_addressSpace, sects);
   EHABISectionIterator<A> end =
       EHABISectionIterator<A>::end(_addressSpace, sects);
+  if (begin == end)
+    return false;
 
   EHABISectionIterator<A> itNextPC = std::upper_bound(begin, end, pc);
-  if (itNextPC == begin || itNextPC == end)
+  if (itNextPC == begin)
     return false;
   EHABISectionIterator<A> itThisPC = itNextPC - 1;
 
   pint_t thisPC = itThisPC.functionAddress();
-  pint_t nextPC = itNextPC.functionAddress();
+  // If an exception is thrown from a function, corresponding to the last entry
+  // in the table, we don't really know the function extent and have to choose a
+  // value for nextPC. Choosing max() will allow the range check during trace to
+  // succeed.
+  pint_t nextPC = (itNextPC == end) ? std::numeric_limits<pint_t>::max()
+                                    : itNextPC.functionAddress();
   pint_t indexDataAddr = itThisPC.dataAddress();
 
   if (indexDataAddr == 0)
     return false;
 
   uint32_t indexData = _addressSpace.get32(indexDataAddr);
   if (indexData == UNW_EXIDX_CANTUNWIND)
     return false;