This is an archive of the discontinued LLVM Phabricator instance.

Enable overriding `__libcpp_debug_function` invocation
AbandonedPublic

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

Details

Reviewers
jdoerrie
EricWF
jfb
Quuxplusone
palmer
akhuang
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.May 7 2021, 10:59 AM

Rebased against the new origin/main.

EricWF accepted this revision.May 11 2021, 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.May 11 2021, 7:58 AM
Quuxplusone requested changes to this revision.May 11 2021, 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.May 11 2021, 8:29 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.

thakis added a subscriber: thakis.May 19 2021, 6:03 PM
thakis added inline comments.
libcxx/include/__debug
42

(For B: We build libc++ from source and statically link it, so that's fine for us.)

palmer added inline comments.May 24 2021, 11:44 AM
libcxx/docs/DesignDocs/DebugMode.rst
113

The implementation in include/__locale includes some iterators. If you have turned on this debugging mode, those iterators will be checked. Nothing directly API-exposed, but under the hood, debug checks could fire. I figure people might want to know.

libcxx/include/__debug
42

Why do we (Chromium) want this: in Chromium, we want these debugging/hardening checks in production. But, we are very sensitive to object code size, and the standard implementation has those print statements (which can get big). This macro allows us to use the smallest possible crash code (e.g. a ud2 on Intel, or the like), and thus have the least impact on our overall object code size.

libcxx/docs/DesignDocs/DebugMode.rst
113

Ah, that makes sense. But then I repeat: "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.)"
As of 2021-05-24, nobody is still replying to that email. I don't think we should pursue this PR D89353 until someone does.

palmer added inline comments.May 24 2021, 1:32 PM
libcxx/docs/DesignDocs/DebugMode.rst
113

In Chromium, we build libcxx entirely from source, so I don't think we have that problem.

akhuang commandeered this revision.May 24 2021, 2:58 PM
akhuang added a reviewer: palmer.
akhuang added a subscriber: akhuang.

apparently i'll take over this patch from here!

akhuang updated this revision to Diff 350714.Jun 8 2021, 2:28 PM

Remove parens from macro and add test case

thought I'd updated this patch but apparently I hadn't--

ldionne requested changes to this revision.Jun 10 2021, 11:47 AM
ldionne added a subscriber: ldionne.

For some reason this went entirely under my radar.

I think we should have a broader discussion about what to do with the debug mode (top of thread: https://lists.llvm.org/pipermail/libcxx-dev/2021-June/001168.html) before we make this change.

If I understand correctly, the main problem being solved by this patch is that Chromium does not want to have those __FILE__ string constants stored in the compiled binaries (because it doesn't use that information anyway)?

This revision now requires changes to proceed.Jun 10 2021, 11:47 AM

If I understand correctly, the main problem being solved by this patch is that Chromium does not want to have those __FILE__ string constants stored in the compiled binaries (because it doesn't use that information anyway)?

It's not just the string constants, it's that we want everything about the code to be as small as possible:

"But, we are very sensitive to object code size, and the standard implementation has those print statements (which can get big). This macro allows us to use the smallest possible crash code (e.g. a ud2 on Intel, or the like), and thus have the least impact on our overall object code size."

rnk added a subscriber: rnk.Jun 15 2021, 2:32 PM

If I understand correctly, the main problem being solved by this patch is that Chromium does not want to have those __FILE__ string constants stored in the compiled binaries (because it doesn't use that information anyway)?

As I understand it, the plan is to actually ship with these checks enabled. This expands the scope of these debug mode checks from a testing tool to a security hardening tool. That's why the binary size matters: it will ship to users. It starts from the simple idea that std::vector<>::operator[] could have bounds checks, and one way to get there is to enable debug checks and add this override mechanism.

jfb added a comment.Jun 15 2021, 3:06 PM

FWIW I support something along the lines of what Palmer is trying to do: many users of the STL would like to be able to handle particular types of library UB instead of performing potentially unbounded effects. Which ones is a bit of an art that we need to figure out, for example I wouldn't want to unduly affect performance (by e.g. changing algorithmic complexity) or code size (here, with diagnostic bloat). In a way we're experimenting on the edges of what contract checking would be for the STL, and I would approach it with this optic: could we have a mode of the STL which enforces contracts, and a mode where you get whatever. If so, what should be checked and what should remain unchecked, and what are the principles to distinguish these. I think in this discussion it's important to separate the invariants that are details of libc++'s implementation, from what are actual contracts at the STL API level.

ldionne commandeered this revision.Aug 31 2023, 8:09 AM
ldionne edited reviewers, added: akhuang; removed: ldionne.

[Github PR Transition cleanup]

The Debug mode doesn't exist anymore, but this was done in D141326. I will close this to clean up the queue (which requires commandeering and abandoning, sorry if that seems rude but it's how Phabricator works).

Herald added a project: Restricted Project. · View Herald TranscriptAug 31 2023, 8:09 AM
ldionne abandoned this revision.Aug 31 2023, 8:10 AM
rnk added a comment.Aug 31 2023, 9:08 AM

+1, this use case has long been addressed by the assertion mode, and then the new safe and hardened modes.