Page MenuHomePhabricator

Enable overriding `__libcpp_debug_function` invocation
Needs RevisionPublic

Authored by palmer on Oct 13 2020, 5:02 PM.

Details

Reviewers
jdoerrie
EricWF
jfb
Quuxplusone
Group Reviewers
Restricted Project
Summary

Enable not just changing the handler function at run-time, but also
enable changing how it's invoked at compile time.

Document that, and update some other Debug Mode documentation as well.

Diff Detail

Event Timeline

palmer created this revision.Oct 13 2020, 5:02 PM
palmer requested review of this revision.Oct 13 2020, 5:02 PM

This is following up on the discussion at https://reviews.llvm.org/D70343. Does this overall direction make sense to you? I have hacked it into Chromium and it seems to work.

jdoerrie accepted this revision.Oct 14 2020, 1:37 AM

LGTM, no concerns from my side.

This revision is now accepted and ready to land.Oct 14 2020, 1:37 AM

Thanks, jdoerrie! Does anyone else have thoughts?

palmer updated this revision to Diff 343722.Fri, May 7, 10:59 AM

Rebased against the new origin/main.

EricWF accepted this revision.Tue, May 11, 7:58 AM
EricWF added a reviewer: Restricted Project.

Assuming this still merges, I don't have any issue with it.

There has been some work done by others on the debug mode. We don't need to wait for their approval, but please be receptive if they have comments.

EricWF accepted this revision.Tue, May 11, 7:58 AM
Quuxplusone requested changes to this revision.Tue, May 11, 8:29 AM
Quuxplusone added a subscriber: Quuxplusone.
Quuxplusone added inline comments.
libcxx/docs/DesignDocs/DebugMode.rst
113

In what sense does std::locale support iterators?

libcxx/include/__debug
42

(A) Please use #x here instead of (x) (and then x instead of #x in the macro above). This preserves the current assertion messages without extra parens. I don't think there's any reason it's a big deal; but we have a choice between "add an extra set of parens" or "don't", and I think "don't" is clearly if microscopically better.

(B) Even after reading D70343, I don't get the point of this new abstraction. Is it so that you can hard-code #define _LIBCPP_DEBUG_FUNCTION_INVOCATION std::abort() at compile time instead of setting __libcpp_debug_function = std::abort; at runtime? Why is the latter unacceptable to you?
Also, are you aware that libc++ compiles some _LIBCPP_ASSERT checks into its libc++.a binary? — Failures in those checks will respect the runtime setting of __libcpp_debug_function, but they will not respect macros set in the user's code. (I sent an email to libcxx-dev about this issue, specifically that debug iterators are broken because of it, on 2021-05-01, but nobody's replying to my email.)

(C) If you want libc++ to support this abstraction long-term, you absolutely need to "put a test on it." libcxx/test/libcxx/debug/ would probably be the right place for a new test that verifies the behavior of defining _LIBCPP_DEBUG_FUNCTION_INVOCATION.

This revision now requires changes to proceed.Tue, May 11, 8:29 AM
Quuxplusone added inline comments.Tue, May 11, 8:30 AM
libcxx/docs/DesignDocs/DebugMode.rst
113

I should add: Other than this std::locale diff which I don't understand (yet?), I love this documentation change; feel free to land it separately from the rest of this PR.