Exit unwinding early when at the top of the stack and additional info won't be found.

[saugustine]

Summary

This patch follows libgcc's lead: When the return-address register is
zero, there won't be additional stack frames to examine, or gather
information about. Exit before spending time looking for something
known not to be found.

[saugustine]

The harbormaster failures can't possibly be related. For example, the failure below has nothing whatever to do with this change.

/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/libunwind/src/CompactUnwinder.hpp:345:10: error: use of undeclared identifier 'UNW_STEP_SUCCESS'; did you mean 'UNW_ESUCCESS'? [clang-diagnostic-error]

return UNW_STEP_SUCCESS;
       ^~~~~~~~~~~~~~~~
       UNW_ESUCCESS

[mstorsjo]

+1 the change looks sensible to me - but I'd leave it to someone else of the runtime projects' maintainers (@mclow.lists @ldionne @EricWF) to approve.

[ldionne]

The change makes sense to me, but TBH I don't consider myself an owner of libunwind because I don't know the code base sufficiently well. I'd trust @mstorsjo (or @phosek or @compnerd) since they have far more commits to libunwind.

BTW, folks, would you like for a libunwind review team to be setup in Phabricator like we did for libc++abi and libc++? It's easy to do and would clarify who's considered a code owner for libunwind.

[compnerd]

This should be safe, if pc is ever 0, we don't have any chance of recovery.

@ldionne - yes, a phabricator review team sounds useful, would you please set that up?

[ldionne]

Is @mstorsjo, @phosek and @compnerd a suitable set of members? I'll coordinate with @benhamilton to create it (he's the Phabricator magician).

[mstorsjo]

In D77099#1957168, @ldionne wrote:

Is @mstorsjo, @phosek and @compnerd a suitable set of members? I'll coordinate with @benhamilton to create it (he's the Phabricator magician).

I'm not comfortable reviewing/approving everything in libunwind, but I've spent some time debugging it at least, so I can at least review things that touch some bits, and the windows aspect.

[ldionne]

In D77099#1957662, @mstorsjo wrote:

In D77099#1957168, @ldionne wrote:

Is @mstorsjo, @phosek and @compnerd a suitable set of members? I'll coordinate with @benhamilton to create it (he's the Phabricator magician).

I'm not comfortable reviewing/approving everything in libunwind, but I've spent some time debugging it at least, so I can at least review things that touch some bits, and the windows aspect.

A review group has been setup now, so at least one person of the group needs to LGTM a patch before it can be merged. It doesn't have to be you when you're not comfortable, but the idea is to give a notion of who the code owners (or at least knowledgeable people) are. Otherwise, there's always a question of is X's review sufficient or not? Who else should I ask? Creating a review group makes that easier. Also, the group allows self-add/remove, so it can adjust itself organically.

[saugustine]

Don't know why Phab thinks compnerd's accepting the revision March 1, at 9:54 left it in an unaccepted state. Happy to revert it if necessary, but I don't think so.

Also, I've now landed about a dozen patches to libunwind. Don't know if that qualifies me to be added to that group or not, but I'm happy to review what I can.

[ldionne]

In D77099#1957794, @saugustine wrote:

Don't know why Phab thinks compnerd's accepting the revision March 1, at 9:54 left it in an unaccepted state. Happy to revert it if necessary, but I don't think so.

Don't worry about it, it's just that we set up a review group in the meantime, and it was added as a blocking reviewer after @compnerd accepted it. You are fine.

Also, I've now landed about a dozen patches to libunwind. Don't know if that qualifies me to be added to that group or not, but I'm happy to review what I can.

There's also the option of making yourself a watcher of the libunwind project -- that way you'll be notified every time there's a patch and you can chime in. That's what I personally did for me since I am interested, but I don't consider myself a code owner.

libunwind/src/UnwindCursor.hpp

 void UnwindCursor<A, R>::setInfoBasedOnIPRegister(bool isReturnAddress) {
   pint_t pc = (pint_t)this->getReg(UNW_REG_IP);
 #if defined(_LIBUNWIND_ARM_EHABI)
   // Remove the thumb bit so the IP represents the actual instruction address.
   // This matches the behaviour of _Unwind_GetIP on arm.
   pc &= (pint_t)~0x1;
 #endif
 
+  // Exit early if at the top of the stack.
+  if (pc == 0) {
+    _unwindInfoMissing = true;
+    return;
+  }
+
   // If the last line of a function is a "throw" the compiler sometimes
   // emits no instructions after the call to __cxa_throw.  This means
   // the return address is actually the start of the next function.
   // To disambiguate this, back up the pc when we know it is a return
   // address.
   if (isReturnAddress)
     --pc;