Skip to content

[clang][bytecode] Don't check returned pointers for liveness #120107

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
Dec 17, 2024

Conversation

tbaederr
Copy link
Contributor

We're supposed to let them through and then later diagnose reading from them, but returning dead pointers is fine.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Dec 16, 2024
@llvmbot
Copy link
Member

llvmbot commented Dec 16, 2024

@llvm/pr-subscribers-clang

Author: Timm Baeder (tbaederr)

Changes

We're supposed to let them through and then later diagnose reading from them, but returning dead pointers is fine.


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

2 Files Affected:

  • (modified) clang/lib/AST/ByteCode/Interp.h (-12)
  • (modified) clang/test/AST/ByteCode/functions.cpp (+6-10)
diff --git a/clang/lib/AST/ByteCode/Interp.h b/clang/lib/AST/ByteCode/Interp.h
index f085f96fdf5391..2a6ea69475f787 100644
--- a/clang/lib/AST/ByteCode/Interp.h
+++ b/clang/lib/AST/ByteCode/Interp.h
@@ -318,18 +318,6 @@ template <PrimType Name, class T = typename PrimConv<Name>::T>
 bool Ret(InterpState &S, CodePtr &PC) {
   const T &Ret = S.Stk.pop<T>();
 
-  // Make sure returned pointers are live. We might be trying to return a
-  // pointer or reference to a local variable.
-  // Just return false, since a diagnostic has already been emitted in Sema.
-  if constexpr (std::is_same_v<T, Pointer>) {
-    // FIXME: We could be calling isLive() here, but the emitted diagnostics
-    // seem a little weird, at least if the returned expression is of
-    // pointer type.
-    // Null pointers are considered live here.
-    if (!Ret.isZero() && !Ret.isLive())
-      return false;
-  }
-
   assert(S.Current);
   assert(S.Current->getFrameOffset() == S.Stk.size() && "Invalid frame");
   if (!S.checkingPotentialConstantExpression() || S.Current->Caller)
diff --git a/clang/test/AST/ByteCode/functions.cpp b/clang/test/AST/ByteCode/functions.cpp
index b00f59a8d8d433..10bea3a0d017e2 100644
--- a/clang/test/AST/ByteCode/functions.cpp
+++ b/clang/test/AST/ByteCode/functions.cpp
@@ -303,21 +303,17 @@ namespace ReturnLocalPtr {
     return &a; // both-warning {{address of stack memory}}
   }
 
-  /// GCC rejects the expression below, just like the new interpreter. The current interpreter
-  /// however accepts it and only warns about the function above returning an address to stack
-  /// memory. If we change the condition to 'p() != nullptr', it even succeeds.
-  static_assert(p() == nullptr, ""); // ref-error {{static assertion failed}} \
-                                     // expected-error {{not an integral constant expression}}
-
-  /// FIXME: The current interpreter emits diagnostics in the reference case below, but the
-  /// new one does not.
+  /// FIXME: Both interpreters should diagnose this. We're returning a pointer to a local
+  /// variable.
+  static_assert(p() == nullptr, ""); // both-error {{static assertion failed}}
+
   constexpr const int &p2() {
-    int a = 12; // ref-note {{declared here}}
+    int a = 12; // both-note {{declared here}}
     return a; // both-warning {{reference to stack memory associated with local variable}}
   }
 
   static_assert(p2() == 12, ""); // both-error {{not an integral constant expression}} \
-                                 // ref-note {{read of variable whose lifetime has ended}}
+                                 // both-note {{read of variable whose lifetime has ended}}
 }
 
 namespace VoidReturn {

@tbaederr tbaederr merged commit 056cd12 into llvm:main Dec 17, 2024
10 checks passed
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.

2 participants