Skip to content

[clang][bytecode] Fix diagnosing reads from temporaries #106868

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 1, 2024

Conversation

tbaederr
Copy link
Contributor

Fix the DeclID not being set in global temporaries and use the same strategy for deciding if a temporary is readable as the current interpreter.

Fix the DeclID not being set in global temporaries and use the same
strategy for deciding if a temporary is readable as the current
interpreter.
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Aug 31, 2024
@llvmbot
Copy link
Member

llvmbot commented Aug 31, 2024

@llvm/pr-subscribers-clang

Author: Timm Baeder (tbaederr)

Changes

Fix the DeclID not being set in global temporaries and use the same strategy for deciding if a temporary is readable as the current interpreter.


Full diff: https://github.com/llvm/llvm-project/pull/106868.diff

3 Files Affected:

  • (modified) clang/lib/AST/ByteCode/Compiler.cpp (+2-1)
  • (modified) clang/lib/AST/ByteCode/Interp.cpp (+13-8)
  • (modified) clang/test/AST/ByteCode/references.cpp (+11-2)
diff --git a/clang/lib/AST/ByteCode/Compiler.cpp b/clang/lib/AST/ByteCode/Compiler.cpp
index dced9ea3493732..1ddaa5bd41df75 100644
--- a/clang/lib/AST/ByteCode/Compiler.cpp
+++ b/clang/lib/AST/ByteCode/Compiler.cpp
@@ -3772,7 +3772,6 @@ VarCreationState Compiler<Emitter>::visitVarDecl(const VarDecl *VD,
 
     auto initGlobal = [&](unsigned GlobalIndex) -> bool {
       assert(Init);
-      DeclScope<Emitter> LocalScope(this, VD);
 
       if (VarT) {
         if (!this->visit(Init))
@@ -3796,6 +3795,8 @@ VarCreationState Compiler<Emitter>::visitVarDecl(const VarDecl *VD,
       return this->emitPopPtr(Init);
     };
 
+    DeclScope<Emitter> LocalScope(this, VD);
+
     // We've already seen and initialized this global.
     if (std::optional<unsigned> GlobalIndex = P.getGlobal(VD)) {
       if (P.getPtrGlobal(*GlobalIndex).isInitialized())
diff --git a/clang/lib/AST/ByteCode/Interp.cpp b/clang/lib/AST/ByteCode/Interp.cpp
index 42012767c22332..3d75f2ce7183a6 100644
--- a/clang/lib/AST/ByteCode/Interp.cpp
+++ b/clang/lib/AST/ByteCode/Interp.cpp
@@ -181,16 +181,21 @@ static bool CheckTemporary(InterpState &S, CodePtr OpPC, const Pointer &Ptr,
     if (!Ptr.isStaticTemporary())
       return true;
 
-    if (Ptr.getDeclDesc()->getType().isConstQualified())
+    const auto *MTE = dyn_cast_if_present<MaterializeTemporaryExpr>(
+        Ptr.getDeclDesc()->asExpr());
+    if (!MTE)
       return true;
 
-    if (S.P.getCurrentDecl() == ID)
-      return true;
-
-    const SourceInfo &E = S.Current->getSource(OpPC);
-    S.FFDiag(E, diag::note_constexpr_access_static_temporary, 1) << AK;
-    S.Note(Ptr.getDeclLoc(), diag::note_constexpr_temporary_here);
-    return false;
+    // FIXME(perf): Since we do this check on every Load from a static
+    // temporary, it might make sense to cache the value of the
+    // isUsableInConstantExpressions call.
+    if (!MTE->isUsableInConstantExpressions(S.getASTContext()) &&
+        Ptr.block()->getEvalID() != S.Ctx.getEvalID()) {
+      const SourceInfo &E = S.Current->getSource(OpPC);
+      S.FFDiag(E, diag::note_constexpr_access_static_temporary, 1) << AK;
+      S.Note(Ptr.getDeclLoc(), diag::note_constexpr_temporary_here);
+      return false;
+    }
   }
   return true;
 }
diff --git a/clang/test/AST/ByteCode/references.cpp b/clang/test/AST/ByteCode/references.cpp
index 9a790dc75d7308..7c1dccb1f9e341 100644
--- a/clang/test/AST/ByteCode/references.cpp
+++ b/clang/test/AST/ByteCode/references.cpp
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 -fexperimental-new-constant-interpreter -verify %s
-// RUN: %clang_cc1 -verify=ref %s
+// RUN: %clang_cc1 -fexperimental-new-constant-interpreter -verify=expected,both %s
+// RUN: %clang_cc1 -verify=ref,both %s
 
 
 constexpr int a = 10;
@@ -135,3 +135,12 @@ static_assert(nonextended_string_ref[2] == '\0', "");
 /// but taking its address is.
 int &&A = 12;
 int arr[!&A];
+
+namespace Temporaries {
+  struct A { int n; };
+  struct B { const A &a; };
+  const B j = {{1}}; // both-note {{temporary created here}}
+
+  static_assert(j.a.n == 1, "");  // both-error {{not an integral constant expression}} \
+                                  // both-note {{read of temporary is not allowed in a constant expression outside the expression that created the temporary}}
+}

@tbaederr tbaederr changed the title [clang][bytecode] Fix diagnosting reads from temporaries [clang][bytecode] Fix diagnosing reads from temporaries Sep 1, 2024
@tbaederr tbaederr merged commit e4f3b56 into llvm:main Sep 1, 2024
9 of 11 checks passed
@llvm-ci
Copy link
Collaborator

llvm-ci commented Sep 1, 2024

LLVM Buildbot has detected a new failure on builder sanitizer-x86_64-linux running on sanitizer-buildbot1 while building clang at step 2 "annotate".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/66/builds/3334

Here is the relevant piece of the build log for the reference
Step 2 (annotate) failure: 'python ../sanitizer_buildbot/sanitizers/zorg/buildbot/builders/sanitizers/buildbot_selector.py' (failure)
...
[378/384] Generating Fuzzer-x86_64-Test
[379/384] Generating MSAN_INST_GTEST.gtest-all.cc.x86_64.o
[380/384] Generating MSAN_INST_TEST_OBJECTS.msan_test.cpp.x86_64-with-call.o
[381/384] Generating Msan-x86_64-with-call-Test
[382/384] Generating MSAN_INST_TEST_OBJECTS.msan_test.cpp.x86_64.o
[383/384] Generating Msan-x86_64-Test
[383/384] Running compiler_rt regression tests
llvm-lit: /home/b/sanitizer-x86_64-linux/build/llvm-project/llvm/utils/lit/lit/main.py:72: note: The test suite configuration requested an individual test timeout of 0 seconds but a timeout of 900 seconds was requested on the command line. Forcing timeout to be 900 seconds.
-- Testing: 4592 of 10298 tests, 88 workers --
Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.
FAIL: SanitizerCommon-lsan-i386-Linux :: Linux/soft_rss_limit_mb_test.cpp (3032 of 4592)
******************** TEST 'SanitizerCommon-lsan-i386-Linux :: Linux/soft_rss_limit_mb_test.cpp' FAILED ********************
Exit Code: 1

Command Output (stderr):
--
RUN: at line 2: /home/b/sanitizer-x86_64-linux/build/build_default/./bin/clang  --driver-mode=g++ -gline-tables-only -fsanitize=leak  -m32 -funwind-tables  -I/home/b/sanitizer-x86_64-linux/build/llvm-project/compiler-rt/test -ldl -O2 /home/b/sanitizer-x86_64-linux/build/llvm-project/compiler-rt/test/sanitizer_common/TestCases/Linux/soft_rss_limit_mb_test.cpp -o /home/b/sanitizer-x86_64-linux/build/build_default/runtimes/runtimes-bins/compiler-rt/test/sanitizer_common/lsan-i386-Linux/Linux/Output/soft_rss_limit_mb_test.cpp.tmp
+ /home/b/sanitizer-x86_64-linux/build/build_default/./bin/clang --driver-mode=g++ -gline-tables-only -fsanitize=leak -m32 -funwind-tables -I/home/b/sanitizer-x86_64-linux/build/llvm-project/compiler-rt/test -ldl -O2 /home/b/sanitizer-x86_64-linux/build/llvm-project/compiler-rt/test/sanitizer_common/TestCases/Linux/soft_rss_limit_mb_test.cpp -o /home/b/sanitizer-x86_64-linux/build/build_default/runtimes/runtimes-bins/compiler-rt/test/sanitizer_common/lsan-i386-Linux/Linux/Output/soft_rss_limit_mb_test.cpp.tmp
RUN: at line 5: env LSAN_OPTIONS=soft_rss_limit_mb=220:quarantine_size=1:allocator_may_return_null=1      /home/b/sanitizer-x86_64-linux/build/build_default/runtimes/runtimes-bins/compiler-rt/test/sanitizer_common/lsan-i386-Linux/Linux/Output/soft_rss_limit_mb_test.cpp.tmp 2>&1 | FileCheck /home/b/sanitizer-x86_64-linux/build/llvm-project/compiler-rt/test/sanitizer_common/TestCases/Linux/soft_rss_limit_mb_test.cpp -check-prefix=CHECK_MAY_RETURN_1
+ env LSAN_OPTIONS=soft_rss_limit_mb=220:quarantine_size=1:allocator_may_return_null=1 /home/b/sanitizer-x86_64-linux/build/build_default/runtimes/runtimes-bins/compiler-rt/test/sanitizer_common/lsan-i386-Linux/Linux/Output/soft_rss_limit_mb_test.cpp.tmp
+ FileCheck /home/b/sanitizer-x86_64-linux/build/llvm-project/compiler-rt/test/sanitizer_common/TestCases/Linux/soft_rss_limit_mb_test.cpp -check-prefix=CHECK_MAY_RETURN_1
/home/b/sanitizer-x86_64-linux/build/llvm-project/compiler-rt/test/sanitizer_common/TestCases/Linux/soft_rss_limit_mb_test.cpp:68:24: error: CHECK_MAY_RETURN_1: expected string not found in input
// CHECK_MAY_RETURN_1: allocating 512 times
                       ^
<stdin>:52:44: note: scanning from here
Some of the malloc calls returned non-null: 256
                                           ^
<stdin>:53:14: note: possible intended match here
==1969073==LeakSanitizer: soft rss limit unexhausted (220Mb vs 34Mb)
             ^

Input file: <stdin>
Check file: /home/b/sanitizer-x86_64-linux/build/llvm-project/compiler-rt/test/sanitizer_common/TestCases/Linux/soft_rss_limit_mb_test.cpp

-dump-input=help explains the following input dump.

Input was:
<<<<<<
            .
            .
            .
           47:  [256] 
           48:  [320] 
           49:  [384] 
           50:  [448] 
           51: Some of the malloc calls returned null: 256 
           52: Some of the malloc calls returned non-null: 256 
check:68'0                                                X~~~~ error: no match found
           53: ==1969073==LeakSanitizer: soft rss limit unexhausted (220Mb vs 34Mb) 
Step 11 (test compiler-rt debug) failure: test compiler-rt debug (failure)
...
[378/384] Generating Fuzzer-x86_64-Test
[379/384] Generating MSAN_INST_GTEST.gtest-all.cc.x86_64.o
[380/384] Generating MSAN_INST_TEST_OBJECTS.msan_test.cpp.x86_64-with-call.o
[381/384] Generating Msan-x86_64-with-call-Test
[382/384] Generating MSAN_INST_TEST_OBJECTS.msan_test.cpp.x86_64.o
[383/384] Generating Msan-x86_64-Test
[383/384] Running compiler_rt regression tests
llvm-lit: /home/b/sanitizer-x86_64-linux/build/llvm-project/llvm/utils/lit/lit/main.py:72: note: The test suite configuration requested an individual test timeout of 0 seconds but a timeout of 900 seconds was requested on the command line. Forcing timeout to be 900 seconds.
-- Testing: 4592 of 10298 tests, 88 workers --
Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.
FAIL: SanitizerCommon-lsan-i386-Linux :: Linux/soft_rss_limit_mb_test.cpp (3032 of 4592)
******************** TEST 'SanitizerCommon-lsan-i386-Linux :: Linux/soft_rss_limit_mb_test.cpp' FAILED ********************
Exit Code: 1

Command Output (stderr):
--
RUN: at line 2: /home/b/sanitizer-x86_64-linux/build/build_default/./bin/clang  --driver-mode=g++ -gline-tables-only -fsanitize=leak  -m32 -funwind-tables  -I/home/b/sanitizer-x86_64-linux/build/llvm-project/compiler-rt/test -ldl -O2 /home/b/sanitizer-x86_64-linux/build/llvm-project/compiler-rt/test/sanitizer_common/TestCases/Linux/soft_rss_limit_mb_test.cpp -o /home/b/sanitizer-x86_64-linux/build/build_default/runtimes/runtimes-bins/compiler-rt/test/sanitizer_common/lsan-i386-Linux/Linux/Output/soft_rss_limit_mb_test.cpp.tmp
+ /home/b/sanitizer-x86_64-linux/build/build_default/./bin/clang --driver-mode=g++ -gline-tables-only -fsanitize=leak -m32 -funwind-tables -I/home/b/sanitizer-x86_64-linux/build/llvm-project/compiler-rt/test -ldl -O2 /home/b/sanitizer-x86_64-linux/build/llvm-project/compiler-rt/test/sanitizer_common/TestCases/Linux/soft_rss_limit_mb_test.cpp -o /home/b/sanitizer-x86_64-linux/build/build_default/runtimes/runtimes-bins/compiler-rt/test/sanitizer_common/lsan-i386-Linux/Linux/Output/soft_rss_limit_mb_test.cpp.tmp
RUN: at line 5: env LSAN_OPTIONS=soft_rss_limit_mb=220:quarantine_size=1:allocator_may_return_null=1      /home/b/sanitizer-x86_64-linux/build/build_default/runtimes/runtimes-bins/compiler-rt/test/sanitizer_common/lsan-i386-Linux/Linux/Output/soft_rss_limit_mb_test.cpp.tmp 2>&1 | FileCheck /home/b/sanitizer-x86_64-linux/build/llvm-project/compiler-rt/test/sanitizer_common/TestCases/Linux/soft_rss_limit_mb_test.cpp -check-prefix=CHECK_MAY_RETURN_1
+ env LSAN_OPTIONS=soft_rss_limit_mb=220:quarantine_size=1:allocator_may_return_null=1 /home/b/sanitizer-x86_64-linux/build/build_default/runtimes/runtimes-bins/compiler-rt/test/sanitizer_common/lsan-i386-Linux/Linux/Output/soft_rss_limit_mb_test.cpp.tmp
+ FileCheck /home/b/sanitizer-x86_64-linux/build/llvm-project/compiler-rt/test/sanitizer_common/TestCases/Linux/soft_rss_limit_mb_test.cpp -check-prefix=CHECK_MAY_RETURN_1
/home/b/sanitizer-x86_64-linux/build/llvm-project/compiler-rt/test/sanitizer_common/TestCases/Linux/soft_rss_limit_mb_test.cpp:68:24: error: CHECK_MAY_RETURN_1: expected string not found in input
// CHECK_MAY_RETURN_1: allocating 512 times
                       ^
<stdin>:52:44: note: scanning from here
Some of the malloc calls returned non-null: 256
                                           ^
<stdin>:53:14: note: possible intended match here
==1969073==LeakSanitizer: soft rss limit unexhausted (220Mb vs 34Mb)
             ^

Input file: <stdin>
Check file: /home/b/sanitizer-x86_64-linux/build/llvm-project/compiler-rt/test/sanitizer_common/TestCases/Linux/soft_rss_limit_mb_test.cpp

-dump-input=help explains the following input dump.

Input was:
<<<<<<
            .
            .
            .
           47:  [256] 
           48:  [320] 
           49:  [384] 
           50:  [448] 
           51: Some of the malloc calls returned null: 256 
           52: Some of the malloc calls returned non-null: 256 
check:68'0                                                X~~~~ error: no match found
           53: ==1969073==LeakSanitizer: soft rss limit unexhausted (220Mb vs 34Mb) 

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants