[AST] Pick last tentative definition as the acting definition

[pestctrl]

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.

[rsmith]

Thanks, this makes sense.

clang/lib/AST/Decl.cpp

2192

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]

Removed extra pass over decl chain for acting definition.

clang/lib/AST/Decl.cpp

2192

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]

Ping!

[pestctrl]

@rsmith Ping!

[rsmith]

Thanks!

clang/lib/AST/Decl.cpp

2203

Might help to clarify what we're aiming for?

[mizvekov]

Do you need help merging this?

[pestctrl]

Update comment

[pestctrl]

Only look for attributes in check string for unit test

[pestctrl]

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

[thakis]

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.

clang/lib/AST/Decl.cpp

VarDecl::isThisDeclarationADefinition(ASTContext &C) const {
 
   // What's left is (in C, block-scope) declarations without initializers or
   // external storage. These are definitions.
   return Definition;
 }
 
 VarDecl *VarDecl::getActingDefinition() {
   DefinitionKind Kind = isThisDeclarationADefinition();
   if (Kind != TentativeDefinition)
     return nullptr;
 
   VarDecl *LastTentative = nullptr;
-  VarDecl *First = getFirstDecl();
-  for (auto I : First->redecls()) {
-    Kind = I->isThisDeclarationADefinition();
+
+  // Loop through the declaration chain, starting with the most recent.
+  for (VarDecl *Decl = getMostRecentDecl(); Decl;
+       Decl = Decl->getPreviousDecl()) {
+    Kind = Decl->isThisDeclarationADefinition();
     if (Kind == Definition)
       return nullptr;
-    if (Kind == TentativeDefinition)
-      LastTentative = I;
+    // Record the first TentativeDefinition that is encountered.
+    if (Kind == TentativeDefinition && !LastTentative)
+      LastTentative = Decl;
   }
+
   return LastTentative;
 }
 
 VarDecl *VarDecl::getDefinition(ASTContext &C) {
   VarDecl *First = getFirstDecl();
   for (auto I : First->redecls()) {
     if (I->isThisDeclarationADefinition(C) == Definition)
       return I;

clang/test/CodeGen/attr-tentative-definition.c

+// RUN: %clang_cc1 -emit-llvm < %s | FileCheck %s
+
+char arr[10];
+char arr[10] __attribute__((section("datadata")));
+char arr[10] __attribute__((aligned(16)));
+
+// CHECK: @arr = dso_local global [10 x i8] zeroinitializer, section "datadata", align 16