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] Fix crash when #embed used in a compound literal #102304

Merged
merged 2 commits into from
Aug 8, 2024

Conversation

Fznamznon
Copy link
Contributor

Fixes #102248

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

llvmbot commented Aug 7, 2024

@llvm/pr-subscribers-clang

Author: Mariya Podchishchaeva (Fznamznon)

Changes

Fixes #102248


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

3 Files Affected:

  • (modified) clang/docs/ReleaseNotes.rst (+1)
  • (modified) clang/lib/Sema/SemaInit.cpp (+2-2)
  • (modified) clang/test/Preprocessor/embed_codegen.cpp (+6)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 829a759ca4a0b..946d09b0215e5 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -167,6 +167,7 @@ Bug Fixes in This Version
   be used in C++.
 - Fixed a failed assertion when checking required literal types in C context. (#GH101304).
 - Fixed a crash when trying to transform a dependent address space type. Fixes #GH101685.
+- Fixed crash when #embed is used in a compound literal Fixes #GH102248.
 
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Sema/SemaInit.cpp b/clang/lib/Sema/SemaInit.cpp
index 90fd6df782f09..abd4401e02981 100644
--- a/clang/lib/Sema/SemaInit.cpp
+++ b/clang/lib/Sema/SemaInit.cpp
@@ -515,8 +515,8 @@ class InitListChecker {
     uint64_t ElsCount = 1;
     // Otherwise try to fill whole array with embed data.
     if (Entity.getKind() == InitializedEntity::EK_ArrayElement) {
-      ValueDecl *ArrDecl = Entity.getParent()->getDecl();
-      auto *AType = SemaRef.Context.getAsArrayType(ArrDecl->getType());
+      auto *AType =
+          SemaRef.Context.getAsArrayType(Entity.getParent()->getType());
       assert(AType && "expected array type when initializing array");
       ElsCount = Embed->getDataElementCount();
       if (const auto *CAType = dyn_cast<ConstantArrayType>(AType))
diff --git a/clang/test/Preprocessor/embed_codegen.cpp b/clang/test/Preprocessor/embed_codegen.cpp
index 5baab9b59a9c1..d9624306dc828 100644
--- a/clang/test/Preprocessor/embed_codegen.cpp
+++ b/clang/test/Preprocessor/embed_codegen.cpp
@@ -1,9 +1,15 @@
 // RUN: %clang_cc1 %s -triple x86_64 --embed-dir=%S/Inputs -emit-llvm -o - | FileCheck %s
 
+// CHECK: @.compoundliteral = internal global [2 x i8] c"jk"
 // CHECK: @__const._Z3fooi.ca = private unnamed_addr constant [3 x i32] [i32 0, i32 106, i32 107], align 4
 // CHECK: @__const._Z3fooi.sc = private unnamed_addr constant %struct.S1 { i32 106, i32 107, i32 0 }, align 4
 // CHECK: @__const._Z3fooi.t = private unnamed_addr constant [3 x %struct.T] [%struct.T { [2 x i32] [i32 48, i32 49], %struct.S1 { i32 50, i32 51, i32 52 } }, %struct.T { [2 x i32] [i32 53, i32 54], %struct.S1 { i32 55, i32 56, i32 57 } }, %struct.T { [2 x i32] [i32 10, i32 0], %struct.S1 zeroinitializer }], align 16
 // CHECK: @__const._Z3fooi.W = private unnamed_addr constant %struct.Wrapper { i32 48, %struct.HasCharArray { [10 x i8] c"123456789\0A" } }, align 4
+
+char *p = (char[]){
+#embed "jk.txt"
+};
+
 void foo(int a) {
 // CHECK: %a.addr = alloca i32, align 4
 // CHECK: store i32 %a, ptr %a.addr, align 4

Copy link
Contributor

@cor3ntin cor3ntin left a comment

Choose a reason for hiding this comment

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

LGTM.
I think it is reasonable to backport that (in which case we don't need the changelog entry)

Comment on lines 9 to 12
char *p = (char[]){
#embed "jk.txt"
};

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add tests for char[N], int[] ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, because compound literals in C++ are different from compound literals in C, we should have a C-specific test as well. And that test should probably be in Sema because this is fixing an issue in SemaInit.cpp.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem. Is it ok now?

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.

Changes LGTM as far as they go, but some additional test coverage is requested.

Comment on lines 9 to 12
char *p = (char[]){
#embed "jk.txt"
};

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, because compound literals in C++ are different from compound literals in C, we should have a C-specific test as well. And that test should probably be in Sema because this is fixing an issue in SemaInit.cpp.

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!

@Fznamznon Fznamznon merged commit 3606d69 into llvm:main Aug 8, 2024
7 checks passed
@Fznamznon Fznamznon added this to the LLVM 19.X Release milestone Aug 8, 2024
@Fznamznon
Copy link
Contributor Author

/cherry-pick 3606d69

llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Aug 8, 2024
@llvmbot
Copy link
Member

llvmbot commented Aug 8, 2024

/pull-request #102428

tru pushed a commit to llvmbot/llvm-project that referenced this pull request Aug 10, 2024
kstoimenov pushed a commit to kstoimenov/llvm-project that referenced this pull request Aug 15, 2024
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
Development

Successfully merging this pull request may close these issues.

#embed crashes when used in a compound literal
4 participants