Details
- Reviewers
Mordante philnik fsb4000 EricWF - Group Reviewers
Restricted Project - Commits
- rGc555a1237730: [libc++] Make sure std::declval() produces an error when ODR-used
Diff Detail
Event Timeline
Can you please add a test? This could be a .verify.cpp test with code similar to #61202
| libcxx/include/__utility/declval.h | ||
|---|---|---|
| 29–32 | I think it's better not to modify the template head. | |
| 30 | The return type is... cursed for cv void. It seems that decltype(std::declval<const void>) should be const void() (until C++17) / const void() noexcept (since C++17) (same for volatile void and const volatile void), but the cv-qualifiers are dropped in decltype(std::__declval<_Tp>(0)). | |
| libcxx/include/__utility/declval.h | ||
|---|---|---|
| 29–32 | ||
| libcxx/include/__utility/declval.h | ||
|---|---|---|
| 30 | I found that Eric Niebler added the patch: https://bugs.llvm.org/show_bug.cgi?id=27798 It was typename add_rvalue_reference<_Tp>::type before. | |
| libcxx/include/__utility/declval.h | ||
|---|---|---|
| 30 | I've created: https://github.com/llvm/llvm-project/issues/61232 | |
| libcxx/include/__utility/declval.h | ||
|---|---|---|
| 30 | I think the performance improvement might not apply anymore, since we have a builtin now. Using __add_rvalue_reference_t<_Tp> should fix the bug and probably not result in any performance regressions. I'll try to make a small benchmark. | |
This change is not correct and cannot proceed.
It changes the meaning of this code:
template <class T, class U, class = U[std::declval<T>()]>
constexpr bool xxx(int) { return true; }
template <class T, class U>
constexpr bool xxx(long) { return false; }
constexpr bool y = xxx<long long, int>(0);declval has been constructed very carefully to performant, and correct in all the weird cases where Clang will try to eagerly instantiate something (Ex. when it tries to check for C++11 narrowing when evaluating constant expressions).
Please be cautious when modifying weird things like declval.
What do you mean?
The change is correct.
You mean gcc and clang compiler frontends have bugs?
By the way libstdc++ has the static_assert since Nov 13, 2009 (https://github.com/gcc-mirror/gcc/commit/7274deff738213914a31413a9d1a671020694175)
and GCC failed "error: static assertion failed: declval() must not be used!"
https://gcc.godbolt.org/z/MssGMMqsd
I see that MSVC works with that example: https://gcc.godbolt.org/z/cjE8dard7
However it has the same library implementation...
Thank you for catching this, I am hoping that we have test for these edge cases, if not do you think you can enumerate more of them so that we can ensure that we are doing the right thing.
Why?
We went out of our way to make declval as low cost as we possibly could. Adding a body for the compiler to see is just work.
Cool. Thanks for the archaeology. But even if they're bugs, it means we break code. So this change isn't going anywhere as-is until
the different compilers converge on a behavior.
In the meantime, If you want to produce a better diagnostics in cases where the thing is ODR used, you can use the __attribute__((unavailable)) to do that.
Also, I'm not sure a static assert pointing to the stable name in the
standard is helpful to our users. Will they even know what it is?
Well, while this is technically a QOI issue (since ODR-using declval makes the program ill-formed no-diagnostic-required), we usually strive to provide nice error messages for users, when we can. So this seems desirable to me (assuming that doesn't make our implementation incorrect, obviously).
Summarizing the situation, it looks like Clang (with libstdc++) and GCC (with libstdc++) both reject valid code right now: https://gcc.godbolt.org/z/4Kb9qG5e5
However, what we're trying to achieve is that Clang (with libc++) does not accept this invalid code: https://gcc.godbolt.org/z/aPKjb7odo
I think we should just file a bug report against Clang and GCC and get those fixed, and then we can proceed with this change. It seems weird to me that the body of the function is instantiated when trying to check for narrowing, since we are providing the return type of declval explicitly. That just seems like a bug that could bite in other scenarios to me. And once Clang and GCC don't try to instantiate the body of declval, I don't think there would be performance concerns anymore (@EricWF do you disagree?).
| libcxx/test/std/input.output/iostream.format/output.streams/ostream/deleted_output_functions.verify.cpp | ||
|---|---|---|
| 21 ↗ | (On Diff #502914) | This is a nice catch! If this patch gets stuck, I would consider committing this separately since it's just a NFC fix to a test that is clearly incorrect. |
| libcxx/test/std/utilities/utility/declval/declval.verify.cpp | ||
| 15–23 ↗ | (On Diff #502914) | I think you could just do this: int x = std::declval<int>(); // expected-error-re@*:* {{{{(static_assert|static assertion)}} failed{{.*}}Calling declval is ill-formed, see [declval]/2.}}There is no need to have a main function in a .verify.cpp test, since it only compiles the code but doesn't try to link or run it! |
I do believe diagnostic is required (otherwise, implementations are allowed to ignore "Mandates:").
- [declval]/2 uses "Mandates:";
- [structure.specifications]/3.2 says that violation of "Mandates:" make the program ill-formed;
- [intro.compliance.general] implies that unless "no diagnostic required" is explicitly mentioned, diagnostic is required for ill-formedness.
I think this is a bug of the Clang frontend.
P0859R0 clarified when a function template or a defaulted function should be instantiated due to constant evaluation. std::declval is never a constexpr function, so it is never needed for constant evaluation, and the (ill-formed) type-id U[std::declval<T>()] should result in substitution failure without instantiating std::declval<T>.
| libcxx/include/__utility/declval.h | ||
|---|---|---|
| 30 | Can this please be a more user-friendly diagnostic? Saying "This is ill-formed. Read this [standarese]" doesn't help the user understand what is going on or how to fix it. Something along the lines of this will be more in line with the Clang effort to improve diagnostics (something that hasn't been broached with libc++ yet, but here's a good place to try a new diagnostic). static_assert( always_false<T>, "`std::declval` can only be used in an unevaluated context. It's likely that our current usage is trying to extract a value from the function."); | |
Disregard my suggestion about unavailable. It seems to always fire, including in unevaluated contexts. I feel silly.
Is this code ODR used? I suspect it is, and so it's not valid, but I'm not sure and I ran into a bunch of different instances of this pattern while testing this change.
template <class T>
decltype(auto) compute_return_type() {
// ...
// stuff that makes this function look reasonable...
// ...
return std::declval<T>();
}
using MyT = decltype(compute_return_type<int>());OK, so I've come around on this change. But please give me 2 weeks to cleanup the breakages it's going to cause inside Google.
I'll come back and let you know how many breakages were bugs vs how many were just weird meta-programming.
Yeah, that code would be invalid because declval() is ODR used IIUC.
Commandeering to finish as part of the GH PR Transition.
Rebase and apply comments. I will merge this if CI is green. Per the update in https://github.com/llvm/llvm-project/issues/61254, I don't think this causes any well-formed code to be rejected, and it fixes a real bug (https://github.com/llvm/llvm-project/issues/61202).
Merging -- the CI was green. The benchmark issue was something transient that has been fixed now.
The return type is... cursed for cv void.
It seems that decltype(std::declval<const void>) should be const void() (until C++17) / const void() noexcept (since C++17) (same for volatile void and const volatile void), but the cv-qualifiers are dropped in decltype(std::__declval<_Tp>(0)).