Skip to content
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

[Clang] [CodeGen] Fix codegen bug in constant initialisation in C23 mode #84981

Merged
merged 6 commits into from
Mar 13, 2024

Conversation

Sirraide
Copy link
Contributor

Consider the following code:

bool const inf =  (1.0/0.0);

When trying to emit the initialiser of this variable in C23, we end up hitting a code path in codegen in VarDecl::evaluateValueImpl() where we check for IsConstantInitialization && (Ctx.getLangOpts().CPlusPlus || Ctx.getLangOpts().C23), and if that is the case and we emitted any notes, constant evaluation fails, and as a result, codegen issues this error:

<source>:1:12: error: cannot compile this static initializer yet
    1 | bool const inf =  (1.0/0.0);
      |        

As a fix, only fail in C23 mode if we’re initialising a constexpr variable.

This fixes #84784.

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

llvmbot commented Mar 12, 2024

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Author: None (Sirraide)

Changes

Consider the following code:

bool const inf =  (1.0/0.0);

When trying to emit the initialiser of this variable in C23, we end up hitting a code path in codegen in VarDecl::evaluateValueImpl() where we check for IsConstantInitialization &amp;&amp; (Ctx.getLangOpts().CPlusPlus || Ctx.getLangOpts().C23), and if that is the case and we emitted any notes, constant evaluation fails, and as a result, codegen issues this error:

&lt;source&gt;:1:12: error: cannot compile this static initializer yet
    1 | bool const inf =  (1.0/0.0);
      |        

As a fix, only fail in C23 mode if we’re initialising a constexpr variable.

This fixes #84784.


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

3 Files Affected:

  • (modified) clang/docs/ReleaseNotes.rst (+3)
  • (modified) clang/lib/AST/Decl.cpp (+3-1)
  • (modified) clang/test/CodeGen/const-init.c (+3)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index f61dca9bbc8467..c773991b43f47c 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -259,6 +259,9 @@ Bug Fixes in This Version
   operator.
   Fixes (#GH83267).
 
+- Clang no longer fails to codegen static ``const`` variables whose initialiser performs
+  a floating-point division by 0 in C23.
+
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
diff --git a/clang/lib/AST/Decl.cpp b/clang/lib/AST/Decl.cpp
index d681791d3920c3..38317dea6d22fe 100644
--- a/clang/lib/AST/Decl.cpp
+++ b/clang/lib/AST/Decl.cpp
@@ -2581,7 +2581,9 @@ APValue *VarDecl::evaluateValueImpl(SmallVectorImpl<PartialDiagnosticAt> &Notes,
   // case, we can't keep the result, because it may only be correct under the
   // assumption that the initializer is a constant context.
   if (IsConstantInitialization &&
-      (Ctx.getLangOpts().CPlusPlus || Ctx.getLangOpts().C23) && !Notes.empty())
+      (Ctx.getLangOpts().CPlusPlus ||
+       (isConstexpr() && Ctx.getLangOpts().C23)) &&
+      !Notes.empty())
     Result = false;
 
   // Ensure the computed APValue is cleaned up later if evaluation succeeded,
diff --git a/clang/test/CodeGen/const-init.c b/clang/test/CodeGen/const-init.c
index 0e4fc4ad48af8d..ad3e9551199ac2 100644
--- a/clang/test/CodeGen/const-init.c
+++ b/clang/test/CodeGen/const-init.c
@@ -216,3 +216,6 @@ int PR4517_x2 = PR4517_arrc[PR4517_idx];
 // CHECK: @PR4517_x = global i32 42, align 4
 // CHECK: @PR4517_idx = constant i32 1, align 4
 // CHECK: @PR4517_x2 = global i32 42, align 4
+
+// CHECK: @GH84784_inf = constant i8 1
+_Bool const GH84784_inf = (1.0/0.0);

Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

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

Thank you for the fix! This is looking pretty good to me.

clang/test/CodeGen/const-init.c Show resolved Hide resolved
@Sirraide
Copy link
Contributor Author

I’ve added the test. The CI failure is the same thing from a few days ago I think; must be because I created this branch before updating the main branch.

Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

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

LGTM!

clang/test/Sema/const-init.c Outdated Show resolved Hide resolved
@Sirraide
Copy link
Contributor Author

CI is just going to fail again because of the aforementioned problem, so I’m just going to merge this now and hope I won’t have to revert it later.

@Sirraide Sirraide merged commit 2cf2bc4 into llvm:main Mar 13, 2024
3 of 4 checks passed
@Sirraide Sirraide deleted the c23-inf branch March 13, 2024 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c23 clang:codegen 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.

C23 mode: cannot compile this static initializer yet
4 participants