Skip to content

[clang] Implement statement printer for EmbedExpr #117770

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

Closed
wants to merge 2 commits into from
Closed

[clang] Implement statement printer for EmbedExpr #117770

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
184359e
[clang] Store filename in EmbedExpr
circl-lastname Nov 26, 2024
c175f5c
[clang] Implement statement printer for EmbedExpr
circl-lastname Nov 26, 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
1 change: 1 addition & 0 deletions clang/docs/ReleaseNotes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,7 @@ C23 Feature Support
- Clang now officially supports `N3030 <https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3030.htm>`_ Enhancements to Enumerations. Clang already supported it as an extension, so there were no changes to compiler behavior.
- Fixed the value of ``BOOL_WIDTH`` in ``<limits.h>`` to return ``1``
explicitly, as mandated by the standard. Fixes #GH117348
- Initial support for pretty printing ``#embed`` directives, which no longer crash clangd when hovered.

Non-comprehensive list of changes in this release
-------------------------------------------------
Expand Down
9 changes: 7 additions & 2 deletions clang/include/clang/AST/Expr.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4917,19 +4917,24 @@ class EmbedExpr final : public Expr {
SourceLocation EmbedKeywordLoc;
IntegerLiteral *FakeChildNode = nullptr;
const ASTContext *Ctx = nullptr;
StringRef Filename;
bool IsAngled;
EmbedDataStorage *Data;
unsigned Begin = 0;
unsigned NumOfElements;

public:
EmbedExpr(const ASTContext &Ctx, SourceLocation Loc, EmbedDataStorage *Data,
unsigned Begin, unsigned NumOfElements);
EmbedExpr(const ASTContext &Ctx, SourceLocation Loc, StringRef Filename,
bool IsAngled, EmbedDataStorage *Data, unsigned Begin,
unsigned NumOfElements);
explicit EmbedExpr(EmptyShell Empty) : Expr(SourceLocExprClass, Empty) {}

SourceLocation getLocation() const { return EmbedKeywordLoc; }
SourceLocation getBeginLoc() const { return EmbedKeywordLoc; }
SourceLocation getEndLoc() const { return EmbedKeywordLoc; }

StringRef getFilename() const { return Filename; }
bool getIsAngled() const { return IsAngled; }
StringLiteral *getDataStringLiteral() const { return Data->BinaryData; }
EmbedDataStorage *getData() const { return Data; }

Expand Down
5 changes: 4 additions & 1 deletion clang/include/clang/Lex/Preprocessor.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2761,7 +2761,8 @@ class Preprocessor {
// Binary data inclusion
void HandleEmbedDirective(SourceLocation HashLoc, Token &Tok,
const FileEntry *LookupFromFile = nullptr);
void HandleEmbedDirectiveImpl(SourceLocation HashLoc,
void HandleEmbedDirectiveImpl(SourceLocation HashLoc, StringRef Filename,
bool IsAngled,
const LexEmbedParametersResult &Params,
StringRef BinaryContents);

Expand Down Expand Up @@ -3066,6 +3067,8 @@ class EmptylineHandler {
/// Helper class to shuttle information about #embed directives from the
/// preprocessor to the parser through an annotation token.
struct EmbedAnnotationData {
std::string Filename;
bool IsAngled;
StringRef BinaryData;
};

Expand Down
4 changes: 2 additions & 2 deletions clang/include/clang/Sema/Sema.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7086,8 +7086,8 @@ class Sema final : public SemaBase {
SourceLocation RPLoc);

// #embed
ExprResult ActOnEmbedExpr(SourceLocation EmbedKeywordLoc,
StringLiteral *BinaryData);
ExprResult ActOnEmbedExpr(SourceLocation EmbedKeywordLoc, StringRef Filename,
bool IsAngled, StringLiteral *BinaryData);

// Build a potentially resolved SourceLocExpr.
ExprResult BuildSourceLocExpr(SourceLocIdentKind Kind, QualType ResultTy,
Expand Down
8 changes: 4 additions & 4 deletions clang/lib/AST/Expr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2383,11 +2383,11 @@ APValue SourceLocExpr::EvaluateInContext(const ASTContext &Ctx,
}

EmbedExpr::EmbedExpr(const ASTContext &Ctx, SourceLocation Loc,
EmbedDataStorage *Data, unsigned Begin,
unsigned NumOfElements)
StringRef Filename, bool IsAngled, EmbedDataStorage *Data,
unsigned Begin, unsigned NumOfElements)
: Expr(EmbedExprClass, Ctx.IntTy, VK_PRValue, OK_Ordinary),
EmbedKeywordLoc(Loc), Ctx(&Ctx), Data(Data), Begin(Begin),
NumOfElements(NumOfElements) {
EmbedKeywordLoc(Loc), Ctx(&Ctx), Filename(Filename), IsAngled(IsAngled),
Data(Data), Begin(Begin), NumOfElements(NumOfElements) {
setDependence(ExprDependence::None);
FakeChildNode = IntegerLiteral::Create(
Ctx, llvm::APInt::getZero(Ctx.getTypeSize(getType())), getType(), Loc);
Expand Down
6 changes: 5 additions & 1 deletion clang/lib/AST/StmtPrinter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1202,7 +1202,11 @@ void StmtPrinter::VisitSourceLocExpr(SourceLocExpr *Node) {
}

void StmtPrinter::VisitEmbedExpr(EmbedExpr *Node) {
llvm::report_fatal_error("Not implemented");
// FIXME: This doesn't handle offsets, prefixes, and if_empty yet
OS << NL << "#embed ";
OS << (Node->getIsAngled() ? '<' : '"');
OS << Node->getFilename();
OS << (Node->getIsAngled() ? '>' : '"') << NL;
}

void StmtPrinter::VisitConstantExpr(ConstantExpr *Node) {
Expand Down
9 changes: 6 additions & 3 deletions clang/lib/Lex/PPDirectives.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3871,8 +3871,8 @@ Preprocessor::LexEmbedParameters(Token &CurTok, bool ForHasEmbed) {
}

void Preprocessor::HandleEmbedDirectiveImpl(
SourceLocation HashLoc, const LexEmbedParametersResult &Params,
StringRef BinaryContents) {
SourceLocation HashLoc, StringRef Filename, bool IsAngled,
const LexEmbedParametersResult &Params, StringRef BinaryContents) {
if (BinaryContents.empty()) {
// If we have no binary contents, the only thing we need to emit are the
// if_empty tokens, if any.
Expand Down Expand Up @@ -3902,6 +3902,8 @@ void Preprocessor::HandleEmbedDirectiveImpl(
}

EmbedAnnotationData *Data = new (BP) EmbedAnnotationData;
Data->Filename = Filename;
Data->IsAngled = IsAngled;
Data->BinaryData = BinaryContents;
Comment on lines 3904 to 3907
Copy link
Contributor

Choose a reason for hiding this comment

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

I struggle to see how the current implementation actually prevents the memory leak. EmbedAnnotationData is allocated first as if the std::string inside was empty, then the string is assigned with an array of chars which will likely extend string's size, giving no chance to deallocate proper size later. I wonder if keeping a StringRef in EmbedAnnotationData is possible instead.

Copy link
Author

Choose a reason for hiding this comment

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

Is there such a thing as an OwningStringRef then? I am not sure how to guarantee the string exists, and then is deallocated along with the tokens. I thought std::string would be a ptr to the dynamically sized string data, but I haven't used the STL much

Copy link
Contributor

Choose a reason for hiding this comment

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

None that I can think of... I don't think it would've helped though since I don't think the bump allocator calls the destructors.
I think we need to find a good place to store filename strings that come from embed directives. @AaronBallman any chance you have a good ideas about where can we store the filename string to manage the memory in a good way?

Copy link
Author

Choose a reason for hiding this comment

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

Similar issue with the deserializer, not sure where to store the string.

Copy link
Author

Choose a reason for hiding this comment

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

What if it's something like a flexible array member and the EmbedAnnotationData is allocated to the right size the first time?

Copy link
Member

Choose a reason for hiding this comment

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

where can we store the filename string to manage the memory in a good way?

Can’t we just copy the string data into the ASTContext’s allocator and store a StringRef to it, or am I missing something here?

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure you can do that during the lexing stage

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the way this may need to work is to find out where we're getting the file name from and seeing how long-lived that string is. It's passed here as a StringRef, and it would nice if we can store it as a StringRef in the EmbedAnnotationData rather than as a std::string. My guess is that the name comes from the FileManager and is as long-lived as the SourceManager which is owned by ASTContext, but we'd need to verify that.


Toks[CurIdx].startToken();
Expand Down Expand Up @@ -4006,5 +4008,6 @@ void Preprocessor::HandleEmbedDirective(SourceLocation HashLoc, Token &EmbedTok,
if (Callbacks)
Callbacks->EmbedDirective(HashLoc, Filename, isAngled, MaybeFileRef,
*Params);
HandleEmbedDirectiveImpl(HashLoc, *Params, BinaryContents);
HandleEmbedDirectiveImpl(HashLoc, Filename, isAngled, *Params,
BinaryContents);
}
3 changes: 2 additions & 1 deletion clang/lib/Parse/ParseInit.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -451,7 +451,8 @@ ExprResult Parser::createEmbedExpr() {

StringLiteral *BinaryDataArg = CreateStringLiteralFromStringRef(
Data->BinaryData, Context.UnsignedCharTy);
Res = Actions.ActOnEmbedExpr(StartLoc, BinaryDataArg);
Res = Actions.ActOnEmbedExpr(StartLoc, Data->Filename, Data->IsAngled,
BinaryDataArg);
}
return Res;
}
Expand Down
5 changes: 3 additions & 2 deletions clang/lib/Sema/SemaExpr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16770,12 +16770,13 @@ ExprResult Sema::BuildSourceLocExpr(SourceLocIdentKind Kind, QualType ResultTy,
}

ExprResult Sema::ActOnEmbedExpr(SourceLocation EmbedKeywordLoc,
StringRef Filename, bool IsAngled,
StringLiteral *BinaryData) {
EmbedDataStorage *Data = new (Context) EmbedDataStorage;
Data->BinaryData = BinaryData;
return new (Context)
EmbedExpr(Context, EmbedKeywordLoc, Data, /*NumOfElements=*/0,
Data->getDataElementCount());
EmbedExpr(Context, EmbedKeywordLoc, Filename, IsAngled, Data,
/*NumOfElements=*/0, Data->getDataElementCount());
}

static bool maybeDiagnoseAssignmentToFunction(Sema &S, QualType DstType,
Expand Down
6 changes: 3 additions & 3 deletions clang/lib/Sema/SemaInit.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -525,9 +525,9 @@ class InitListChecker {
}
}

Result = new (SemaRef.Context)
EmbedExpr(SemaRef.Context, Embed->getLocation(), Embed->getData(),
CurEmbedIndex, ElsCount);
Result = new (SemaRef.Context) EmbedExpr(
SemaRef.Context, Embed->getLocation(), Embed->getFilename(),
Embed->getIsAngled(), Embed->getData(), CurEmbedIndex, ElsCount);
CurEmbedIndex += ElsCount;
if (CurEmbedIndex >= Embed->getDataElementCount()) {
CurEmbed = nullptr;
Expand Down