This is an archive of the discontinued LLVM Phabricator instance.

[AST] Pick last tentative definition as the acting definition
ClosedPublic

Authored by pestctrl on Apr 1 2021, 7:46 AM.

Details

Summary

Clang currently picks the second tentative definition when
VarDecl::getActingDefinition is called. This can lead to attributes
being dropped if they are attached to tentative definitions that
appear after the second one. This is because
VarDecl::getActingDefinition loops through VarDecl::redecls assuming
that the last tentative definition is the last element in the
iterator. However, it is the second element that would be the last
tentative definition. This changeset modifies getActingDefinition to
iterate through the declaration chain in reverse, so that it can
immediately return when it encounters a tentative definition.

Diff Detail

Event Timeline

pestctrl requested review of this revision.Apr 1 2021, 7:46 AM
pestctrl created this revision.
pestctrl edited the summary of this revision. (Show Details)Apr 1 2021, 7:46 AM
pestctrl edited the summary of this revision. (Show Details)Apr 1 2021, 7:49 AM
pestctrl edited the summary of this revision. (Show Details)Apr 1 2021, 7:52 AM
akyrtzi resigned from this revision.Apr 1 2021, 9:33 AM
rsmith added a comment.Apr 1 2021, 4:12 PM

Thanks, this makes sense.

clang/lib/AST/Decl.cpp
2215

Is there a good reason to switch from checking for a Definition in the loop to checking it ahead of time? This change means we'll do two passes over the redeclarations, and call isThisDeclarationADefinition twice for each, where the old approach only performed one pass.

pestctrl updated this revision to Diff 335113.Apr 3 2021, 3:00 PM
pestctrl edited the summary of this revision. (Show Details)

Removed extra pass over decl chain for acting definition.

clang/lib/AST/Decl.cpp
2215

My thought was that separating the logic would make the loop easier to read, but I see that this causes 2 passes vs. 1.

pestctrl marked an inline comment as done.Jul 3 2021, 6:45 AM

@rsmith Ping!

rsmith accepted this revision.Jul 7 2021, 1:26 PM

Thanks!

clang/lib/AST/Decl.cpp
2226–2228

Might help to clarify what we're aiming for?

This revision is now accepted and ready to land.Jul 7 2021, 1:26 PM

Do you need help merging this?

pestctrl updated this revision to Diff 366513.Aug 15 2021, 11:17 AM

Update comment

pestctrl marked an inline comment as done.Aug 15 2021, 11:17 AM
pestctrl updated this revision to Diff 366522.Aug 15 2021, 1:16 PM

Only look for attributes in check string for unit test

@mizvekov Thanks for the help! I recently got commit access, so I think I can commit this myself.

thakis added a subscriber: thakis.Aug 24 2021, 7:31 AM

This breaks tests on macOS: http://45.33.8.238/macm1/16663/step_7.txt

Please to a look. Just adding a -triple flag is probably sufficient.