This is an archive of the discontinued LLVM Phabricator instance.

[clang][SemaCXX] Diagnose tautological uses of consteval if and is_constant_evaluated
ClosedPublic

Authored by hazohelet on Jul 12 2023, 3:21 AM.

Details

Summary

This patch makes clang diagnose extensive cases of consteval if and is_constant_evaluated usage that are tautologically true or false.
This introduces a new IsRuntimeEvaluated boolean flag to Sema::ExpressionEvaluationContextRecord that means the immediate appearance of if consteval or is_constant_evaluated are tautologically false(e.g. inside if !consteval {} block or non-constexpr-qualified function definition body)
This patch also pushes new expression evaluation context when parsing the condition of if constexpr and initializer of constexpr variables so that Sema can be aware that the use of consteval if and is_consteval are tautologically true in if constexpr condition and constexpr variable initializers.
BEFORE this patch, the warning for is_constant_evaluated was emitted from constant evaluator. This patch moves the warning logic to Sema in order to diagnose tautological use of is_constant_evaluated in the same way as consteval if.

This patch separates initializer evaluation context from InitializerScopeRAII.
This fixes a bug that was happening when user takes address of function address in initializers of non-local variables.

Fixes https://github.com/llvm/llvm-project/issues/43760
Fixes https://github.com/llvm/llvm-project/issues/51567

Diff Detail

Event Timeline

hazohelet created this revision.Jul 12 2023, 3:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 12 2023, 3:21 AM
hazohelet requested review of this revision.Jul 12 2023, 3:21 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 12 2023, 3:21 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Thanks for working on this!
I left some comments, i hope it helps!

clang/include/clang/Sema/Sema.h
1300

I think I'd prefer a boolean for that, ExpressionEvaluationContext somewhat matches standard definitions.
I think we might mostly be able to reuse PotentiallyEvaluatedIfUsed | PotentiallyEvaluated.

RuntimeEvaluated is anything that does not have a parent context that is constant evaluated/immediate/unevaluated. Maybe you could try to make a function for that?

clang/lib/Parse/ParseDeclCXX.cpp
3230–3232

I think I'd prefer casting D to a VarDecl rather than adding a parameter

3239–3246

Maybe we should instead say that a constexpr FieldDecl is ConstantEvaluated ?
InConstantEvaluated in general seems redundant

clang/lib/Parse/ParseExpr.cpp
241

I'd rather either get rid of InConstantEvaluated, or at least consider UnevaluatedContext as ConstantEvaluated everywhere (ie by changing DiagnoseTautologicalIsConstantEvaluated)

clang/lib/Parse/ParseStmt.cpp
1514–1517

This seems wrong, the condition of a if statement is not constant evaluated.

clang/lib/Sema/SemaDecl.cpp
15446–15457

There are a bunch of white spaces only changes there

clang/lib/Sema/SemaDeclCXX.cpp
18248–18292

Can you explain the changes to this file? they seems to affect test cases unrelated to the goal of this PR

clang/test/SemaCXX/constant-expression-cxx11.cpp
1964

This change does not seem correct

I only looked at the libc++ part.

libcxx/test/std/utilities/meta/meta.const.eval/is_constant_evaluated.verify.cpp
27

Since libc++ support the latest ToT Clang and the last two official releases this wont work. The expected-warning needs to be a expected-warning-re that works for both the new and old diagnostic

philnik added inline comments.
libcxx/test/std/utilities/meta/meta.const.eval/is_constant_evaluated.verify.cpp
27

You can also just shorten it to 'std::is_constant_evaluated' will always evaluate to. Seems good enough to me.

hazohelet updated this revision to Diff 540029.Jul 13 2023, 7:40 AM
hazohelet edited the summary of this revision. (Show Details)

Address comments from Corentin, Mark and Nikolas

  • Removed InConstantEvaluated
  • Removed RuntimeEvaluated expression evaluation context kind and instead added a boolean IsRuntimeEvaluated to Sema::ExpressionEvaluationContextRecord
  • Handle unevaluated context as constant when emitting the diagnostics
  • NFC stylistic changes
  • Fixed libcxx CI test
hazohelet marked 9 inline comments as done.Jul 13 2023, 8:02 AM

