Page MenuHomePhabricator

Implement P1937 consteval in unevaluated contexts
ClosedPublic

Authored by cor3ntin on Jul 19 2021, 12:35 PM.

Details

Summary

In an unevaluated contexts, consteval functions
should not be immediately evaluated.

Diff Detail

Unit TestsFailed

TimeTest
2,880 msx64 debian > libarcher.critical::critical.c
Script: -- : 'RUN: at line 15'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -g -O1 -fsanitize=thread -I /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests -I /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/runtime/src -L /var/lib/buildkite-agent/builds/llvm-project/build/lib -Wl,-rpath,/var/lib/buildkite-agent/builds/llvm-project/build/lib /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests/critical/critical.c -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/critical/Output/critical.c.tmp -latomic && env TSAN_OPTIONS='ignore_noninstrumented_modules=0:ignore_noninstrumented_modules=1' /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/critical/Output/critical.c.tmp 2>&1 | tee /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/critical/Output/critical.c.tmp.log | /var/lib/buildkite-agent/builds/llvm-project/build/./bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests/critical/critical.c
2,840 msx64 debian > libarcher.races::critical-unrelated.c
Script: -- : 'RUN: at line 13'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -g -O1 -fsanitize=thread -I /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests -I /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/runtime/src -L /var/lib/buildkite-agent/builds/llvm-project/build/lib -Wl,-rpath,/var/lib/buildkite-agent/builds/llvm-project/build/lib /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests/races/critical-unrelated.c -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/critical-unrelated.c.tmp -latomic && env TSAN_OPTIONS='ignore_noninstrumented_modules=0:ignore_noninstrumented_modules=1' /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests/deflake.bash /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/critical-unrelated.c.tmp 2>&1 | tee /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/critical-unrelated.c.tmp.log | /var/lib/buildkite-agent/builds/llvm-project/build/./bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests/races/critical-unrelated.c
3,080 msx64 debian > libarcher.races::lock-nested-unrelated.c
Script: -- : 'RUN: at line 13'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -g -O1 -fsanitize=thread -I /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests -I /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/runtime/src -L /var/lib/buildkite-agent/builds/llvm-project/build/lib -Wl,-rpath,/var/lib/buildkite-agent/builds/llvm-project/build/lib /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests/races/lock-nested-unrelated.c -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/lock-nested-unrelated.c.tmp -latomic && env TSAN_OPTIONS='ignore_noninstrumented_modules=0:ignore_noninstrumented_modules=1' /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests/deflake.bash /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/lock-nested-unrelated.c.tmp 2>&1 | tee /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/lock-nested-unrelated.c.tmp.log | /var/lib/buildkite-agent/builds/llvm-project/build/./bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests/races/lock-nested-unrelated.c
3,020 msx64 debian > libarcher.races::lock-unrelated.c
Script: -- : 'RUN: at line 13'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -g -O1 -fsanitize=thread -I /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests -I /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/runtime/src -L /var/lib/buildkite-agent/builds/llvm-project/build/lib -Wl,-rpath,/var/lib/buildkite-agent/builds/llvm-project/build/lib /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests/races/lock-unrelated.c -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/lock-unrelated.c.tmp -latomic && env TSAN_OPTIONS='ignore_noninstrumented_modules=0:ignore_noninstrumented_modules=1' /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests/deflake.bash /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/lock-unrelated.c.tmp 2>&1 | tee /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/lock-unrelated.c.tmp.log | /var/lib/buildkite-agent/builds/llvm-project/build/./bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests/races/lock-unrelated.c
3,320 msx64 debian > libarcher.races::parallel-simple.c
Script: -- : 'RUN: at line 13'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -g -O1 -fsanitize=thread -I /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests -I /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/runtime/src -L /var/lib/buildkite-agent/builds/llvm-project/build/lib -Wl,-rpath,/var/lib/buildkite-agent/builds/llvm-project/build/lib /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests/races/parallel-simple.c -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/parallel-simple.c.tmp -latomic && env TSAN_OPTIONS='ignore_noninstrumented_modules=0:ignore_noninstrumented_modules=1' /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests/deflake.bash /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/parallel-simple.c.tmp 2>&1 | tee /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/parallel-simple.c.tmp.log | /var/lib/buildkite-agent/builds/llvm-project/build/./bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests/races/parallel-simple.c
View Full Test Results (16 Failed)

Event Timeline

cor3ntin requested review of this revision.Jul 19 2021, 12:35 PM
cor3ntin created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJul 19 2021, 12:35 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Note that I think this was partially implemented as part of https://reviews.llvm.org/D74130 - which has not progressed since October.
This PR implements P1937 only

aaron.ballman added inline comments.Jul 20 2021, 12:00 PM
clang/test/SemaCXX/cxx2a-consteval.cpp
603
608
612

Here's an interesting test case:

#include <typeinfo>

struct S {
  virtual void f();
};

struct D : S {
  void f() override;
};

consteval S *get_s() { return nullptr; }

void func() {
  (void)typeid(*get_s());
}

