This is an archive of the discontinued LLVM Phabricator instance.

[libc++] add declval failure assertion for instantiation
ClosedPublic

Authored by ldionne on Mar 6 2023, 6:24 AM.

Diff Detail

Event Timeline

fsb4000 created this revision.Mar 6 2023, 6:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 6 2023, 6:24 AM
fsb4000 requested review of this revision.Mar 6 2023, 6:24 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptMar 6 2023, 6:24 AM
fsb4000 updated this revision to Diff 502622.Mar 6 2023, 6:38 AM

inline variables is a c++17 feature

fsb4000 updated this revision to Diff 502624.Mar 6 2023, 6:45 AM

add a test

ldionne requested changes to this revision.Mar 6 2023, 6:45 AM
ldionne added a subscriber: ldionne.

Can you please add a test? This could be a .verify.cpp test with code similar to #61202

This revision now requires changes to proceed.Mar 6 2023, 6:45 AM
fsb4000 updated this revision to Diff 502642.Mar 6 2023, 7:24 AM

change a compile.fail test to a verify test

fsb4000 updated this revision to Diff 502648.Mar 6 2023, 7:41 AM
shafik added a subscriber: shafik.Mar 6 2023, 7:51 AM

Thank you for fixing this so quickly.

philnik accepted this revision as: philnik.Mar 6 2023, 7:53 AM
fsb4000 updated this revision to Diff 502859.Mar 6 2023, 4:43 PM

add _LIBCPP_HIDE_FROM_ABI

fsb4000 updated this revision to Diff 502865.Mar 6 2023, 5:08 PM

fix a test

frederick-vs-ja added inline comments.
libcxx/include/__utility/declval.h
29–32

I think it's better not to modify the template head.
Doesn't libc++ have a concentrated __dependent_false? If so, I think we can still use !__is_same(_Tp, _Tp) in static_assert.

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)).

fsb4000 updated this revision to Diff 502914.Mar 6 2023, 9:22 PM
fsb4000 marked an inline comment as done.
fsb4000 added inline comments.Mar 6 2023, 9:38 PM
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.

fsb4000 marked an inline comment as done.Mar 6 2023, 10:12 PM
fsb4000 added inline comments.
libcxx/include/__utility/declval.h
30
philnik added inline comments.Mar 7 2023, 5:53 AM
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.

EricWF requested changes to this revision.Mar 7 2023, 1:45 PM
EricWF added a subscriber: EricWF.

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.

This revision now requires changes to proceed.Mar 7 2023, 1:45 PM
fsb4000 added a comment.EditedMar 7 2023, 5:21 PM

@EricWF

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...

shafik added a comment.Mar 7 2023, 9:35 PM

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.

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.

@EricWF

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...

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?

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.

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
16–24

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!

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.

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?).

I do believe diagnostic is required (otherwise, implementations are allowed to ignore "Mandates:").

  1. [declval]/2 uses "Mandates:";
  2. [structure.specifications]/3.2 says that violation of "Mandates:" make the program ill-formed;
  3. [intro.compliance.general] implies that unless "no diagnostic required" is explicitly mentioned, diagnostic is required for ill-formedness.

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.

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>.

Just use __attribute__((unavailable("declval cannot be called")) or some such.

cjdb added a subscriber: cjdb.Mar 10 2023, 12:22 PM
cjdb added inline comments.
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>());
EricWF added a comment.Apr 6 2023, 7:34 AM

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.

ldionne commandeered this revision.Sep 26 2023, 6:33 AM
ldionne edited reviewers, added: fsb4000; removed: ldionne.

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>());

Yeah, that code would be invalid because declval() is ODR used IIUC.

Commandeering to finish as part of the GH PR Transition.

ldionne marked an inline comment as done.Sep 26 2023, 6:47 AM
ldionne updated this revision to Diff 557360.Sep 26 2023, 6:48 AM

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).

ldionne accepted this revision as: Restricted Project.Sep 26 2023, 6:48 AM
ldionne updated this revision to Diff 557406.Sep 27 2023, 6:56 AM

Format added test.

EricWF resigned from this revision.Oct 3 2023, 11:51 AM

I'll unblock this. But I don't love it.

This revision is now accepted and ready to land.Oct 3 2023, 11:51 AM

Merging -- the CI was green. The benchmark issue was something transient that has been fixed now.

This revision was landed with ongoing or failed builds.Oct 26 2023, 8:27 AM
This revision was automatically updated to reflect the committed changes.