Thank you for the review!

clang/include/clang/Sema/Sema.h
1300

I made it a boolean instead of a new expression evaluation context.

RuntimeEvaluated is anything that does not have a parent context that is constant evaluated/immediate/unevaluated. Maybe you could try to make a function for that?

I dont't think it is correct. The definitions body of constexpr-qualified function and non-qualified function both push PotentiallyEvaluated, so we need some way to distinguish them, and this time it is a boolean.

clang/lib/Parse/ParseDeclCXX.cpp
3239–3246

I removed InConstantEvaluated

clang/lib/Parse/ParseStmt.cpp
1514–1517

In this case /*ShouldEnter=*/IsConstexpr, so the constant evaluation context is pushed only when it is a condition of constexpr if. The condition of constexpr if shall be a constant expression as per https://eel.is/c++draft/stmt.if#2

clang/lib/Sema/SemaDeclCXX.cpp
18248–18292

When we push/pop evaluation context here, it behaves synchronously with InitializerScopeRAII, in ParseDecl.cpp, which makes the evaluation of Actions.AddInitializerToDecl happen outside of the initializer expression evaluation context (e.g. initializer scope popped in 2581 and AddInitializerToDecl evaluated in 2592 in ParseDecl.cpp). This is causing a bug BEFORE this patch (https://godbolt.org/z/b18jxKT54 and the removed FIXME in test/SemaCXX/cxx2a-consteval.cpp) because we were pushing evaluation context if the variable isNonlocalVariable. This PR separates the handling of expression evaluation context of initializers from InitializerScopeRAII to resolve these bugs.

The arguable changes in diagnostics caused by this are the removals of
warn_uninit_self_reference_in_reference_init against constexpr int &n = n; (You mentioned) and
warn_impcast_integer_precision_constant against constexpr signed char a = 2*100;
Both are diagnosed by calling Sema::DiagRuntimeBehavior, and Sema::DiagIfReachable called by this intentionally avoids emitting diagnostics against initializer of constexpr variables. (https://github.com/llvm/llvm-project/blob/ab73bd3897b58ecde22ec5eea14b6252f2d69b6e/clang/lib/Sema/SemaExpr.cpp#L20712-L20719)
They were diagnosed BEFORE this patch because the expression evaluation context was outside of the initializer evaluation context, and it emitted false positives against cases like constexpr signed char a = true ? 3 : 200; (https://godbolt.org/z/9z1rer7E3)
I think if the evaluated result of constexpr variable initializer does not fit the variable type it should be diagnosed, but the logic should be added to where we handle constexpr variables.

clang/test/SemaCXX/constant-expression-cxx11.cpp
1964

This is because it is diagnosed by Sema::DiagRuntimeBehavior
In this case constexpr evaluator is emitting an error, so I don't think the loss of this warning is problematic.
Please see my comment about the changes in ParseDeclCXX.cpp for details

libcxx/test/std/utilities/meta/meta.const.eval/is_constant_evaluated.verify.cpp
27

Thanks!

Mordante added inline comments.Jul 14 2023, 8:49 AM
libcxx/test/std/utilities/meta/meta.const.eval/is_constant_evaluated.verify.cpp
27

I really would like a regex. To me the current message misses an important piece of information; the true part. I care less about the rest of the message, but stripping the true means a warning like std::is_constant_evaluated' will always evaluate to FALSE would be valid too.

cor3ntin added inline comments.Jul 16 2023, 7:00 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
1579–1580

To be consistent with other tautological warnings

8886
clang/include/clang/Sema/Sema.h
1300

Right, thanks for clarifying, i missed that

clang/lib/Parse/ParseDecl.cpp
2517

Why are we looking at whether static vars are const qualified?

clang/lib/Parse/ParseStmt.cpp
1514–1517

I missed that, makes sense!

clang/lib/Sema/SemaDeclCXX.cpp
18248–18292

Thanks for the detailed explanation.
Separating InitializerScopeRAII from evaluation scope is something i support.
Did you try removing the test in DiagIfReachable, see how much breaks?
In any case, there are 2 fixme in that function already, that should cover us.
It might be useful to better describe these changes in the release notes / commit message

libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy.segmented.pass.cpp
96

this is a funny one, what's the history of that?

libcxx/test/std/utilities/meta/meta.const.eval/is_constant_evaluated.verify.cpp
27

Agreed with Mordante

philnik added inline comments.Jul 16 2023, 10:09 AM
libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy.segmented.pass.cpp
96

Probably some code moving around. I think this was originally in another function.

libcxx/test/std/utilities/meta/meta.const.eval/is_constant_evaluated.verify.cpp
27

We're not in the business of testing the compiler though. Taking a closer look, I'm not actually sure why this test exists at all. It doesn't seem like it tests anything useful w.r.t. the library. This has been added in 2fc5a78, but there the warning isn't checked, so that was clearly not the original intention.

hazohelet marked an inline comment as done.Jul 17 2023, 9:22 AM
hazohelet added inline comments.Jul 17 2023, 9:22 AM
clang/lib/Parse/ParseDecl.cpp
2517
void func() { // IsRuntimeEvaluated
	int        A = std::is_constant_evaluated(); // false
	const int  B = std::is_constant_evaluated(); // not tautology
	static int C = std::is_constant_evaluated(); // not tautology
}

Variable initializers can inherit the enclosing evaluation scope's IsRuntimeEvaluated attribute in most cases, but when the enclosing scope is IsRuntimeEvaluated and the variable is const-qualified or non-local, the initializer is not IsRuntimeEvaluated.
Manually setting context kind to PotentiallyEvaluated is redundant, so I'll remove it.

clang/lib/Sema/SemaDeclCXX.cpp
18248–18292

I forgot to write that this PR pushes ConstantEvaluated on constexpr var initializers and thus DiagRuntimeBehavior returns before calling DiagIfReachable.

Locally, I tried making DiagRuntimeBehavior emit diagnostics on constexpr var initializers. It causes test failure in 7 test files and the added diagnostics seem redundant.
I don't list all of them here, but especially problematic would be variable template: https://github.com/llvm/llvm-project/blob/e8dc9dcd7df798039ccf23d74343bf688b1fd648/clang/test/CXX/temp/temp.constr/temp.constr.constr/non-function-templates.cpp#L11-L16 . Clang would emit false positive about narrowing conversion every time it gets instantiated.
So, I don't think it's a good idea to let DiagRuntimeBehavior emit diagnostics on constexpr var initializers.
I'll better describe the change here, thanks.

ldionne added inline comments.
libcxx/test/std/utilities/meta/meta.const.eval/is_constant_evaluated.verify.cpp
27

FWIW I agree, in fact I think we could probably use // ADDITIONAL_COMPILE_FLAGS: -Xclang -verify-ignore-unexpected=warning

hazohelet updated this revision to Diff 541528.Jul 18 2023, 7:52 AM
hazohelet marked 8 inline comments as done.
hazohelet edited the summary of this revision. (Show Details)

Address review comments

  • Rename tablegen name of the diagnostics to follow other tautological warnings
  • Remove redundant modification of context kind to PotentiallyEvaluated
  • Use regex for libcxx test part (tentative)

Regarding the clang CI failure, libcxx's __libcpp_is_constant_evaluated always returns false under C++03 or earlier, where constexpr isn't available.
(Code: https://github.com/llvm/llvm-project/blob/036a1b2202cb71aacfa72ef15145a88dc50a02cf/libcxx/include/__type_traits/is_constant_evaluated.h#L26-L28)
_LIBCPP_CONSTEXPR expands to nothing under C++03 mode, and thus this returns false every time as clang now says.
(Live demo: https://godbolt.org/z/oszebM5o5)
I'm not sure how I should fix it, so I would like to ask for help from libcxx folks.

ldionne accepted this revision as: Restricted Project.Jul 19 2023, 6:45 AM

LGTM for libc++.

This looks good to me modulo nitpicks.
When you land that, please make an issue on github for the missing narrowing warning, it seems important.

I'll wait before approving in case @aaron.ballman spot things i missed

clang/lib/Sema/SemaExpr.cpp
7146

I think DiagnoseTautologicalIsConstantEvaluated might be confusing without context, suggesting another name

clang/lib/Sema/SemaStmt.cpp
938–944

I think that might be somewhat nicer (make sure to run clang-format :))

This looks good to me modulo nitpicks.
When you land that, please make an issue on github for the missing narrowing warning, it seems important.

I'll wait before approving in case @aaron.ballman spot things i missed

I've already prepared another patch to fix the narrowing issue. Should I add the diff to this patch or open another revision?

FWIW, I believe const bool c = std::is_constant_evaluated(); this particular case should also be diagnosed because it could be a common mistake as was mentioned in https://github.com/llvm/llvm-project/issues/43760#issuecomment-981022686.

This looks good to me modulo nitpicks.
When you land that, please make an issue on github for the missing narrowing warning, it seems important.

I'll wait before approving in case @aaron.ballman spot things i missed

I've already prepared another patch to fix the narrowing issue. Should I add the diff to this patch or open another revision?

I think merging the two patches would make sense, yes, that way we avoid regressions

FWIW, I believe const bool c = std::is_constant_evaluated(); this particular case should also be diagnosed because it could be a common mistake as was mentioned in https://github.com/llvm/llvm-project/issues/43760#issuecomment-981022686.

i think this is less pressing. doing that later (ie, for clang 18) is more reasonable. There are probably some interesting edge cases with constant initialization

hazohelet updated this revision to Diff 543189.Jul 22 2023, 8:06 AM
hazohelet marked 3 inline comments as done.
hazohelet edited the summary of this revision. (Show Details)

Address comments from Corentin

  • NFC stylistic changes
  • Bring back narrowing warning on constexpr variables to avoid regression
  • Newly diagnose narrowing conversions on constant-evaluated initializers in general (e..g immediate function context, and constexpr variable templates)
cor3ntin added inline comments.Jul 23 2023, 1:30 AM
clang/test/SemaCXX/vartemplate-lambda.cpp
17

This also looks like a regression.

The current error is much clearer, can you investigate?

<source>:3:22: error: constexpr variable 't<int>' must be initialized by a constant expression
    3 |   static constexpr T t = [](int f = T(7)){return f;}();
      |                      ^   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
<source>:6:12: note: in instantiation of static data member 'S::t<int>' requested here
    6 | int a = S::t<int>;
      |            ^
<source>:3:26: note: non-literal type 'S::(lambda at <source>:3:26)' cannot be used in a constant expression
    3 |   static constexpr T t = [](int f = T(7)){return f;}();
      |                          ^

Why do we emt 2 errors instead of a single note? Here the error is that the initializer is not a constant expression, everything else should be notes.

hazohelet added inline comments.Jul 23 2023, 4:26 AM
clang/test/SemaCXX/vartemplate-lambda.cpp
17

"lambda cannot be in constant expression" error is emitted from Sema against lambda expressions in constant-evaluated contexts in C++14 mode, and the note is emitted from constexpr evaluator.
The Sema-side error is emitted twice because it is emitted both before/after instantiation.
We can suppress one of them by ignoring it when sema is instantiating variable template initializer.
Or we can completely suppress this Sema error against initializers to avoid duplicate errors from Sema and constexpr evaluator.
I think "lambda cannot be in constant expression" Sema error is more understandable than the constexpr evaluator note "non-literal type cannot be in constant expression", so I think it is ok to keep one Sema error here.

cor3ntin added inline comments.Jul 24 2023, 12:16 AM
clang/test/SemaCXX/vartemplate-lambda.cpp
17

So maybe the issue is that we are not making the declaration invalid in sema when we get this error? Can you look into it?
any opinion @aaron.ballman

hazohelet updated this revision to Diff 543510.Jul 24 2023, 6:38 AM
hazohelet marked 4 inline comments as done.

Address comments from Corentin

  • When emitting "lambda expression may not appear inside of a constant expression" error from Sema::PopExpressionEvaluationContext, mark the variable declaration as invalid if the popped context is initializer evaluation context.
  • This stops constexpr evaluator from evaluating the initializer and emitting duplicate errors.
hazohelet added inline comments.Jul 24 2023, 6:49 AM
clang/test/SemaCXX/vartemplate-lambda.cpp
17

I updated the patch to keep a single sema error here and stop constant interpreter from evaluating the initializer by marking declaration invalid. I like having one sema error here.

cor3ntin added inline comments.Jul 28 2023, 8:14 AM
clang/test/SemaCXX/vartemplate-lambda.cpp
17

I'll wait a bit to approve because I'd like a second opinion, but otherwise I think we are in pretty good shape in this patch!

Fixed libc++ tests (NFC)
There's no change in clang codes.
This change should not change the semantics of all relevant codes because this only replaces tautologically-false uses of is_constant_evaluated() with false through macros.
These tautologically-false uses in tests are in if-condition expression inside non-constexpr functions.

  • Make __libcpp_is_constant_evaluated() in libcxx/include/__type_traits/is_constant_evaluated.h return false instead of __builtin_is_constant_evaluated() in pre-C++11 mode.
  • Set TEST_IS_CONSTANT_EVALUATED macro to false in pre-C++11 mode.
  • Remove all tautologically-false use of TEST_IS_CONSTANT_EVALUATED macro in tests by defining 4 more macros TEST_IS_CONSTANT_EVALUATED_CXX{14, 17, 20, 23} and replacing former use of TEST_IS_CONSTANT_EVALUATED with the corresponding one.

(Git diff context is not complete in order to fit within ~8MB patch file size limit of Phabricator)

@ldionne @philnik @Mordante
Is this way of fixing libc++ tests OK from libc++ side? If there's better way to fix them I am happy to follow it.

I've noticed several shortcoming/limitations of this patch.

  1. Function arguments: When we parse the arguments to a function call, the callee isn't still resolved, so we don't know whether it's consteval. If the callee is consteval, its argument is known to be constant-evaluated regardless of its surrounding evaluation context.

e.g.

consteval int ce(int n){ return n;};
int f() {
	int a = ce(__builtin_is_constant_evaluated()); // tautologically true
}
  1. init-statement of constexpr-if:

The condition is required to be a constant expression, but the init-stmt before it isn't.
e.g. Given if constexpr (InitStmt; Cond) {}, InitStmt is evaluated in the surrounding evaluation context.
If the init-statement is a declaration we can push initializer evaluation context like we do in other variables. However, if the init-statement is an expression, when clang parses the expression, clang doesn't know whether it is parsing the init-statement or the condition. https://github.com/llvm/llvm-project/blob/e3c57fdd8439ba82c67347629a3c66f293e1f3d0/clang/lib/Parse/ParseExprCXX.cpp#L2102

If we are to handle these cases with perfection, we need to defer warning emissions, but I'm skeptical about the value we obtain from this effort.

For the first problem, we can make IsRuntimeEvaluated false while parsing arguments to avoid emitting incorrect diagnostics although it makes false negatives on tautologically-false uses in arguments.
The second problem is difficult because we cannot avoid incorrect diagnostics in InitStmt expression of constexpr-if with a simple fix. But I think it's acceptable for the time being because it would be a pretty rare case to use is_constant_evaluated there.

@cor3ntin
BTW, can I separate the initializer evaluation context bug fix part to make another PR? Another patch I'm working on locally also suffers from the evaluation context bug, and I want to land that part relatively soon.
Also, it might be nice to separate the NFC libc++ test modifications I recently uploaded because it's too much despite being NFC. Or we can turn off this tautology warning against macros. Do you think it's reasonable?

hazohelet updated this revision to Diff 548197.Aug 8 2023, 6:55 AM
  • Tentatively disable the warning on macros so as not to make a fuss in libc++ tests.
  • Fixed incorrect output in arguments.
  • Fixed incorrect output in declaration of init-statement of constexpr-if condition.
  • Added tests with FIXMEs about the limitation I mentioned before.
hazohelet updated this revision to Diff 549203.Aug 10 2023, 5:06 PM

Resolved the constexpr-if init-statement expression evaluation context problem.

cjdb added a subscriber: cjdb.Aug 11 2023, 10:18 AM
cjdb added inline comments.
clang/test/SemaCXX/warn-constant-evaluated-constexpr.cpp
38

This should also generate a fix-it hint, since we can automate the fix.

clang/test/SemaCXX/warn-tautological-meta-constant.cpp
19

I'm not a fan of this diagnostic text: it doesn't offer insight into what's gone wrong or provide actionable feedback on how to fix the code.

Thanks for the feedback.

clang/test/SemaCXX/warn-constant-evaluated-constexpr.cpp
38

I think it's reasonable to show fix-it hint to remove constexpr here.
For that we need to store the source range of constexpr in evaluation context record if it's constexpr-if condition.

clang/test/SemaCXX/warn-tautological-meta-constant.cpp
19

I agree that the current wording (... will always evaluate to true in this context) is not great, but I'm not sure how to improve it because the reason to see this diagnostic would mostly be a misunderstanding of the C++ const semantics.
At least, if the appearance of is_constant_evaluated is in the condition of constexpr-if, probably we can say they should remove constexpr.
CC @aaron.ballman

hazohelet updated this revision to Diff 550881.Aug 16 2023, 2:03 PM
hazohelet marked an inline comment as done.

Address comments from Chris

  • Generate fix-it hint to remove constexpr in constexpr-if

Added SourceLocation field to Sema that saves the location of constexpr in constexpr-if.

hazohelet updated this revision to Diff 550899.Aug 16 2023, 3:07 PM

rebase to upstream to run ci

Mordante added inline comments.Aug 19 2023, 4:41 AM
libcxx/include/__type_traits/is_constant_evaluated.h
34

Why is this needed? Does this mean the builtin will fail in C++03 mode?

libcxx/test/std/utilities/meta/meta.const.eval/is_constant_evaluated.verify.cpp
27

We're not in the business of testing the compiler though. Taking a closer look, I'm not actually sure why this test exists at all. It doesn't seem like it tests anything useful w.r.t. the library. This has been added in 2fc5a78, but there the warning isn't checked, so that was clearly not the original intention.

I agree. But to me this test tests whether

hazohelet added inline comments.Aug 19 2023, 4:52 AM
libcxx/include/__type_traits/is_constant_evaluated.h
34

__libcpp_is_constant_evaluated always returns false under C++03 or earlier, where constexpr isn't available. This is a fix to run libc++ tests without seeing warnings from clang with this patch.
(Live demo: https://godbolt.org/z/oszebM5o5)

Mordante added inline comments.Aug 19 2023, 5:12 AM
libcxx/include/__type_traits/is_constant_evaluated.h
34

I see can you add a comment in that regard? To me it looks like the builtin fails in C++03, where returning false indeed is always the right answer.

hazohelet updated this revision to Diff 551744.Aug 19 2023, 5:51 AM
hazohelet marked 2 inline comments as done.

Address comments from Mark

  • Added comment about the need of macro use in libc++ include file.
philnik added inline comments.Aug 19 2023, 8:25 AM
libcxx/include/__type_traits/is_constant_evaluated.h
34

https://godbolt.org/z/bafeeY7b7 shows that __builtin_is_constant_evaluated() doesn't always return false in C++03, so this change seems wrong to me.

hazohelet marked an inline comment as done.Aug 19 2023, 8:48 AM
hazohelet added inline comments.
libcxx/include/__type_traits/is_constant_evaluated.h
34

This is NFC.
The problem is that __libcpp_is_constant_evaluated() has different semantics from __builtin_is_constant_evaluated() in C++03.
The cause of this tautological-falsity here is whether __libcpp_is_constant_evaluated() is constexpr-qualified or not.
_LIBCPP_CONSTEXPR macro expands to constexpr since C++11, but in pre-C++11 mode, it expands to nothing.
So in C++03 mode this function is something like bool __libcpp_is_constant_evaluated() { return __builtin_is_constant_evaluated(); }, where it is tautologically-false because it is not constexpr-qualified.

As you mentioned, __builtin_is_constant_evaluated does not change its semantics depending on C++ versions.
However, __libcpp_is_constant_evaluated() always returns false in C++03 as in https://godbolt.org/z/oszebM5o5

@philnik @Mordante were the libc++ concerns addressed?

ldionne accepted this revision.Sep 7 2023, 9:09 AM

The libc++ changes are reasonable. We could discuss more around how to handle C++03 in __libcpp_is_constant_evaluated but I don't think it's worth delaying this patch for that.

libcxx/include/__type_traits/is_constant_evaluated.h
34

This explanation makes sense to me. I think it's reasonable to return false unconditionally in C++03 mode.

This revision is now accepted and ready to land.Sep 7 2023, 9:09 AM
cor3ntin accepted this revision.Sep 7 2023, 10:01 AM

LGTM, thanks!

hazohelet updated this revision to Diff 556946.Sep 18 2023, 7:03 AM

After discussion in https://github.com/llvm/llvm-project/pull/66222, I noticed I had misunderstood the recursiveness of constant-evaluated context.
I was pushing constant-evaluating on the lambda body when it appears inside constant-evaluated context. This was causing false positive in

constexpr auto cexpr_lambda = []() {
  return __builtin_is_constant_evaluated();
}

I fixed this by stopping pushing constant-evaluated context (See SemaLambda.cpp). Instead, I am pushing immediate-function context when the outer evaluation context is immediate function context as well as when it's consteval lambda so that we can warn on

consteval void f() {
	auto lam = []() { return __builtin_is_constant_evaluated(); };
}

@cor3ntin
Should this patch wait for https://github.com/llvm/llvm-project/pull/66222?

cor3ntin accepted this revision.Sep 21 2023, 12:47 AM

@hazohelet Nah, feel free to merge that first! Thanks

This revision was landed with ongoing or failed builds.Sep 26 2023, 5:27 PM
This revision was automatically updated to reflect the committed changes.
hans added a subscriber: hans.Sep 27 2023, 7:46 AM

We saw a new warning in Chromium after this, and I wasn't sure if that's intentional:

#include <limits>

template <typename T> float foo(T input) {
  if (sizeof(T) > 2) {
    return 42;
  } else {
    constexpr float inverseMax = 1.0f / std::numeric_limits<T>::max();
    return input * inverseMax;
  }
}

float f() {
  return foo(1);
}
$ build/bin/clang -c /tmp/a.cc
/tmp/a.cc:7:41: warning: implicit conversion from 'int' to 'float' changes value from 2147483647 to 2147483648 [-Wimplicit-const-int-float-conversion]
    7 |     constexpr float inverseMax = 1.0f / std::numeric_limits<T>::max();
      |                                       ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/tmp/a.cc:13:10: note: in instantiation of function template specialization 'foo<int>' requested here
   13 |   return foo(1);
      |          ^
1 warning generated.

Note that the branch with the conversion can be determined to not be taken based on the template argument.

Using if constexpr suppresses the warning, but I'm not sure if that should really be necessary?

(Our tracking bug is https://crbug.com/1487142)

struct S {
  template <typename F>
  constexpr bool a(F&& f) const {
    // This works previously in clang and on gcc
    return f(1); // no matching function for call to object of type ...
  }
};

template <typename>
struct S1 {
  void f() {
    auto test = [] (auto) {
      S s;
      // Remove this constexpr and it compiles fine.
      constexpr auto f = [](auto&&) { return true; };
      return s.a(f);
    };
    test(1);
  }
};

void a() {
  S1<int> s;
  s.f();
}

The following doesn't compile after this change (I've verified that reverting makes the issue go away). This code also compiles fine on gcc. Would you mind taking a look?

Hi, I'm afraid this breaks valid code with false diagnostics, I need to revert.

Specifically it complains about missing lambda captures for values that are in fact arguments to the lambda:

template <typename F>
constexpr void inner(F f) {
  return f(0);
}

template <typename F>
void outer(F f) {
  inner([](auto I) {
    constexpr auto kFuncs = [](F& g) { return g(); };
    (void) kFuncs;
  });
}

void entry() {
  outer([&] {});
}
$ bin/clang++ -fsyntax-only ~/test.cc

/usr/local/google/home/sammccall/test.cc:9:47: error: variable 'g' cannot be implicitly captured in a lambda with no capture-default specified
    9 |     constexpr auto kFuncs = [](F& g) { return g(); };
      |                                               ^
/usr/local/google/home/sammccall/test.cc:9:29: note: while substituting into a lambda expression here
    9 |     constexpr auto kFuncs = [](F& g) { return g(); };
      |                             ^
/usr/local/google/home/sammccall/test.cc:3:10: note: in instantiation of function template specialization 'outer((lambda at /usr/local/google/home/sammccall/test.cc:15:9))::(anonymous class)::operator()<int>' requested here
    3 |   return f(0);
      |          ^
/usr/local/google/home/sammccall/test.cc:8:3: note: in instantiation of function template specialization 'inner<(lambda at /usr/local/google/home/sammccall/test.cc:8:9)>' requested here
    8 |   inner([](auto I) {
      |   ^
/usr/local/google/home/sammccall/test.cc:15:3: note: in instantiation of function template specialization 'outer<(lambda at /usr/local/google/home/sammccall/test.cc:15:9)>' requested here
   15 |   outer([&] {});
      |   ^
/usr/local/google/home/sammccall/test.cc:9:35: note: 'g' declared here
    9 |     constexpr auto kFuncs = [](F& g) { return g(); };
      |                                   ^
/usr/local/google/home/sammccall/test.cc:9:29: note: lambda expression begins here
    9 |     constexpr auto kFuncs = [](F& g) { return g(); };
      |                             ^
/usr/local/google/home/sammccall/test.cc:9:30: note: capture 'g' by value
    9 |     constexpr auto kFuncs = [](F& g) { return g(); };
      |                              ^
      |                              g
/usr/local/google/home/sammccall/test.cc:9:30: note: capture 'g' by reference
    9 |     constexpr auto kFuncs = [](F& g) { return g(); };
      |                              ^
      |                              &g
/usr/local/google/home/sammccall/test.cc:9:30: note: default capture by value
    9 |     constexpr auto kFuncs = [](F& g) { return g(); };
      |                              ^
      |                              =
/usr/local/google/home/sammccall/test.cc:9:30: note: default capture by reference
    9 |     constexpr auto kFuncs = [](F& g) { return g(); };
      |                              ^
      |                              &
1 error generated.

We saw a new warning in Chromium after this, and I wasn't sure if that's intentional:

#include <limits>

template <typename T> float foo(T input) {
  if (sizeof(T) > 2) {
    return 42;
  } else {
    constexpr float inverseMax = 1.0f / std::numeric_limits<T>::max();
    return input * inverseMax;
  }
}

float f() {
  return foo(1);
}
$ build/bin/clang -c /tmp/a.cc
/tmp/a.cc:7:41: warning: implicit conversion from 'int' to 'float' changes value from 2147483647 to 2147483648 [-Wimplicit-const-int-float-conversion]
    7 |     constexpr float inverseMax = 1.0f / std::numeric_limits<T>::max();
      |                                       ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/tmp/a.cc:13:10: note: in instantiation of function template specialization 'foo<int>' requested here
   13 |   return foo(1);
      |          ^
1 warning generated.

Note that the branch with the conversion can be determined to not be taken based on the template argument.

Using if constexpr suppresses the warning, but I'm not sure if that should really be necessary?

(Our tracking bug is https://crbug.com/1487142)

It's unintended, but the cause is that I started using Sema::Diag instead of Sema::DiagRuntimeBehavior, in narrowing check for constexpr variable initializers, and thus lost the reachability analysis. It will be a simple fix, but would involve some adaptations to Sema::DiagIfReachable. I'll fix it after other concerns are resolved.

Thanks for the report about the errors in lambdas. The culprit is clang/lib/Sema/SemaTemplateInstantiateDecl.cpp. I am pushing constant-evaluated context if the instantiated variable is constexpr variable, and somehow it causes those errors. I'll take deeper look into them tomorrow.

Thank you for the revert!

This looks like a general issue of clang on template deduction in constant-evaluated context (https://godbolt.org/z/zTro7Ycfa). Pushing constant-evaluated context against initializer of instantiated constexpr variables made this bug appear in broader scenarios.
I can circumvent this problem by not pushing constant-evaluated context on instantiated constexpr variable initializers, but that in turn would force me to do tricky workaround in warnings on narrowing conversions in global constexpr variable templates.

@cor3ntin
The constant-evaluated-context miscompilation bisected to https://reviews.llvm.org/D140554
Is this the intended result of that patch?
I'm at lost how to proceed further on this patch. The only way looks like some workarounds in narrowing conversion warning mechanism, but is it acceptable?