typeid still needs to evaluate its operand (due to the polymorphic return type of *get_s()), and so you should get a diagnostic about evaluating the side effects by calling get_s(). I think this then runs into https://eel.is/c++draft/expr.const#13.sentence-3 and we should diagnose?

cor3ntin updated this revision to Diff 360235.Jul 20 2021, 12:36 PM

Fix formatting

cor3ntin added inline comments.Jul 20 2021, 12:38 PM
clang/test/SemaCXX/cxx2a-consteval.cpp
612

Not sure!
Also, in the context of this pr, the question is also whether decltype(typeid(*get_s())) should be ill-formed I think

cor3ntin added inline comments.Jul 20 2021, 3:07 PM
clang/test/SemaCXX/cxx2a-consteval.cpp
612

Actually, I'm reading the wording again and I really don't know anymore.
get_s() is a constant expression, right?
*get_s() is not, I think but is that relevant here

I played with a bunch of things in the code but the more I look at it the less I'm convinced an action is needed.

aaron.ballman added inline comments.Jul 21 2021, 7:17 AM
clang/test/SemaCXX/cxx2a-consteval.cpp
612

The changes to Sema::CheckForImmediateInvocation() to check for an unevaluated context and https://eel.is/c++draft/expr.const#13.sentence-3 that say an immediate invocation shall be a constant expression are what got me thinking about this code snippet in the first place. I was trying to decide whether isUnevaluatedContext() is correct or not because with typeid, it is potentially evaluated (so sometimes it's unevaluated).

Interestingly, everyone comes up with a different answer: https://godbolt.org/z/TqjGh1he6 and I don't (yet) know who is correct.

cor3ntin added inline comments.Jul 23 2021, 9:32 AM
clang/test/SemaCXX/cxx2a-consteval.cpp
612

@rsmith Can you enlighten us here?
My take is that get_s() is a constant expression and therefore an immediate invocation. independently of what *get_s() does but I'm not sure if that's a correct reading.

Thanks a lot!

@rsmith Ping again! Thanks

rsmith added inline comments.Aug 5 2021, 12:46 PM
clang/test/SemaCXX/cxx2a-consteval.cpp
612

There are a few different cases here and I don't think any compiler is getting them all right.

struct S {
  void f();
};
struct T {
  virtual void f();
};

consteval S *null_s() { return nullptr; }
consteval S *make_s() { return new S; }
consteval T *null_t() { return nullptr; }
consteval T *make_s() { return new T; }

void func() {
  (void)typeid(*null_s()); // #1
  (void)typeid(*make_s()); // #2
  (void)typeid(*null_t()); // #3
  (void)typeid(*make_t()); // #4
}

Here, #3 and #4 pass an lvalue of polymorphic class type to typeid, so the arguments to those typeids are potentially evaluated. #1 and #2 pass an lvalue of non-polymorphic class type, so those arguments are unevaluated operands. So we have two immediate invocations: the null_t() call and the make_t() call.

Lines #1 and #2 are valid because there's no immediate invocation to check. (Clang and GCC get this wrong and reject #2.)
Line #3 is valid because the call to null_t() is a constant expression. (MSVC gets this wrong for reasons I don't understand.)
Line #4 is ill-formed because the call to make_t() is not a constant expression, because it returns a heap allocation.

The way we handle typeid in general is to parse its operand as an unevaluated operand, and then later TransformToPotentiallyEvaluated if we find it's a glvalue of or pointer to polymorphic class type. If you find the above testcase isn't handled correctly, you may need to make some changes in TransformToPotentiallyEvaluated to trigger the proper rebuilding. (You might need to force it to transform CallExprs that refer to consteval functions even if nothing within them have changed, for example.)

cor3ntin updated this revision to Diff 364606.Aug 5 2021, 1:56 PM
  • Add tests for typeid

All the tests suggested by Richard pass as expected without having to
modify the implementation of type id itself!

cor3ntin marked 2 inline comments as done.Aug 5 2021, 2:02 PM
cor3ntin added inline comments.
clang/test/SemaCXX/cxx2a-consteval.cpp
612

Thanks, this was super useful!

I think clang gets everything right now - I added your scenarios as tests with the other typeid tests

aaron.ballman added inline comments.Aug 6 2021, 5:52 AM
clang/test/CXX/basic/basic.def.odr/p2-typeid.cpp
53–62

Cleaning up the test a bit to not use relative locations, should be NFC.

clang/test/SemaCXX/cxx2a-consteval.cpp
11

Looks like a bunch of unintended whitespace changes snuck in.

608–609
cor3ntin updated this revision to Diff 364765.Aug 6 2021, 6:03 AM

Remove WS changes and cleanup tests

cor3ntin updated this revision to Diff 364778.Aug 6 2021, 6:35 AM

Changing release version to clang 14 in cxx_status

This revision is now accepted and ready to land.Aug 6 2021, 6:38 AM
aaron.ballman closed this revision.Aug 6 2021, 7:30 AM

I have commit on your behalf in 131b4620ee7847102479f399ce3e35a3c1cb5461, thank you for the fix!