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.

Sign up for GitHub

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

Jump to bottom

[clang] Fix underlying type of EmbedExpr #99050

Merged
merged 3 commits into from
Jul 19, 2024
Merged

[clang] Fix underlying type of EmbedExpr #99050

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c089e25
[clang] Fix underlying type of EmbedExpr
Fznamznon Jul 16, 2024
5d7d825
Merge branch 'main' into fix-embed-ty
Fznamznon Jul 18, 2024
8798f21
Add test cases
Fznamznon Jul 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion clang/lib/AST/Expr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion clang/lib/Sema/SemaInit.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2016,7 +2016,7 @@ 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());
Expand Down
1 change: 1 addition & 0 deletions clang/test/Preprocessor/Inputs/big_char.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
ÿ
8 changes: 4 additions & 4 deletions clang/test/Preprocessor/embed_codegen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@cor3ntin cor3ntin Jul 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

// 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(,)
Expand Down Expand Up @@ -75,9 +75,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(,)
Expand Down
11 changes: 11 additions & 0 deletions clang/test/Preprocessor/embed_weird.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,3 +115,14 @@ void f1() {
};
}
#endif

struct HasChar {
signed char ch;
};

constexpr struct HasChar c = {
#embed "Inputs/big_char.txt" // cxx-error {{constant expression evaluates to 255 which cannot be narrowed to type 'signed char'}} \
cxx-note {{insert an explicit cast to silence this issue}} \
c-error {{constexpr initializer evaluates to 255 which is not exactly representable in type 'signed char'}}

};
Loading