Skip to content

[C++20][Modules] Fix non-determinism in serialized AST #110131

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

Merged
merged 1 commit into from
Sep 27, 2024
Merged

[C++20][Modules] Fix non-determinism in serialized AST #110131

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
22 changes: 22 additions & 0 deletions clang/include/clang/AST/DeclID.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,7 @@ class LocalDeclID : public DeclIDBase {
// Every Decl ID is a local decl ID to the module being writing in ASTWriter.
friend class ASTWriter;
friend class GlobalDeclID;
friend struct llvm::DenseMapInfo<clang::LocalDeclID>;

public:
LocalDeclID() : Base() {}
Expand Down Expand Up @@ -267,6 +268,27 @@ template <> struct DenseMapInfo<clang::GlobalDeclID> {
}
};

template <> struct DenseMapInfo<clang::LocalDeclID> {
using LocalDeclID = clang::LocalDeclID;
using DeclID = LocalDeclID::DeclID;

static LocalDeclID getEmptyKey() {
return LocalDeclID(DenseMapInfo<DeclID>::getEmptyKey());
}

static LocalDeclID getTombstoneKey() {
return LocalDeclID(DenseMapInfo<DeclID>::getTombstoneKey());
}

static unsigned getHashValue(const LocalDeclID &Key) {
return DenseMapInfo<DeclID>::getHashValue(Key.getRawValue());
}

static bool isEqual(const LocalDeclID &L, const LocalDeclID &R) {
return L == R;
}
};

} // namespace llvm

#endif
6 changes: 3 additions & 3 deletions clang/include/clang/Serialization/ASTWriter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,13 +233,13 @@ class ASTWriter : public ASTDeserializationListener,
/// instead of comparing the result of `getDeclID()` or `GetDeclRef()`.
llvm::SmallPtrSet<const Decl *, 32> PredefinedDecls;

/// Mapping from FunctionDecl to the list of lambda IDs inside the function.
/// Mapping from FunctionDecl ID to the list of lambda IDs inside the
/// function.
///
/// These lambdas have to be loaded right after the function they belong to.
/// In order to have canonical declaration for lambda class from the same
/// module as enclosing function during deserialization.
llvm::DenseMap<const Decl *, SmallVector<LocalDeclID, 4>>
FunctionToLambdasMap;
llvm::DenseMap<LocalDeclID, SmallVector<LocalDeclID, 4>> FunctionToLambdasMap;

/// Offset of each declaration in the bitstream, indexed by
/// the declaration's ID.
Expand Down
3 changes: 1 addition & 2 deletions clang/lib/Serialization/ASTWriter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5713,8 +5713,7 @@ void ASTWriter::WriteDeclAndTypes(ASTContext &Context) {
// efficent becuase it allows lazy deserialization.
RecordData FunctionToLambdasMapRecord;
for (const auto &Pair : FunctionToLambdasMap) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is now an iteration of a DenseMap of non-pointers, which I suppose is arbitrary, but determinstic. Are you sure it isn't preferable to write these in the order in which they were added to the map? That's the behavior you'd get from a MapVector, which seems better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rnk yes, the order is not important because we read the whole map into memory anyway. So IMHO the only thing we need to do here is to make sure that we order doesn't depend on pointer values.

FunctionToLambdasMapRecord.push_back(
GetDeclRef(Pair.first).getRawValue());
FunctionToLambdasMapRecord.push_back(Pair.first.getRawValue());
FunctionToLambdasMapRecord.push_back(Pair.second.size());
for (const auto &Lambda : Pair.second)
FunctionToLambdasMapRecord.push_back(Lambda.getRawValue());
Expand Down
3 changes: 2 additions & 1 deletion clang/lib/Serialization/ASTWriterDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1524,7 +1524,8 @@ void ASTDeclWriter::VisitCXXRecordDecl(CXXRecordDecl *D) {
// For lambdas inside canonical FunctionDecl remember the mapping.
if (auto FD = llvm::dyn_cast_or_null<FunctionDecl>(D->getDeclContext());
FD && FD->isCanonicalDecl()) {
Writer.FunctionToLambdasMap[FD].push_back(Writer.GetDeclRef(D));
Writer.FunctionToLambdasMap[Writer.GetDeclRef(FD)].push_back(
Writer.GetDeclRef(D));
}
} else {
Record.push_back(CXXRecNotTemplate);
Expand Down
Loading