[clang] Fix underlying type of EmbedExpr #99050

Fznamznon · 2024-07-16T14:40:09Z

This patch makes remaining cases of #embed to emit int type since there is an agreement to do that for C. C++ is being discussed, but in general we don't want to produce different types for C and C++.

llvmbot · 2024-07-16T14:40:44Z

@llvm/pr-subscribers-clang

Author: Mariya Podchishchaeva (Fznamznon)

Changes

This patch makes remaining cases of #embed to emit int type since there is an agreement to do that for C. C++ is being discussed, but in general we don't want to produce different types for C and C++.

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

3 Files Affected:

(modified) clang/lib/AST/Expr.cpp (+1-1)
(modified) clang/lib/Sema/SemaInit.cpp (+1-1)
(modified) clang/test/Preprocessor/embed_codegen.cpp (+4-4)

diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
index 6af1b5683e08b..9d5b8167d0ee6 100644
--- a/clang/lib/AST/Expr.cpp
+++ b/clang/lib/AST/Expr.cpp
@@ -2376,7 +2376,7 @@ APValue SourceLocExpr::EvaluateInContext(const ASTContext &Ctx,
 EmbedExpr::EmbedExpr(const ASTContext &Ctx, SourceLocation Loc,
                      EmbedDataStorage *Data, unsigned Begin,
                      unsigned NumOfElements)
-    : Expr(EmbedExprClass, Ctx.UnsignedCharTy, VK_PRValue, OK_Ordinary),
+    : Expr(EmbedExprClass, Ctx.IntTy, VK_PRValue, OK_Ordinary),
       EmbedKeywordLoc(Loc), Ctx(&Ctx), Data(Data), Begin(Begin),
       NumOfElements(NumOfElements) {
   setDependence(ExprDependence::None);
diff --git a/clang/lib/Sema/SemaInit.cpp b/clang/lib/Sema/SemaInit.cpp
index a27ed02fc73b8..2c963b28aeb2b 100644
--- a/clang/lib/Sema/SemaInit.cpp
+++ b/clang/lib/Sema/SemaInit.cpp
@@ -2007,7 +2007,7 @@ static bool canInitializeArrayWithEmbedDataString(ArrayRef<Expr *> ExprList,
   if (InitType->isArrayType()) {
     const ArrayType *InitArrayType = InitType->getAsArrayTypeUnsafe();
     QualType InitElementTy = InitArrayType->getElementType();
-    QualType EmbedExprElementTy = EE->getType();
+    QualType EmbedExprElementTy = EE->getDataStringLiteral()->getType();
     const bool TypesMatch =
         Context.typesAreCompatible(InitElementTy, EmbedExprElementTy) ||
         (InitElementTy->isCharType() && EmbedExprElementTy->isCharType());
diff --git a/clang/test/Preprocessor/embed_codegen.cpp b/clang/test/Preprocessor/embed_codegen.cpp
index 201bf300bc669..fdbf4f2aa400f 100644
--- a/clang/test/Preprocessor/embed_codegen.cpp
+++ b/clang/test/Preprocessor/embed_codegen.cpp
@@ -13,9 +13,9 @@ int ca[] = {
 };
 
 // CHECK: %arrayinit.element = getelementptr inbounds i32, ptr %notca, i64 1
-// CHECK: store i8 106, ptr %arrayinit.element, align 4
+// CHECK: store i32 106, ptr %arrayinit.element, align 4
 // CHECK: %arrayinit.element1 = getelementptr inbounds i32, ptr %notca, i64 2
-// CHECK: store i8 107, ptr %arrayinit.element1, align 4
+// CHECK: store i32 107, ptr %arrayinit.element1, align 4
 int notca[] = {
 a
 #embed <jk.txt> prefix(,)
@@ -74,9 +74,9 @@ constexpr struct T t[] = {
 // CHECK:  %arrayinit.element7 = getelementptr inbounds %struct.T, ptr %tnonc, i64 1
 // CHECK:  call void @llvm.memset.p0.i64(ptr align 4 %arrayinit.element7, i8 0, i64 20, i1 false)
 // CHECK:  %arr8 = getelementptr inbounds %struct.T, ptr %arrayinit.element7, i32 0, i32 0
-// CHECK:  store i8 106, ptr %arr8, align 4
+// CHECK:  store i32 106, ptr %arr8, align 4
 // CHECK:  %arrayinit.element9 = getelementptr inbounds i32, ptr %arr8, i64 1
-// CHECK:  store i8 107, ptr %arrayinit.element9, align 4
+// CHECK:  store i32 107, ptr %arrayinit.element9, align 4
 struct T tnonc[] = {
   a, 300, 1, 2, 3
 #embed <jk.txt> prefix(,)

Fznamznon · 2024-07-16T14:45:08Z

clang/test/Preprocessor/embed_codegen.cpp

@@ -13,9 +13,9 @@ int ca[] = {
 };

 // CHECK: %arrayinit.element = getelementptr inbounds i32, ptr %notca, i64 1
-// CHECK: store i8 106, ptr %arrayinit.element, align 4
+// CHECK: store i32 106, ptr %arrayinit.element, align 4


This is actually shows a bug in CodeGen for #embed, it should have always stored i32 because the target storage is i32. A cast emission is missing in CGExprAgg.cpp. Though I'm not sure it should be included into this PR.
Another problem is that I can't think of a good test case that shows that elements embedded in the initializer list are of int type and not of unsigned char type.

I was trying to write a test case and found an unrelated embed crash https://compiler-explorer.com/z/bMWbGfh79

can we add the test here #97274 (comment) (with sizeof) ? Thanks

AaronBallman

Changes LGTM but I agree with @cor3ntin that we should add the test case from #97274 (comment) explicitly. Some other tests to consider adding at the same time:

static_assert(_Generic(
#embed __FILE__ limit(1)
  , int : 1, default : 0));

static_assert(alignof(typeof(
#embed __FILE__ limit(1)
)) == alignof(int));

printf("%hhu", // Do we get a -Wformat diagnostic about the specifier mismatch?
#embed __FILE__ limit(1)
);

Fznamznon · 2024-07-17T09:05:12Z

I can add test with sizeof for sure, the problem is that it is already working https://godbolt.org/z/K69v8KPYa and we have similar ones in codebase already.
The test I'm struggling with is to check that elements in initializer list that are expanded from embed also have int type.

AaronBallman · 2024-07-17T13:02:19Z

I can add test with sizeof for sure, the problem is that it is already working https://godbolt.org/z/K69v8KPYa and we have similar ones in codebase already.

Ah, if we have sufficient coverage already, that's fine. I was just thinking about coverage in general, didn't check to see if it was redundant. :-)

The test I'm struggling with is to check that elements in initializer list that are expanded from embed also have int type.

If you have an embed file with a single byte in it whose value is 0xFF, you could use a test like:

struct S {
  signed char ch;
};

constexpr struct S s = {
#embed "foo"
};

which should give the same behavior as: https://godbolt.org/z/Tno5YPexM

Fznamznon · 2024-07-18T15:42:01Z

If you have an embed file with a single byte in it whose value is 0xFF, you could use a test like:
struct S {
signed char ch;
};
constexpr struct S s = {
#embed "foo"
};
which should give the same behavior as: https://godbolt.org/z/Tno5YPexM

Thanks! That worked.

I didn't add these though

static_assert(_Generic(
#embed __FILE__ limit(1)
  , int : 1, default : 0));

static_assert(alignof(typeof(
#embed __FILE__ limit(1)
)) == alignof(int));

printf("%hhu", // Do we get a -Wformat diagnostic about the specifier mismatch?
#embed __FILE__ limit(1)
);

for several reasons:

They don't test this patch as it affects only what happens in an initializer list
The one with alignof/typeof asserts for C++ (not for C) and I'd like to make a fix and add this test in a separate PR
we don't emit warnings even for printf("%hhu", 300);

Fznamznon · 2024-07-19T12:08:53Z

The one with alignof/typeof asserts for C++ (not for C) and I'd like to make a fix and add this test in a separate PR

Posted #99624 to fix the assertion

AaronBallman

LGTM!

This patch makes remaining cases of #embed to emit int type since there is an agreement to do that for C. C++ is being discussed, but in general we don't want to produce different types for C and C++.

Summary: This patch makes remaining cases of #embed to emit int type since there is an agreement to do that for C. C++ is being discussed, but in general we don't want to produce different types for C and C++. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60251342

[clang] Fix underlying type of EmbedExpr

c089e25

This patch makes remaining cases of #embed to emit int type since there is an agreement to do that for C. C++ is being discussed, but in general we don't want to produce different types for C and C++.

Fznamznon requested a review from AaronBallman July 16, 2024 14:40

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

Fznamznon commented Jul 16, 2024

View reviewed changes

Fznamznon mentioned this pull request Jul 16, 2024

[clang] Inject tokens containing #embed back into token stream #97274

Merged

AaronBallman reviewed Jul 16, 2024

View reviewed changes

Fznamznon added 2 commits July 18, 2024 06:14

Merge branch 'main' into fix-embed-ty

5d7d825

Add test cases

8798f21

Fznamznon requested review from AaronBallman and cor3ntin July 18, 2024 15:42

AaronBallman approved these changes Jul 19, 2024

View reviewed changes

Fznamznon merged commit 7122b70 into llvm:main Jul 19, 2024
7 checks passed

Fznamznon mentioned this pull request Jul 24, 2024

[clang] Follow-up on #embed implementation #95222

Open

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[clang] Fix underlying type of EmbedExpr #99050

[clang] Fix underlying type of EmbedExpr #99050

Fznamznon commented Jul 16, 2024

llvmbot commented Jul 16, 2024

Fznamznon Jul 16, 2024

cor3ntin Jul 16, 2024 •

edited

Loading

AaronBallman left a comment

Fznamznon commented Jul 17, 2024

AaronBallman commented Jul 17, 2024

Fznamznon commented Jul 18, 2024 •

edited

Loading

Fznamznon commented Jul 19, 2024

AaronBallman left a comment

[clang] Fix underlying type of EmbedExpr #99050

[clang] Fix underlying type of EmbedExpr #99050

Conversation

Fznamznon commented Jul 16, 2024

llvmbot commented Jul 16, 2024

Fznamznon Jul 16, 2024

Choose a reason for hiding this comment

cor3ntin Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

AaronBallman left a comment

Choose a reason for hiding this comment

Fznamznon commented Jul 17, 2024

AaronBallman commented Jul 17, 2024

Fznamznon commented Jul 18, 2024 • edited Loading

Fznamznon commented Jul 19, 2024

AaronBallman left a comment

Choose a reason for hiding this comment

cor3ntin Jul 16, 2024 •

edited

Loading

Fznamznon commented Jul 18, 2024 •

edited

Loading