[modules] Fix miscompilation when using two RecordDecl definitions with the same name #3497
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…th the same name. When deserializing a RecordDecl we don't enforce that redeclaration chain contains only a single definition. So if the canonical decl is not a definition itself, `RecordType::getDecl` can return different objects before and after an include. It means we can build CGRecordLayout for one RecordDecl with its set of FieldDecl but try to use it with FieldDecl belonging to a different RecordDecl. With assertions enabled it results in > Assertion failed: (FieldInfo.count(FD) && "Invalid field for record!"), > function getLLVMFieldNo, file llvm-project/clang/lib/CodeGen/CGRecordLayout.h, line 199. and with assertions disabled a bunch of fields are treated as their memory is located at offset 0. Fix by keeping the first encountered RecordDecl definition and marking the subsequent ones as non-definitions. Also need to merge FieldDecl properly, so that `getPrimaryMergedDecl` works correctly and during name lookup we don't treat fields from same-name RecordDecl as ambiguous. rdar://80184238 Differential Revision: https://reviews.llvm.org/D106994 (cherry picked from commit 93764ff)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When deserializing a RecordDecl we don't enforce that redeclaration
chain contains only a single definition. So if the canonical decl is not
a definition itself,
RecordType::getDeclcan return different objectsbefore and after an include. It means we can build CGRecordLayout for
one RecordDecl with its set of FieldDecl but try to use it with
FieldDecl belonging to a different RecordDecl. With assertions enabled
it results in
and with assertions disabled a bunch of fields are treated as their
memory is located at offset 0.
Fix by keeping the first encountered RecordDecl definition and marking
the subsequent ones as non-definitions. Also need to merge FieldDecl
properly, so that
getPrimaryMergedDeclworks correctly and during namelookup we don't treat fields from same-name RecordDecl as ambiguous.
rdar://80184238
Differential Revision: https://reviews.llvm.org/D106994
(cherry picked from commit 93764ff)