Skip to content

[clang][bytecode] Fix emitting dtors of zero-sized arrays #134672

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
Apr 8, 2025

Conversation

tbaederr
Copy link
Contributor

@tbaederr tbaederr commented Apr 7, 2025

Desc->getNumElems() returning 0 made us underflow here.

Desc->getNumElems() returning 0 made us underflow here.
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Apr 7, 2025
@llvmbot
Copy link
Member

llvmbot commented Apr 7, 2025

@llvm/pr-subscribers-clang

Author: Timm Baeder (tbaederr)

Changes

Desc->getNumElems() returning 0 made us underflow here.


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

2 Files Affected:

  • (modified) clang/lib/AST/ByteCode/Compiler.cpp (+11-9)
  • (modified) clang/test/AST/ByteCode/cxx23.cpp (+12)
diff --git a/clang/lib/AST/ByteCode/Compiler.cpp b/clang/lib/AST/ByteCode/Compiler.cpp
index 021acbd798646..9e9252558b1a9 100644
--- a/clang/lib/AST/ByteCode/Compiler.cpp
+++ b/clang/lib/AST/ByteCode/Compiler.cpp
@@ -6818,15 +6818,17 @@ bool Compiler<Emitter>::emitDestruction(const Descriptor *Desc,
         return true;
     }
 
-    for (ssize_t I = Desc->getNumElems() - 1; I >= 0; --I) {
-      if (!this->emitConstUint64(I, Loc))
-        return false;
-      if (!this->emitArrayElemPtrUint64(Loc))
-        return false;
-      if (!this->emitDestruction(ElemDesc, Loc))
-        return false;
-      if (!this->emitPopPtr(Loc))
-        return false;
+    if (size_t N = Desc->getNumElems()) {
+      for (ssize_t I = N - 1; I >= 0; --I) {
+        if (!this->emitConstUint64(I, Loc))
+          return false;
+        if (!this->emitArrayElemPtrUint64(Loc))
+          return false;
+        if (!this->emitDestruction(ElemDesc, Loc))
+          return false;
+        if (!this->emitPopPtr(Loc))
+          return false;
+      }
     }
     return true;
   }
diff --git a/clang/test/AST/ByteCode/cxx23.cpp b/clang/test/AST/ByteCode/cxx23.cpp
index 6a62ac11cde79..d0ade4f5278b1 100644
--- a/clang/test/AST/ByteCode/cxx23.cpp
+++ b/clang/test/AST/ByteCode/cxx23.cpp
@@ -304,3 +304,15 @@ namespace NonLiteralDtorInParam {
                               // expected23-note {{non-constexpr function '~NonLiteral' cannot be used in a constant expression}}
   }
 }
+
+namespace ZeroSizedArray {
+  struct S {
+    constexpr ~S() {
+    }
+  };
+  constexpr int foo() {
+    S s[0];
+    return 1;
+  }
+  static_assert(foo() == 1);
+}

@tbaederr tbaederr merged commit 65cede2 into llvm:main Apr 8, 2025
14 checks passed
return false;
if (!this->emitPopPtr(Loc))
return false;
if (size_t N = Desc->getNumElems()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please try to avoid implicit integer conversions where overflow is a potential issue; Desc->getNumElems() returns an unsigned, so please just make the variable unsigned as well.

tbaederr added a commit to tbaederr/llvm-project that referenced this pull request Apr 9, 2025
tbaederr added a commit that referenced this pull request Apr 9, 2025
llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this pull request Apr 9, 2025
AllinLeeYL pushed a commit to AllinLeeYL/llvm-project that referenced this pull request Apr 10, 2025
var-const pushed a commit to ldionne/llvm-project that referenced this pull request Apr 17, 2025
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