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

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

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.



================
Comment at: src/UnwindCursor.hpp:757
 
-  pint_t thisPC = itThisPC.functionAddress();
-  pint_t nextPC = itNextPC.functionAddress();
-  pint_t indexDataAddr = itThisPC.dataAddress();
+  pint_t thisPC, nextPC, indexDataAddr;
+  if (itNextPC == end) {
----------------
I've not seen multiple comma separated declarations in libunwind, instead I've seen each on a separate line. May be worth staying consistent. 


================
Comment at: src/UnwindCursor.hpp:759
+  if (itNextPC == end) {
+    EHABISectionIterator<A> last =
+        EHABISectionIterator<A>::last(_addressSpace, sects);
----------------
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.


================
Comment at: src/UnwindCursor.hpp:764
+      return false;
+    nextPC = ~static_cast<pint_t>(0);
+    indexDataAddr = last.dataAddress();
----------------
In DwarfParser.hpp I've seen std::numeric_limits<pint_t>::max used, personally I prefer this to ~(0). 


================
Comment at: src/UnwindCursor.hpp:767
+  } else {
+    EHABISectionIterator<A> itThisPC = itNextPC - 1;
+    thisPC = itThisPC.functionAddress();
----------------
This is by far the most common case, is it worth rewriting if (itNextPC ==end) as if (itNextPC != end) and making this appear first. 


https://reviews.llvm.org/D35265