Details
- Reviewers
- None
- Group Reviewers
Restricted Project
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
libcxx/include/__config_hardening | ||
---|---|---|
20–40 ↗ | (On Diff #526366) | This should move to __config under the ABI selection block. Or while we're at it, I've been meaning to move the ABI checks to somewhere outside of __config for a while but never got to it, so this could be a refactoring we do before adding these new knobs. Maybe __config_dir/abi.h, __config_dir/assertions.h, __config_dir/availability.h, etc? |
61 ↗ | (On Diff #526366) | Let's create a bug report on github to track this improvement and explain what it is in a few words. |
63 ↗ | (On Diff #526366) | We should have a way to define default values for those in the __config_site so that a vendor can pick a default value for a given platform. This will probably require using 0 and 1 instead of just #ifndef to check for these macros. |
63 ↗ | (On Diff #526366) | We should add documentation for these macros where we document the safe libc++ mode (actually this needs more updating in light of the "new" debug mode). |
63 ↗ | (On Diff #526366) | We should have tests that a user can enable/disable the mode on a per-TU basis -- that's a pretty important property of the design. You can probably take inspiration from libcxx/test/libcxx/assertions/assertions_disabled.pass.cpp. |
71 ↗ | (On Diff #526366) | This is going to conflict with the existing "legacy" debug mode. Can you write a short RFC proposing the removal of the legacy debug mode since it's really causing trouble here and that legacy debug mode has been broken. A few problems about it:
|
78 ↗ | (On Diff #526366) | For this one, we probably want to do // in __config_dir/assertions.h #ifdef _LIBCPP_DEBUG_RANDOMIZE_UNSPECIFIED_STABILITY # define _LIBCPP_ASSERTIONS_ENABLE_DEBUG_RANDOMIZE_UNSPECIFIED_BEHAVIOR # warning "this is deprecated, use _LIBCPP_ASSERTIONS_ENABLE_DEBUG_RANDOMIZE_UNSPECIFIED_BEHAVIOR" #endif Or maybe we can mark the macro as deprecated with an attribute, it seems to ring a bell but I'm not sure anymore. |
84–95 ↗ | (On Diff #526366) | I think we should just use _LIBCPP_ASSERT here. _LIBCPP_ASSERT isn't tied to the debug mode anymore, it's just a self-standing utility for assertions in libc++ and we should use it universally. #ifdef _LIBCPP_ENABLE_HARDENING_ASSERTIONS # define _LIBCPP_HARDENING_ASSERT(...) _LIBCPP_ASSERT(__VA_ARGS__) #else # define _LIBCPP_HARDENING_ASSERT(...) /* nothing */ #endif |
102 ↗ | (On Diff #526366) | We should force ourselves to put a semicolon after using the macro, otherwise editors will be confused. |
libcxx/include/__hardening.h | ||
26 ↗ | (On Diff #526366) | Should this be part of some kind of __hardening_traits class? Would there be anything else in there other than __refer_to_same_range? |
libcxx/include/__config_hardening | ||
---|---|---|
20–40 ↗ | (On Diff #526366) | Created https://reviews.llvm.org/D151900 to move things under __config_dir (without any other changes for now). We want to move __config itself into the new directory as well, right? |
71 ↗ | (On Diff #526366) | Posted the RFC to Discourse: https://discourse.llvm.org/t/rfc-removing-the-legacy-debug-mode-from-libc/71026/1 |
libcxx/include/__config_hardening | ||
---|---|---|
61 ↗ | (On Diff #526366) |
hardening.h -> hardening
libcxx/include/__config_hardening | ||
---|---|---|
20–40 ↗ | (On Diff #526366) | Update: this turned out to be a lot harder than initially assumed because the split makes it easier to forget to include a header and mistakenly presume that a macro is undefined. I will postpone D151900 for now and keep everything in the same __config file for the purposes of this patch. Moved to the ABI section of __config instead. |
63 ↗ | (On Diff #526366) | It looks like the flags I'm passing to the command line get ignored (probably overwritten), I'll need to investigate more to find out why. |
libcxx/include/__hardening.h | ||
26 ↗ | (On Diff #526366) | Done. My first idea was to rely on overload resolution to find the best match given that _Iter is a dependent name. However, that needs ADL to work which I coincidentally disabled by explicitly qualifying the function name with std::. I think using a traits class is more convenient regardless, thanks for the suggestion! |
libcxx/include/__config | ||
---|---|---|
88 | These are leftovers from testing, but I'm keeping them for now as useful reminders. | |
92 | Note to self: this prevents a compiler warning/error. | |
286 | IIUC, the current state where _LIBCPP_ENABLE_NEW_DEBUG_MODE automatically enables _LIBCPP_ASSERTIONS_ENABLE_HARDENING_ASSERTIONS doesn't quite work because then the user cannot use _LIBCPP_ASSERTIONS_ENABLE_HARDENING_ASSERTIONS to override the current hardening mode, right? If so, does that mean that the user has to define both e.g. _LIBCPP_ASSERTIONS_ENABLE_HARDENING_ASSERTIONS and _LIBCPP_ENABLE_NEW_DEBUG_MODE? | |
325 | I copied this from existing macros. Is there a reason to prefer this form to just /*nothing*/? | |
libcxx/include/__config_site.in | ||
42 | Some notes and questions:
| |
46 | Do we want lower-level macros like _LIBCPP_ASSERTIONS_ENABLE_CHECK_INVALID_RANGE here? | |
48 | I used the word "feature" here since it's not directly a check, rather it enables a certain behavior. Also, this name is pretty verbose, very open to suggestions on how to shorten it. | |
libcxx/test/std/containers/views/views.span/span.cons/iterator_sentinel.pass.cpp | ||
117 | Looks like this operator wasn't invoked before. |
I really like the shape of this. At a high level, here's what I would recommend:
- Extract the ABI macro for bounded iterators out of this patch. This will make this patch smaller and also get that out of the way when you try to remove the legacy debug mode.
- Introduce the assertions machinery in a separate patch: _LIBCPP_ENABLE_HARDENED_MODE and _LIBCPP_ENABLE_NEW_DEBUG_MODE, but without any of the check macros like _LIBCPP_ASSERTIONS_ENABLE_CHECK_BAD_CONTAINER_ACCESS. Yes, that patch is going to look kinda stupid, but that will allow us to properly consider the interaction between those settings and the existing settings we have.
- Then, in a separate patch, let's introduce some (or all) of the _LIBCPP_ASSERTIONS_ENABLE_CHECK_foo checks and plumb them into _LIBCPP_ENABLE_HARDENED_MODE/_LIBCPP_ENABLE_NEW_DEBUG_MODE which will already have been introduced.
libcxx/include/__config | ||
---|---|---|
196–198 | I would suggest not introducing this macro, at least not for now. Instead, I would rename _LIBCPP_ASSERTIONS_ENABLE_HARDENING_BOUNDED_ITERATORS to _LIBCPP_ABI_BOUNDED_ITERATORS, a normal ABI macro like we have others and I would simply not define it in any configuration by default. In fact, this could be done in a separate patch to simplify this one quite a bit. Actually if you do that in a separate patch, _LIBCPP_ABI_BOUNDED_ITERATORS could take over _LIBCPP_DEBUG_ITERATOR_BOUNDS_CHECKING. | |
242 | Since this affects both hardening and "debug" checks, I don't think _LIBCPP_ASSERTIONS_ENABLE_HARDENING_ASSERTIONS is the right name. Is this different from _LIBCPP_ENABLE_ASSERTIONS? Answer: The difference between this macro and _LIBCPP_ENABLE_ASSERTIONS is that the latter also disables some currently-uncategorized assertions. IMO it's not worth introducing another public knob for this, I think people don't need that much granularity. I can't think of anyone wanting to disable hardening and debug assertions while somehow keeping all the "uncategorized" assertions. | |
274–276 | Suggestion: instead, you could do this outside of the #if block: #if _LIBCPP_ENABLE_HARDENED_MODE && _LIBCPP_ENABLE_NEW_DEBUG_MODE # error "Only one of _LIBCPP_ENABLE_HARDENED_MODE and _LIBCPP_ENABLE_NEW_DEBUG_MODE can be defined." #endif | |
316 | Let's fully qualify everything in macros. | |
325 | I think it's to make this valid to use in an expression. I have a test that _LIBCPP_ASSERT can be used in an expression, and we should have similar tests for new assertion macros being introduced (possibly add to the same test). | |
libcxx/include/__config_site.in | ||
42 |
I would agree with that.
As I suggested, I think we can remove _LIBCPP_ENABLE_HARDENED_MODE_ABI_BREAK entirely, at least for now. This makes the issue moot. | |
46 | I don't think it makes sense for those to be settable at configure time in the __config_site.in. I think I would only define the default value we want for the debug and hardened mode in here. Basically, vendors could decide whether they want a specific build of libc++ to enable the debug/hardened mode by default (users could still override it) by setting it in CMake. For example you might want to do: #cmakedefine01 _LIBCPP_ENABLE_HARDENED_MODE_DEFAULT #cmakedefine01 _LIBCPP_ENABLE_DEBUG_MODE_DEFAULT Those would be set from CMake and would provide the default value to use for _LIBCPP_ENABLE_HARDENED_MODE resp _LIBCPP_ENABLE_DEBUG_MODE inside __config. | |
libcxx/include/__debug_utils/randomize_range.h | ||
14 | To reduce the size of this patch, I would suggest that we "onboard" features into the hardened/debug mode in separate patches. This patch can introduce the basic functionality, but then we can have a separate patch that simply moves _LIBCPP_DEBUG_RANDOMIZE_UNSPECIFIED_STABILITY from the old debug mode into the "new debug mode". I think doing those piecemeal is going to make this patch much easier to review, but also give us space to consider important things like transitions and temporary backwards compatibility for existing macros. | |
libcxx/include/span | ||
241 | I find this potentially confusing. The name _LIBCPP_ASSERT_CHECK_INVALID_RANGE makes it look as if we're checking that the range is invalid, when we're trying to do the reverse. Should this be called _LIBCPP_ASSERT_CHECK_VALID_RANGE instead (and similarly for other types of checks)? | |
libcxx/test/std/containers/views/views.span/span.cons/iterator_sentinel.pass.cpp | ||
117 | Let's land this as a NFC before this patch. Actually, is there a reason for defining it at all if it's not used? |
Address more feedback
libcxx/include/__config | ||
---|---|---|
242 | https://reviews.llvm.org/D153816 (s/_LIBCPP_ASSERT/_LIBCPP_ASSERT_UNCATEGORIZED). | |
325 | Do we need to add those tests considering that we deliberately define the new assertions to expand to _LIBCPP_ASSERT which is already thoroughly tested in this regard? | |
libcxx/include/span | ||
241 | I think it depends on how the reader sees the purpose of the check. I'm looking at it from the perspective of "which condition will cause the assertion to trigger?" (and more broadly, "what kind of bad behavior are we providing guarantees against?"). Or to put it simpler, I'm reading this kinda like "abort if the range is invalid", which I find slightly more natural than "make sure the range is valid and proceed". I don't feel too strongly about this and can be convinced to change. One argument in favor of "check valid range" is that it follows the semantics of "assert" (which fires if the condition is false, not true). EDIT: after some thinking, I went ahead with the renaming as suggested in this comment. | |
libcxx/test/std/containers/views/views.span/span.cons/iterator_sentinel.pass.cpp | ||
117 | Done. It's used by some of the hardening checks. |
Thanks for working on this!
libcxx/include/__config | ||
---|---|---|
88 | Would it make sense to try to land the __config split patch first? | |
206 | Is it intended to add all containers in the future? | |
223 | I really dislike names with _NEW_ in it, it tends to get stale at some point. Just like terms as "modern C++" which some people feel is C++11... I would suggest _LIBCPP_ENABLE_HARDENED_DEBUG_MODE instead. Or would it make sense to do | |
238 | ||
244 | Can you change them to TODO HARDENING or similar to make grepping them easier. | |
253 | Thanks a lot for the documentation! It really helps to understand what the hardening mode will do. |
libcxx/include/__config | ||
---|---|---|
196–198 | Patch for introducing _LIBCPP_ABI_BOUNDED_ITERATORS: https://reviews.llvm.org/D153895 |
https://reviews.llvm.org/D153895
- Introduce the assertions machinery in a separate patch: _LIBCPP_ENABLE_HARDENED_MODE and _LIBCPP_ENABLE_NEW_DEBUG_MODE, but without any of the check macros like _LIBCPP_ASSERTIONS_ENABLE_CHECK_BAD_CONTAINER_ACCESS. Yes, that patch is going to look kinda stupid, but that will allow us to properly consider the interaction between those settings and the existing settings we have.
https://reviews.llvm.org/D153902
- Then, in a separate patch, let's introduce some (or all) of the _LIBCPP_ASSERTIONS_ENABLE_CHECK_foo checks and plumb them into _LIBCPP_ENABLE_HARDENED_MODE/_LIBCPP_ENABLE_NEW_DEBUG_MODE which will already have been introduced.
I'll reuse this patch once the other two patches have landed.
libcxx/include/__config | ||
---|---|---|
223 | My hope is that I can land https://reviews.llvm.org/D153672 first which would allow me to reuse the "obvious" name _LIBCPP_ENABLE_DEBUG_MODE. Otherwise, I like _LIBCPP_ENABLE_HARDENED_DEBUG_MODE -- I wasn't very happy with the "new" name either, so thanks a lot for the suggestion! Re. using 1 and 2 as levels -- I agree it automatically makes them mutually exclusive which is a good property to have. On the other hand, having named macros is perhaps a little more readable than using numbers to represent each mode -- it's not obvious without reading the documentation what exactly _LIBCPP_ENABLE_HARDENED_MODE 2 means (and whether 2 is the max value or not). Following @ldionne's feedback, I created a new patch that only adds the configuration macros, without any assertions, and used the name _LIBCPP_ENABLE_HARDENED_DEBUG_MODE there: https://reviews.llvm.org/D153902 |
This patch is no longer relevant -- most of its contents have been merged in other patches.
These are leftovers from testing, but I'm keeping them for now as useful reminders.