[libcxx-commits] [PATCH] D121478: [libc++] Add a lightweight overridable assertion handler

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/__assert:44
+_LIBCPP_OVERRIDABLE_FUNC_VIS
+void __libcpp_assertion_handler(char const* __file, int __line, char const* __expression, char const* __message);
 
----------------
Mordante wrote:
> ldionne wrote:
> > Mordante wrote:
> > > I assume you didn't add `[[noreturn]]` to be compatible with C++98.
> > Actually, I omitted `[[noreturn]]` because I wanted to leave the door open for an assertion handler to return. Yes, I know the documentation says users shouldn't do it, but I was thinking that perhaps there are useful applications I'm not thinking about.
> > 
> > I'm not married to this -- do you think I should strengthen the signature and force everybody's handler to be `__attribute__((noreturn))`? I assume that works with all supported compilers, I can confirm if we agree we want to do this.
> I don't think the strengthening is required. It won't work in C++03 and when there are indeed user who want to return it really becomes UB. I can think of one use case: users using a sanitizer and only want to log asserts to see whether the sanitzers pick up an error.
SGTM


================
Comment at: libcxx/utils/generate_feature_test_macro_components.py:944
 
+#include <__assert>
 #include <__config>
----------------
Mordante wrote:
> ldionne wrote:
> > Mordante wrote:
> > > This looks suspicious. Most headers don't include `__assert`.
> > > Is this required for all headers? When it is I prefer to see required part be moved to `__config`.
> > > Alternatively we need to enforce all headers include `version`. I added this, but there are no validations executed in the CI.
> > > 
> > We need to settle on a way users can be *guaranteed* that `std::__libcpp_assertion_handler` will be declared. I'm currently using "include any libc++ header and it will be declared", but it could be something else, like "include `<cassert>` and you can be guaranteed it'll be declared", or whatever. It could also be something more libc++ specific like "include `<__libcpp_assertion_handler>` and it will be declared", but I like that less.
> > 
> > WDYT about including `<__assert>` in `<cassert>`, and saying that users only need to include `<cassert>` in order to define a custom assertion handler? The only thing I'd be slightly worried about is giving the false impression that this assertion handler will be used for `assert(...)`, which is not the case. Actually, while we're at it, do we think that the `assert` macro should use the custom assertion handler? That's a really good question, I guess.
> The only issue is, that not every header is required to include `version`. 
> `git grep -L --no-recursive 'include.*version' -- libcxx/include/` shows a lot of our c headers, `barrier` and `initializer_list`.
> So it we use version we need to add that to these headers.
> Since all new C++ proposals add a new feature-test-macro, I think there's not a big chance of regressions.
> 
> So I like the idea users only need to include any Standard header, but then we need to make sure it works for all of them.
> I think any header is safer, when it requires `<cassert>` I'm not sure whether a tool like IWYU will remove the include by accident.
> 
> I think using this handler for `assert` sounds interesting. But I see a bit of an issue, we only ship `cassert` and not `assert.h`. I'm not sure whether that can cause subtile issues depending on whether `cassert` or `assert.h` is included. But when we go that route it should be done in a separate patch.
I agree that any header should be sufficient. I propose this because the patch is starting to grow quite a bit: For now, I'll require that `<__assert>` be included before defining a custom handler. Then, I will relax this requirement in future patches:

1. Refactor the generated tests that check for per-header properties -- each header should be tested in a different translation unit instead.
2. Add a test that checks that all public headers provide a declaration for the assertion handler

Once we have that framework in place, we can also easily add a test to check that e.g. all public headers define e.g. `_LIBCPP_VERSION` (we have such tests today but they are written manually, see e.g. `libcxx/test/libcxx/numerics/cfenv/version.pass.cpp`).



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121478/new/

https://reviews.llvm.org/D121478