Page MenuHomePhabricator

Don't fail the shared_ptr test if libc++ has insufficient debug info.
ClosedPublic

Authored by saugustine on Thu, Apr 15, 6:06 PM.

Details

Summary

Don't fail the shared_ptr test if libc++ has insufficient debug info.

This addresses https://bugs.llvm.org/show_bug.cgi?id=48937

Diff Detail

Event Timeline

saugustine requested review of this revision.Thu, Apr 15, 6:06 PM
saugustine created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptThu, Apr 15, 6:06 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Generally makes sense to me - I'd probably lean on this being a bit too narrow (but if it handles the current cases, we can generalize if/when needed), in that it'll be a bit brittle to changes to libc++. Well, I guess if anything's renamed, or moved into the libc++ library it'll require touching the pretty printer anyway.

Ah, I see - so you're looking for the field which is a member of shared_count - might be good for the comment to have a few more words about what the purpose is there. Connecting the dots between "determine if libc++ itself has debug info" and why this test achieves that result (something like... because shared_count has virtual functions and has its key function defined in the libc++ library, so the type definition will only be emitted there (if libc++ itself is built with debug info) and without the definition the __shared_owners_ field won't be available)

Any sense of the difference between UNSUPPORTED and IGNORED for things like this?

Dramatically rework the last attempt.

Thanks for the review. I reworked to take a somewhat different approach--this improves the printer itself to tolerate the situation quite a bit better. But I have included your improved comment at the decision point.

dblaikie accepted this revision.Mon, Apr 19, 4:56 PM

Oh, nice improvement! Yeah, the number of bugs we get due to the arbitrary/inscrutible nature of the failure when pretty printing std::string due to this optimization... - hopefully the more scrutible error message will hopefully reduce that and/ror make triage easier when folks do reach out for help.

ldionne accepted this revision as: Restricted Project.Tue, Apr 20, 6:55 AM
ldionne added a subscriber: ldionne.

LGTM for libc++ if you folks agree this is OK.

This revision is now accepted and ready to land.Tue, Apr 20, 6:55 AM

@saugustine Please watch out for your commit messages, they are messed up:

commit 0e8378032597bcaccb948de88e965ad75bfaeb7b
Author: Sterling Augustine <saugustine@google.com>
Date:   Thu Apr 15 18:03:01 2021 -0700

    Don't fail the shared_ptr test if libc++ has insufficient debug info.

    Don't fail the shared_ptr test if libc++ has insufficient debug info.

    This addresses https://bugs.llvm.org/show_bug.cgi?id=48937

    Differential Revision: https://reviews.llvm.org/D100610