Skip to content

[WIP] [Modules] Delay reading type source info of the preferred_name attribute. #122250

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

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
15 changes: 15 additions & 0 deletions clang/include/clang/Serialization/ASTReader.h
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@
#ifndef LLVM_CLANG_SERIALIZATION_ASTREADER_H
#define LLVM_CLANG_SERIALIZATION_ASTREADER_H

#include "clang/AST/NestedNameSpecifier.h"
#include "clang/AST/Type.h"
#include "clang/Basic/AttributeCommonInfo.h"
#include "clang/Basic/Diagnostic.h"
#include "clang/Basic/DiagnosticOptions.h"
#include "clang/Basic/IdentifierTable.h"
Expand Down Expand Up @@ -1156,6 +1158,19 @@ class ASTReader
SmallVector<std::pair<VarDecl *, serialization::TypeID>, 16>
PendingDeducedVarTypes;

struct PendingPreferredNameAttribute {
Decl *D;
AttributeCommonInfo Info;
serialization::TypeID TypeID;
bool isInherited;
bool isImplicit;
bool isPackExpansion;
SourceLocation ElaboratedTypedefSL;
NestedNameSpecifierLoc NestedNameSL;
SourceLocation TypedefSL;
};
std::deque<PendingPreferredNameAttribute> PendingPreferredNameAttributes;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not SmallVector like the above?
For a slightly higher efficiency and for consistency with the code above?

Copy link
Contributor Author

@VitaNuo VitaNuo Jan 14, 2025

Choose a reason for hiding this comment

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

For a slightly higher efficiency and for consistency with the code above?

Exactly. But I'm not sure if this matters much.


/// The list of redeclaration chains that still need to be
/// reconstructed, and the local offset to the corresponding list
/// of redeclarations.
Expand Down
8 changes: 8 additions & 0 deletions clang/include/clang/Serialization/ASTRecordReader.h
Original file line number Diff line number Diff line change
Expand Up @@ -337,6 +337,14 @@ class ASTRecordReader
/// Reads attributes from the current stream position, advancing Idx.
void readAttributes(AttrVec &Attrs);

/// Reads one attribute from the current stream position, advancing Idx.
/// Parent Decl is provided to delay attribute deserialization.
Copy link
Contributor

Choose a reason for hiding this comment

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

The lazy deserialization is a subtle difference, could we name the functions appropriately and "scary", e.g. readAttrOrDelay.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in #122726.

Attr *readAttr(Decl *D);

/// Reads attributes from the current stream position, advancing Idx.
/// Parent Decl is provided to delay attribute deserialization.
void readAttributes(AttrVec &Attrs, Decl *D);

