[PATCH] D99732: [AST] Pick last tentative definition as the acting definition

[Benson Chu via Phabricator via cfe-commits]

pestctrl created this revision.
pestctrl added reviewers: akyrtzi, rsmith.
pestctrl added a project: clang.
pestctrl requested review of this revision.
Herald added a subscriber: cfe-commits.

I noticed this bug because attributes were being dropped from tentative
definitions after the second tentative definition. If you have the following C
code:

char chararr[13];
char chararr[13] __attribute__ ((section("data")));
char chararr[13] __attribute__ ((aligned(16)));

If you emit the LLVM IR, the chararr will have the section attribute, but not
the aligned attribute. If you have more tentative definitions following the
last line, those attributes will be dropped as well.

This is not a problem with merging of attributes, as that works completely
fine. Clang will store a merged list of attributes in the most recent tentative
definition encountered. The problem is that clang will choose one of the
tentative definitions to emit into LLVM IR, and it's NOT the most recent
tentative definition.

VarDecl::getActingDefinition is called to choose the which tentative definition
should be emitted. VarDecl::getActingDefinition loops through VarDecl::redecls,
which will return an iterator of all redeclarations of a variable, which will
include all tentative definitions. VarDecl::getActingDefinition assumes that
the order of this iterator is the order in which the defs/decls appear in the
file, but this is not the case. The first element of the iterator is in fact
the first def/decl, but the rest of the elements are reversed. So, if we had 4
def/decls, the order they appear in the iterator would be [1, 4, 3, 2].

So, when VarDecl::getActingDefinition loops through VarDecl::redecls, picking
the last element in the iterator, it's actually picking the second
decl/def. Because merged attributes appear in the most recent def/decl, any
attributes that appear after the second decl are dropped.

This changeset modifies VarDecl::getActingDefinition to use some helper
functions - namely getMostRecentDecl and getPreviousDecl - to instead loop
through the declaration chain in reverse order of how they appear in the file,
returning the first instance that is a tentative definition. This means
attributes will no longer be dropped.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D99732

Files:
  clang/lib/AST/Decl.cpp
  clang/test/CodeGen/attr-tentative-definition.c


Index: clang/test/CodeGen/attr-tentative-definition.c
===================================================================
--- /dev/null
+++ clang/test/CodeGen/attr-tentative-definition.c
@@ -0,0 +1,7 @@
+// 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
Index: clang/lib/AST/Decl.cpp
===================================================================
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -2189,19 +2189,20 @@
 
 VarDecl *VarDecl::getActingDefinition() {
   DefinitionKind Kind = isThisDeclarationADefinition();
-  if (Kind != TentativeDefinition)
+  if (Kind != TentativeDefinition || hasDefinition())
     return nullptr;
 
-  VarDecl *LastTentative = nullptr;
-  VarDecl *First = getFirstDecl();
-  for (auto I : First->redecls()) {
-    Kind = I->isThisDeclarationADefinition();
-    if (Kind == Definition)
-      return nullptr;
+  // Loop through the declaration chain, starting with the most recent.
+  for (VarDecl *Decl = getMostRecentDecl(); Decl;
+       Decl = Decl->getPreviousDecl()) {
+    Kind = Decl->isThisDeclarationADefinition();
+
+    // Return the first TentativeDefinition that is encountered.
     if (Kind == TentativeDefinition)
-      LastTentative = I;
+      return Decl;
   }
-  return LastTentative;
+
+  return nullptr;
 }
 
 VarDecl *VarDecl::getDefinition(ASTContext &C) {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D99732.334683.patch
Type: text/x-patch
Size: 1535 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20210401/0ac3b21e/attachment.bin>