[PATCH] D99732: [AST] Pick last tentative definition as the acting definition
thakis added a comment. 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99732/new/ https://reviews.llvm.org/D99732 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D99732: [AST] Pick last tentative definition as the acting definition
This revision was automatically updated to reflect the committed changes.
Closed by commit rG9a5f38885056: [AST] Pick last tentative definition as the
acting definition (authored by pestctrl).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D99732/new/
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 ={{.*}}section "datadata", align 16
Index: clang/lib/AST/Decl.cpp
===
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -2216,14 +2216,18 @@
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 (most recent) TentativeDefinition that is encountered.
+if (Kind == TentativeDefinition && !LastTentative)
+ LastTentative = Decl;
}
+
return LastTentative;
}
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 ={{.*}}section "datadata", align 16
Index: clang/lib/AST/Decl.cpp
===
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -2216,14 +2216,18 @@
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 (most recent) TentativeDefinition that is encountered.
+if (Kind == TentativeDefinition && !LastTentative)
+ LastTentative = Decl;
}
+
return LastTentative;
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D99732: [AST] Pick last tentative definition as the acting definition
pestctrl added a comment. @mizvekov Thanks for the help! I recently got commit access, so I think I can commit this myself. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99732/new/ https://reviews.llvm.org/D99732 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D99732: [AST] Pick last tentative definition as the acting definition
pestctrl updated this revision to Diff 366522.
pestctrl added a comment.
Only look for attributes in check string for unit test
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D99732/new/
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 ={{.*}}section "datadata", align 16
Index: clang/lib/AST/Decl.cpp
===
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -2216,14 +2216,18 @@
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 (most recent) TentativeDefinition that is encountered.
+if (Kind == TentativeDefinition && !LastTentative)
+ LastTentative = Decl;
}
+
return LastTentative;
}
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 ={{.*}}section "datadata", align 16
Index: clang/lib/AST/Decl.cpp
===
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -2216,14 +2216,18 @@
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 (most recent) TentativeDefinition that is encountered.
+if (Kind == TentativeDefinition && !LastTentative)
+ LastTentative = Decl;
}
+
return LastTentative;
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D99732: [AST] Pick last tentative definition as the acting definition
pestctrl updated this revision to Diff 366513.
pestctrl added a comment.
Update comment
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D99732/new/
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
@@ -2216,14 +2216,18 @@
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 (most recent) TentativeDefinition that is encountered.
+if (Kind == TentativeDefinition && !LastTentative)
+ LastTentative = Decl;
}
+
return LastTentative;
}
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
@@ -2216,14 +2216,18 @@
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 (most recent) TentativeDefinition that is encountered.
+if (Kind == TentativeDefinition && !LastTentative)
+ LastTentative = Decl;
}
+
return LastTentative;
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D99732: [AST] Pick last tentative definition as the acting definition
mizvekov added a comment. Do you need help merging this? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99732/new/ https://reviews.llvm.org/D99732 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D99732: [AST] Pick last tentative definition as the acting definition
rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Thanks! Comment at: clang/lib/AST/Decl.cpp:2203 return nullptr; -if (Kind == TentativeDefinition) - LastTentative = I; +// Record the first TentativeDefinition that is encountered. +if (Kind == TentativeDefinition && !LastTentative) Might help to clarify what we're aiming for? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99732/new/ https://reviews.llvm.org/D99732 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D99732: [AST] Pick last tentative definition as the acting definition
pestctrl marked an inline comment as done. pestctrl added a comment. @rsmith Ping! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99732/new/ https://reviews.llvm.org/D99732 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D99732: [AST] Pick last tentative definition as the acting definition
pestctrl added a comment. Ping! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99732/new/ https://reviews.llvm.org/D99732 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D99732: [AST] Pick last tentative definition as the acting definition
pestctrl added inline comments. Comment at: clang/lib/AST/Decl.cpp:2192 DefinitionKind Kind = isThisDeclarationADefinition(); - if (Kind != TentativeDefinition) + if (Kind != TentativeDefinition || hasDefinition()) return nullptr; rsmith wrote: > 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. My thought was that separating the logic would make the loop easier to read, but I see that this causes 2 passes vs. 1. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99732/new/ https://reviews.llvm.org/D99732 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D99732: [AST] Pick last tentative definition as the acting definition
pestctrl updated this revision to Diff 335113.
pestctrl edited the summary of this revision.
pestctrl added a comment.
Removed extra pass over decl chain for acting definition.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D99732/new/
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
@@ -2193,14 +2193,18 @@
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;
}
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
@@ -2193,14 +2193,18 @@
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;
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D99732: [AST] Pick last tentative definition as the acting definition
rsmith added a comment. Thanks, this makes sense. Comment at: clang/lib/AST/Decl.cpp:2192 DefinitionKind Kind = isThisDeclarationADefinition(); - if (Kind != TentativeDefinition) + if (Kind != TentativeDefinition || hasDefinition()) return nullptr; 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99732/new/ https://reviews.llvm.org/D99732 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D99732: [AST] Pick last tentative definition as the acting definition
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 ) {
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;
+
