Page MenuHomePhabricator

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

Authored by saugustine on Mar 30 2020, 3:22 PM.

Details

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.

Diff Detail

Event Timeline

saugustine created this revision.Mar 30 2020, 3:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 30 2020, 3:22 PM

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 added subscribers: ldionne, EricWF.

+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.

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 accepted this revision.Apr 1 2020, 9:54 PM

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?

This revision is now accepted and ready to land.Apr 1 2020, 9:54 PM

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

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.

Herald added a project: Restricted Project. · View Herald TranscriptApr 2 2020, 10:54 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
This revision now requires review to proceed.Apr 2 2020, 10:54 AM
This revision was not accepted when it landed; it landed in state Needs Review.Apr 2 2020, 11:23 AM
This revision was automatically updated to reflect the committed changes.

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.

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.

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.