/// Read an BTFTypeTagAttr object.
BTFTypeTagAttr *readBTFTypeTagAttr() {
return cast<BTFTypeTagAttr>(readAttr());
Expand Down
27 changes: 27 additions & 0 deletions clang/lib/Serialization/ASTReader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9860,6 +9860,33 @@ void ASTReader::finishPendingActions() {
}
PendingDeducedVarTypes.clear();

ASTContext &Context = getContext();
for (unsigned I = 0; I != PendingPreferredNameAttributes.size(); ++I) {
auto *D = PendingPreferredNameAttributes[I].D;
QualType InfoTy = GetType(PendingPreferredNameAttributes[I].TypeID);
TypeSourceInfo *TInfo = nullptr;
if (!InfoTy.isNull()) {
TInfo = getContext().CreateTypeSourceInfo(InfoTy);
if (auto Loc = TInfo->getTypeLoc().getAs<ElaboratedTypeLoc>()) {
Loc.setElaboratedKeywordLoc(
PendingPreferredNameAttributes[I].ElaboratedTypedefSL);
Loc.setQualifierLoc(PendingPreferredNameAttributes[I].NestedNameSL);
if (auto TypedefLoc = Loc.getNextTypeLoc().getAs<TypedefTypeLoc>())
TypedefLoc.setNameLoc(PendingPreferredNameAttributes[I].TypedefSL);
}
}

AttrVec &Attrs = getContext().getDeclAttrs(D);
PreferredNameAttr *New = new (Context) PreferredNameAttr(
Context, PendingPreferredNameAttributes[I].Info, TInfo);
cast<InheritableAttr>(New)->setInherited(
PendingPreferredNameAttributes[I].isInherited);
New->setImplicit(PendingPreferredNameAttributes[I].isImplicit);
Copy link
Contributor

Choose a reason for hiding this comment

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

Duplicating the code here may lead to errors later.

Could we ensure that's the only code path that's being used to read preferred_name attributes? That would entail asserting that the common readAttr asserts the attribute is not PreferredName and I'd also suggest adding a documentation comment in Attr.td that PreferredName has a custom deserialization logic pointing to that function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Obsolete with #122726.

New->setPackExpansion(PendingPreferredNameAttributes[I].isPackExpansion);
Attrs.push_back(New);
}
PendingPreferredNameAttributes.clear();

// For each decl chain that we wanted to complete while deserializing, mark
// it as "still needs to be completed".
for (unsigned I = 0; I != PendingIncompleteDeclChains.size(); ++I) {
Expand Down
108 changes: 79 additions & 29 deletions clang/lib/Serialization/ASTReaderDecl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -638,7 +638,7 @@ void ASTDeclReader::VisitDecl(Decl *D) {

if (HasAttrs) {
AttrVec Attrs;
Record.readAttributes(Attrs);
Record.readAttributes(Attrs, D);
// Avoid calling setAttrs() directly because it uses Decl::getASTContext()
// internally which is unsafe during derialization.
D->setAttrsImpl(Attrs, Reader.getContext());
Expand Down Expand Up @@ -3129,42 +3129,86 @@ class AttrReader {
OMPTraitInfo *readOMPTraitInfo() { return Reader.readOMPTraitInfo(); }

template <typename T> T *readDeclAs() { return Reader.readDeclAs<T>(); }

AttributeCommonInfo readAttributeCommonInfo() {
IdentifierInfo *AttrName = readIdentifier();
IdentifierInfo *ScopeName = readIdentifier();
SourceRange AttrRange = readSourceRange();
SourceLocation ScopeLoc = readSourceLocation();
unsigned ParsedKind = readInt();
unsigned Syntax = readInt();
unsigned SpellingIndex = readInt();
bool IsAlignas = (ParsedKind == AttributeCommonInfo::AT_Aligned &&
Syntax == AttributeCommonInfo::AS_Keyword &&
SpellingIndex == AlignedAttr::Keyword_alignas);
bool IsRegularKeywordAttribute = readBool();

AttributeCommonInfo Info(AttrName, ScopeName, AttrRange, ScopeLoc,
AttributeCommonInfo::Kind(ParsedKind),
{AttributeCommonInfo::Syntax(Syntax),
SpellingIndex, IsAlignas,
IsRegularKeywordAttribute});
return Info;
}

std::optional<attr::Kind> readAttrKind() {
auto V = readInt();
if (!V)
return {};

// Kind is stored as a 1-based integer because 0 is used to indicate a null
// Attr pointer.
return static_cast<attr::Kind>(V - 1);
}

Attr *createAttribute(attr::Kind Kind, AttributeCommonInfo Info) {
ASTContext &Context = Reader.getContext();
Attr *New = nullptr;
auto Record = *this;
#include "clang/Serialization/AttrPCHRead.inc"

assert(New && "Unable to decode attribute?");
return New;
}
};
}
} // namespace

Attr *ASTRecordReader::readAttr() {
AttrReader Record(*this);
auto V = Record.readInt();
if (!V)
attr::Kind Kind;
if (auto KindOpt = Record.readAttrKind(); !KindOpt)
return nullptr;
else
Kind = *KindOpt;

Attr *New = nullptr;
// Kind is stored as a 1-based integer because 0 is used to indicate a null
// Attr pointer.
auto Kind = static_cast<attr::Kind>(V - 1);
ASTContext &Context = getContext();

IdentifierInfo *AttrName = Record.readIdentifier();
IdentifierInfo *ScopeName = Record.readIdentifier();
SourceRange AttrRange = Record.readSourceRange();
SourceLocation ScopeLoc = Record.readSourceLocation();
unsigned ParsedKind = Record.readInt();
unsigned Syntax = Record.readInt();
unsigned SpellingIndex = Record.readInt();
bool IsAlignas = (ParsedKind == AttributeCommonInfo::AT_Aligned &&
Syntax == AttributeCommonInfo::AS_Keyword &&
SpellingIndex == AlignedAttr::Keyword_alignas);
bool IsRegularKeywordAttribute = Record.readBool();

AttributeCommonInfo Info(AttrName, ScopeName, AttrRange, ScopeLoc,
AttributeCommonInfo::Kind(ParsedKind),
{AttributeCommonInfo::Syntax(Syntax), SpellingIndex,
IsAlignas, IsRegularKeywordAttribute});
AttributeCommonInfo Info = Record.readAttributeCommonInfo();
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of doing this, could we have a single function (e.g. readAttrImpl) that handles both cases and just asserts Decl* is not null whenever it sees the PreferredName?

That way, we only have one copy of the code and readAttributeCommonInfo is not really needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, done in #122726.

return Record.createAttribute(Kind, Info);
}

#include "clang/Serialization/AttrPCHRead.inc"
Attr *ASTRecordReader::readAttr(Decl *D) {
AttrReader Record(*this);
attr::Kind Kind;
if (auto KindOpt = Record.readAttrKind(); !KindOpt)
return nullptr;
else
Kind = *KindOpt;

AttributeCommonInfo Info = Record.readAttributeCommonInfo();
if (Kind == attr::PreferredName) {
bool isInherited = Record.readInt();
bool isImplicit = Record.readInt();
bool isPackExpansion = Record.readInt();
serialization::TypeID TypeID = getGlobalTypeID(Record.readInt());
SourceLocation ElaboratedTypedefSL = Record.readSourceLocation();
Copy link
Contributor

Choose a reason for hiding this comment

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

This code assumes a specific structure of the TypeLoc we store for PreferredName.
It is a major assumption that we will not change the representation of TypeLocs and that the particular TypeLocs used for PreferredName stay the same over time.

We could probably make the code here more robust by also updating serialization so that it writes only the source locations we read here, rather than call the generic method for serializing TypeLoc.
However, I don't think this will lead to better code.

Could we instead try postponing reading the whole attribute?
As chatted outside of this thread, this would entail updating the serialization of PreferredName to store the number of record entries that it takes.
Later, when deserializing, we can simply remember the position at which the attribute starts, skip it and store the position in the "pending attributes". In finishPendingAction, we can temporarily switch the position back and call readAttr to re-read the whole attribute.

I bet this approach would end up being much simpler and will allow avoiding the duplication of serialization/deseriazliation logic. Moreover, it can be generalized to more attribute if the need arises.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the suggestion, the draft implementation is in #122726.

NestedNameSpecifierLoc NestedNameSL = readNestedNameSpecifierLoc();
SourceLocation TypedefSL = Record.readSourceLocation();
Reader->PendingPreferredNameAttributes.push_back(
{D, Info, TypeID, isInherited, isImplicit, isPackExpansion,
ElaboratedTypedefSL, NestedNameSL, TypedefSL});
return nullptr;
}

assert(New && "Unable to decode attribute?");
return New;
return Record.createAttribute(Kind, Info);
}

/// Reads attributes from the current stream position.
Expand All @@ -3174,6 +3218,12 @@ void ASTRecordReader::readAttributes(AttrVec &Attrs) {
Attrs.push_back(A);
}

void ASTRecordReader::readAttributes(AttrVec &Attrs, Decl *D) {
for (unsigned I = 0, E = readInt(); I != E; ++I)
if (auto *A = readAttr(D))
Attrs.push_back(A);
}

//===----------------------------------------------------------------------===//
// ASTReader Implementation
//===----------------------------------------------------------------------===//
Expand Down
3 changes: 1 addition & 2 deletions clang/lib/Serialization/ASTWriter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4720,8 +4720,7 @@ void ASTRecordWriter::AddAttr(const Attr *A) {
// FIXME: Clang can't handle the serialization/deserialization of
// preferred_name properly now. See
// https://github.com/llvm/llvm-project/issues/56490 for example.
if (!A || (isa<PreferredNameAttr>(A) &&
Writer->isWritingStdCXXNamedModules()))
if (!A)
return Record.push_back(0);

Record.push_back(A->getKind() + 1); // FIXME: stable encoding, target attrs
Expand Down
11 changes: 9 additions & 2 deletions clang/test/Modules/preferred_name.cppm
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,17 @@ import A;
export using ::foo_templ;

//--- Use1.cpp
import A; // [email protected]:8 {{attribute declaration must precede definition}}
#include "foo.h" // [email protected]:9 {{previous definition is here}}
// expected-no-diagnostics
import A;
#include "foo.h"

//--- Use2.cpp
// expected-no-diagnostics
#include "foo.h"
import A;

//--- Use3.cpp
#include "foo.h"
import A;
foo test;
int size = test.size(); // expected-error {{no member named 'size' in 'foo'}}
Loading