[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
Michael137 wrote: > For CGExprConstant, the code is checking "empty" in the sense of whether > there's a corresponding LLVM field. So almost certainly needs changes. Not > sure how that isn't causing test failures; maybe there's missing test > coverage. Yea I was pretty sure we'd have to adjust that too. Will try to get some coverage here https://github.com/llvm/llvm-project/pull/96422 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
https://github.com/Michael137 updated
https://github.com/llvm/llvm-project/pull/96422
>From 419af1b1db2bef88c5145276ffd24ed93bcb15a5 Mon Sep 17 00:00:00 2001
From: Michael Buch
Date: Fri, 21 Jun 2024 12:15:07 +0100
Subject: [PATCH 1/7] [clang][CGRecordLayout] Remove dependency on isZeroSize
This is a follow-up from the conversation starting at
https://github.com/llvm/llvm-project/pull/93809#issuecomment-2173729801
The root problem that motivated the change are external AST
sources that compute `ASTRecordLayout`s themselves instead of
letting Clang compute them from the AST. One such examples is
LLDB using DWARF to get the definitive offsets and sizes of C++
structures. Such layouts should be considered correct (modulo
buggy DWARF), but various assertions and lowering logic around
the `CGRecordLayoutBuilder` relies on the AST having
`[[no_unique_address]]` attached to them. This is a layout-altering
attribute which is not encoded in DWARF. This causes us LLDB to trip
over the various LLVM<->Clang layout consistency checks. There has been
precedent for avoiding such layout-altering attributes to affect
lowering with externally-provided layouts (e.g., packed structs).
This patch proposes to replace the `isZeroSize` checks in
`CGRecordLayoutBuilder` (which roughly means "empty field
with [[no_unique_address]]") with checks for
`CodeGen::isEmptyField`/`CodeGen::isEmptyRecord`.
---
clang/lib/CodeGen/ABIInfoImpl.cpp | 6 ++---
clang/lib/CodeGen/ABIInfoImpl.h | 6 ++---
clang/lib/CodeGen/CGExpr.cpp | 3 ++-
clang/lib/CodeGen/CGRecordLayoutBuilder.cpp | 14 ++--
clang/test/CodeGen/X86/x86_64-vaarg.c | 3 ++-
clang/test/CodeGen/debug-info-packed-struct.c | 2 +-
clang/test/CodeGen/paren-list-agg-init.cpp| 15 +
.../CodeGenCXX/2011-12-19-init-list-ctor.cpp | 6 ++---
clang/test/CodeGenCXX/auto-var-init.cpp | 10 -
.../test/CodeGenCXX/bitfield-access-empty.cpp | 16 +++---
clang/test/CodeGenCXX/class-layout.cpp| 2 +-
clang/test/CodeGenCXX/compound-literals.cpp | 2 +-
clang/test/CodeGenCXX/exceptions.cpp | 7 +++---
.../lambda-deterministic-captures.cpp | 4 +---
clang/test/CodeGenCXX/ms_struct.cpp | 4 +---
clang/test/CodeGenCXX/partial-destruction.cpp | 11 --
clang/test/CodeGenCXX/pr18962.cpp | 5 ++---
clang/test/CodeGenCXX/references.cpp | 4 +---
clang/test/CodeGenCXX/temporaries.cpp | 12 +-
clang/test/CodeGenObjCXX/lambda-to-block.mm | 9
clang/test/OpenMP/irbuilder_for_iterator.cpp | 10 -
clang/test/OpenMP/irbuilder_for_rangefor.cpp | 14 +---
.../test/OpenMP/task_member_call_codegen.cpp | 22 ---
clang/test/Sema/ms_class_layout.cpp | 14 ++--
24 files changed, 85 insertions(+), 116 deletions(-)
diff --git a/clang/lib/CodeGen/ABIInfoImpl.cpp
b/clang/lib/CodeGen/ABIInfoImpl.cpp
index e9a26abb77837..92fcc3bc2c5f4 100644
--- a/clang/lib/CodeGen/ABIInfoImpl.cpp
+++ b/clang/lib/CodeGen/ABIInfoImpl.cpp
@@ -248,7 +248,7 @@ Address CodeGen::emitMergePHI(CodeGenFunction &CGF, Address
Addr1,
return Address(PHI, Addr1.getElementType(), Align);
}
-bool CodeGen::isEmptyField(ASTContext &Context, const FieldDecl *FD,
+bool CodeGen::isEmptyField(const ASTContext &Context, const FieldDecl *FD,
bool AllowArrays, bool AsIfNoUniqueAddr) {
if (FD->isUnnamedBitField())
return true;
@@ -289,8 +289,8 @@ bool CodeGen::isEmptyField(ASTContext &Context, const
FieldDecl *FD,
return isEmptyRecord(Context, FT, AllowArrays, AsIfNoUniqueAddr);
}
-bool CodeGen::isEmptyRecord(ASTContext &Context, QualType T, bool AllowArrays,
-bool AsIfNoUniqueAddr) {
+bool CodeGen::isEmptyRecord(const ASTContext &Context, QualType T,
+bool AllowArrays, bool AsIfNoUniqueAddr) {
const RecordType *RT = T->getAs();
if (!RT)
return false;
diff --git a/clang/lib/CodeGen/ABIInfoImpl.h b/clang/lib/CodeGen/ABIInfoImpl.h
index 92986fb431646..e3f373e39c35a 100644
--- a/clang/lib/CodeGen/ABIInfoImpl.h
+++ b/clang/lib/CodeGen/ABIInfoImpl.h
@@ -126,15 +126,15 @@ Address emitMergePHI(CodeGenFunction &CGF, Address Addr1,
/// is an unnamed bit-field or an (array of) empty record(s). If
/// AsIfNoUniqueAddr is true, then C++ record fields are considered empty if
/// the [[no_unique_address]] attribute would have made them empty.
-bool isEmptyField(ASTContext &Context, const FieldDecl *FD, bool AllowArrays,
- bool AsIfNoUniqueAddr = false);
+bool isEmptyField(const ASTContext &Context, const FieldDecl *FD,
+ bool AllowArrays, bool AsIfNoUniqueAddr = false);
/// isEmptyRecord - Return true iff a structure contains only empty
/// fields. Note that a structure with a flexible array member is not
/// considered empty. If AsIfNoUniqueAddr is true, then C++ r
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
https://github.com/rnk approved this pull request. I think this looks good, then. Any other concerns? https://github.com/llvm/llvm-project/pull/96422 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
Michael137 wrote: Had to adjust `isEmptyFieldForLayout`/`isEmptyRecordForLayout` because `isEmptyField`/`isEmptyRecord` would call each other recursively, so dispatching from `isEmptyFieldForLayout` wouldn't have been correct (since the `isEmptyField`/`isEmptyRecord` have extra checks for unnamed bitfields and arrays which didn't make sense in the context of our emptiness check, correct me if I'm wrong here). > For CGClass, it's not directly tied to the LLVM structure layout, but I'm not > sure the generated code would be semantically correct if an "empty" field > that isn't isEmpty() overlaps with actual data. I haven't addressed this yet. To clarify, are you referring to `addMemcpyableField` and `PushCleanupForField` (both of which check `isZeroSize`? So if we generated an overlap between an "empty" (but not `isEmpty`/`isZeroSize`) field and a non-empty field, previously `addMemcpyableField` wouldn't have considered the field for inclusion in the memcpy? So we should adjust the `isZeroSize` here too? Doing so didn't reflect in the test-suite, so it's possible there's some missing coverage here too https://github.com/llvm/llvm-project/pull/96422 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
https://github.com/Michael137 updated
https://github.com/llvm/llvm-project/pull/96422
>From 7fb44b1cd3ffac0a0f509cefa3a141dac50da142 Mon Sep 17 00:00:00 2001
From: Michael Buch
Date: Fri, 21 Jun 2024 12:15:07 +0100
Subject: [PATCH 1/8] [clang][CGRecordLayout] Remove dependency on isZeroSize
This is a follow-up from the conversation starting at
https://github.com/llvm/llvm-project/pull/93809#issuecomment-2173729801
The root problem that motivated the change are external AST
sources that compute `ASTRecordLayout`s themselves instead of
letting Clang compute them from the AST. One such examples is
LLDB using DWARF to get the definitive offsets and sizes of C++
structures. Such layouts should be considered correct (modulo
buggy DWARF), but various assertions and lowering logic around
the `CGRecordLayoutBuilder` relies on the AST having
`[[no_unique_address]]` attached to them. This is a layout-altering
attribute which is not encoded in DWARF. This causes us LLDB to trip
over the various LLVM<->Clang layout consistency checks. There has been
precedent for avoiding such layout-altering attributes to affect
lowering with externally-provided layouts (e.g., packed structs).
This patch proposes to replace the `isZeroSize` checks in
`CGRecordLayoutBuilder` (which roughly means "empty field
with [[no_unique_address]]") with checks for
`CodeGen::isEmptyField`/`CodeGen::isEmptyRecord`.
---
clang/lib/CodeGen/ABIInfoImpl.cpp | 6 ++---
clang/lib/CodeGen/ABIInfoImpl.h | 6 ++---
clang/lib/CodeGen/CGExpr.cpp | 3 ++-
clang/lib/CodeGen/CGRecordLayoutBuilder.cpp | 14 ++--
clang/test/CodeGen/X86/x86_64-vaarg.c | 3 ++-
clang/test/CodeGen/debug-info-packed-struct.c | 2 +-
clang/test/CodeGen/paren-list-agg-init.cpp| 15 +
.../CodeGenCXX/2011-12-19-init-list-ctor.cpp | 6 ++---
clang/test/CodeGenCXX/auto-var-init.cpp | 10 -
.../test/CodeGenCXX/bitfield-access-empty.cpp | 16 +++---
clang/test/CodeGenCXX/class-layout.cpp| 2 +-
clang/test/CodeGenCXX/compound-literals.cpp | 2 +-
clang/test/CodeGenCXX/exceptions.cpp | 7 +++---
.../lambda-deterministic-captures.cpp | 4 +---
clang/test/CodeGenCXX/ms_struct.cpp | 4 +---
clang/test/CodeGenCXX/partial-destruction.cpp | 11 --
clang/test/CodeGenCXX/pr18962.cpp | 5 ++---
clang/test/CodeGenCXX/references.cpp | 4 +---
clang/test/CodeGenCXX/temporaries.cpp | 12 +-
clang/test/CodeGenObjCXX/lambda-to-block.mm | 9
clang/test/OpenMP/irbuilder_for_iterator.cpp | 10 -
clang/test/OpenMP/irbuilder_for_rangefor.cpp | 14 +---
.../test/OpenMP/task_member_call_codegen.cpp | 22 ---
clang/test/Sema/ms_class_layout.cpp | 14 ++--
24 files changed, 85 insertions(+), 116 deletions(-)
diff --git a/clang/lib/CodeGen/ABIInfoImpl.cpp
b/clang/lib/CodeGen/ABIInfoImpl.cpp
index e9a26abb778375..92fcc3bc2c5f4c 100644
--- a/clang/lib/CodeGen/ABIInfoImpl.cpp
+++ b/clang/lib/CodeGen/ABIInfoImpl.cpp
@@ -248,7 +248,7 @@ Address CodeGen::emitMergePHI(CodeGenFunction &CGF, Address
Addr1,
return Address(PHI, Addr1.getElementType(), Align);
}
-bool CodeGen::isEmptyField(ASTContext &Context, const FieldDecl *FD,
+bool CodeGen::isEmptyField(const ASTContext &Context, const FieldDecl *FD,
bool AllowArrays, bool AsIfNoUniqueAddr) {
if (FD->isUnnamedBitField())
return true;
@@ -289,8 +289,8 @@ bool CodeGen::isEmptyField(ASTContext &Context, const
FieldDecl *FD,
return isEmptyRecord(Context, FT, AllowArrays, AsIfNoUniqueAddr);
}
-bool CodeGen::isEmptyRecord(ASTContext &Context, QualType T, bool AllowArrays,
-bool AsIfNoUniqueAddr) {
+bool CodeGen::isEmptyRecord(const ASTContext &Context, QualType T,
+bool AllowArrays, bool AsIfNoUniqueAddr) {
const RecordType *RT = T->getAs();
if (!RT)
return false;
diff --git a/clang/lib/CodeGen/ABIInfoImpl.h b/clang/lib/CodeGen/ABIInfoImpl.h
index 92986fb4316465..e3f373e39c35a5 100644
--- a/clang/lib/CodeGen/ABIInfoImpl.h
+++ b/clang/lib/CodeGen/ABIInfoImpl.h
@@ -126,15 +126,15 @@ Address emitMergePHI(CodeGenFunction &CGF, Address Addr1,
/// is an unnamed bit-field or an (array of) empty record(s). If
/// AsIfNoUniqueAddr is true, then C++ record fields are considered empty if
/// the [[no_unique_address]] attribute would have made them empty.
-bool isEmptyField(ASTContext &Context, const FieldDecl *FD, bool AllowArrays,
- bool AsIfNoUniqueAddr = false);
+bool isEmptyField(const ASTContext &Context, const FieldDecl *FD,
+ bool AllowArrays, bool AsIfNoUniqueAddr = false);
/// isEmptyRecord - Return true iff a structure contains only empty
/// fields. Note that a structure with a flexible array member is not
/// considered empty. If AsIfNoUniqueAddr is true, then C
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
efriedma-quic wrote: > > For CGClass, it's not directly tied to the LLVM structure layout, but I'm > > not sure the generated code would be semantically correct if an "empty" > > field that isn't isEmpty() overlaps with actual data. > > I haven't addressed this yet. To clarify, are you referring to > `addMemcpyableField` and `PushCleanupForField` (both of which check > `isZeroSize`? So if we generated an overlap between an "empty" (but not > `isEmpty`/`isZeroSize`) field and a non-empty field, previously > `addMemcpyableField` wouldn't have considered the field for inclusion in the > memcpy? So we should adjust the `isZeroSize` here too? Doing so didn't > reflect in the test-suite, so it's possible there's some missing coverage > here too In addMemcpyableField(), if we conclude a field is not zero-size (and the other heuristics pass), we generate a memcpy for it. But if the field is actually empty, it might overlap with some other field, so the memcpy() might actually end up overwriting something unrelated. Similarly, for PushCleanupForField, we'll poison any overlapping data. In both cases, if the field doesn't actually contain any data, we can safely skip the relevant code. I wouldn't be surprised if there's missing coverage here. https://github.com/llvm/llvm-project/pull/96422 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
efriedma-quic wrote: It looks like in the latest patch, there's still a couple uses of isZeroSize() in CGExprConstant? https://github.com/llvm/llvm-project/pull/96422 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
https://github.com/Michael137 updated
https://github.com/llvm/llvm-project/pull/96422
>From 07e603f7afc98e5af31962a5b1f44f4e4c079ebb Mon Sep 17 00:00:00 2001
From: Michael Buch
Date: Fri, 21 Jun 2024 12:15:07 +0100
Subject: [PATCH 1/9] [clang][CGRecordLayout] Remove dependency on isZeroSize
This is a follow-up from the conversation starting at
https://github.com/llvm/llvm-project/pull/93809#issuecomment-2173729801
The root problem that motivated the change are external AST
sources that compute `ASTRecordLayout`s themselves instead of
letting Clang compute them from the AST. One such examples is
LLDB using DWARF to get the definitive offsets and sizes of C++
structures. Such layouts should be considered correct (modulo
buggy DWARF), but various assertions and lowering logic around
the `CGRecordLayoutBuilder` relies on the AST having
`[[no_unique_address]]` attached to them. This is a layout-altering
attribute which is not encoded in DWARF. This causes us LLDB to trip
over the various LLVM<->Clang layout consistency checks. There has been
precedent for avoiding such layout-altering attributes to affect
lowering with externally-provided layouts (e.g., packed structs).
This patch proposes to replace the `isZeroSize` checks in
`CGRecordLayoutBuilder` (which roughly means "empty field
with [[no_unique_address]]") with checks for
`CodeGen::isEmptyField`/`CodeGen::isEmptyRecord`.
---
clang/lib/CodeGen/ABIInfoImpl.cpp | 6 ++---
clang/lib/CodeGen/ABIInfoImpl.h | 6 ++---
clang/lib/CodeGen/CGExpr.cpp | 3 ++-
clang/lib/CodeGen/CGRecordLayoutBuilder.cpp | 14 ++--
clang/test/CodeGen/X86/x86_64-vaarg.c | 3 ++-
clang/test/CodeGen/debug-info-packed-struct.c | 2 +-
clang/test/CodeGen/paren-list-agg-init.cpp| 15 +
.../CodeGenCXX/2011-12-19-init-list-ctor.cpp | 6 ++---
clang/test/CodeGenCXX/auto-var-init.cpp | 10 -
.../test/CodeGenCXX/bitfield-access-empty.cpp | 16 +++---
clang/test/CodeGenCXX/class-layout.cpp| 2 +-
clang/test/CodeGenCXX/compound-literals.cpp | 2 +-
clang/test/CodeGenCXX/exceptions.cpp | 7 +++---
.../lambda-deterministic-captures.cpp | 4 +---
clang/test/CodeGenCXX/ms_struct.cpp | 4 +---
clang/test/CodeGenCXX/partial-destruction.cpp | 11 --
clang/test/CodeGenCXX/pr18962.cpp | 5 ++---
clang/test/CodeGenCXX/references.cpp | 4 +---
clang/test/CodeGenCXX/temporaries.cpp | 12 +-
clang/test/CodeGenObjCXX/lambda-to-block.mm | 9
clang/test/OpenMP/irbuilder_for_iterator.cpp | 10 -
clang/test/OpenMP/irbuilder_for_rangefor.cpp | 14 +---
.../test/OpenMP/task_member_call_codegen.cpp | 22 ---
clang/test/Sema/ms_class_layout.cpp | 14 ++--
24 files changed, 85 insertions(+), 116 deletions(-)
diff --git a/clang/lib/CodeGen/ABIInfoImpl.cpp
b/clang/lib/CodeGen/ABIInfoImpl.cpp
index e9a26abb77837..92fcc3bc2c5f4 100644
--- a/clang/lib/CodeGen/ABIInfoImpl.cpp
+++ b/clang/lib/CodeGen/ABIInfoImpl.cpp
@@ -248,7 +248,7 @@ Address CodeGen::emitMergePHI(CodeGenFunction &CGF, Address
Addr1,
return Address(PHI, Addr1.getElementType(), Align);
}
-bool CodeGen::isEmptyField(ASTContext &Context, const FieldDecl *FD,
+bool CodeGen::isEmptyField(const ASTContext &Context, const FieldDecl *FD,
bool AllowArrays, bool AsIfNoUniqueAddr) {
if (FD->isUnnamedBitField())
return true;
@@ -289,8 +289,8 @@ bool CodeGen::isEmptyField(ASTContext &Context, const
FieldDecl *FD,
return isEmptyRecord(Context, FT, AllowArrays, AsIfNoUniqueAddr);
}
-bool CodeGen::isEmptyRecord(ASTContext &Context, QualType T, bool AllowArrays,
-bool AsIfNoUniqueAddr) {
+bool CodeGen::isEmptyRecord(const ASTContext &Context, QualType T,
+bool AllowArrays, bool AsIfNoUniqueAddr) {
const RecordType *RT = T->getAs();
if (!RT)
return false;
diff --git a/clang/lib/CodeGen/ABIInfoImpl.h b/clang/lib/CodeGen/ABIInfoImpl.h
index 92986fb431646..e3f373e39c35a 100644
--- a/clang/lib/CodeGen/ABIInfoImpl.h
+++ b/clang/lib/CodeGen/ABIInfoImpl.h
@@ -126,15 +126,15 @@ Address emitMergePHI(CodeGenFunction &CGF, Address Addr1,
/// is an unnamed bit-field or an (array of) empty record(s). If
/// AsIfNoUniqueAddr is true, then C++ record fields are considered empty if
/// the [[no_unique_address]] attribute would have made them empty.
-bool isEmptyField(ASTContext &Context, const FieldDecl *FD, bool AllowArrays,
- bool AsIfNoUniqueAddr = false);
+bool isEmptyField(const ASTContext &Context, const FieldDecl *FD,
+ bool AllowArrays, bool AsIfNoUniqueAddr = false);
/// isEmptyRecord - Return true iff a structure contains only empty
/// fields. Note that a structure with a flexible array member is not
/// considered empty. If AsIfNoUniqueAddr is true, then C++ r
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
Michael137 wrote: > > > For CGClass, it's not directly tied to the LLVM structure layout, but I'm > > > not sure the generated code would be semantically correct if an "empty" > > > field that isn't isEmpty() overlaps with actual data. > > > > > > I haven't addressed this yet. To clarify, are you referring to > > `addMemcpyableField` and `PushCleanupForField` (both of which check > > `isZeroSize`? So if we generated an overlap between an "empty" (but not > > `isEmpty`/`isZeroSize`) field and a non-empty field, previously > > `addMemcpyableField` wouldn't have considered the field for inclusion in > > the memcpy? So we should adjust the `isZeroSize` here too? Doing so didn't > > reflect in the test-suite, so it's possible there's some missing coverage > > here too > > In addMemcpyableField(), if we conclude a field is not zero-size (and the > other heuristics pass), we generate a memcpy for it. But if the field is > actually empty, it might overlap with some other field, so the memcpy() might > actually end up overwriting something unrelated. Similarly, for > PushCleanupForField, we'll poison any overlapping data. In both cases, if the > field doesn't actually contain any data, we can safely skip the relevant code. > > I wouldn't be surprised if there's missing coverage here. Makes sense! > It looks like in the latest patch, there's still a couple uses of > isZeroSize() in CGExprConstant? Yea those were the last occurrences I wanted to sanity check since they're calling into `ASTRecordLayout::getFieldOffset` (which is built based on `isZeroSize`) and there's a `NoUniqueAddressAttr` check in there too. But it seemed reasonable to skip empty fields here too. Had to adjust the `CodeGen/2009-06-14-anonymous-union-init.c` test which checks for a `zeroinitializer` (which in the test happened to be for an empty structure, see latest commit). https://github.com/llvm/llvm-project/pull/96422 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
https://github.com/Michael137 updated
https://github.com/llvm/llvm-project/pull/96422
>From 07e603f7afc98e5af31962a5b1f44f4e4c079ebb Mon Sep 17 00:00:00 2001
From: Michael Buch
Date: Fri, 21 Jun 2024 12:15:07 +0100
Subject: [PATCH 01/10] [clang][CGRecordLayout] Remove dependency on isZeroSize
This is a follow-up from the conversation starting at
https://github.com/llvm/llvm-project/pull/93809#issuecomment-2173729801
The root problem that motivated the change are external AST
sources that compute `ASTRecordLayout`s themselves instead of
letting Clang compute them from the AST. One such examples is
LLDB using DWARF to get the definitive offsets and sizes of C++
structures. Such layouts should be considered correct (modulo
buggy DWARF), but various assertions and lowering logic around
the `CGRecordLayoutBuilder` relies on the AST having
`[[no_unique_address]]` attached to them. This is a layout-altering
attribute which is not encoded in DWARF. This causes us LLDB to trip
over the various LLVM<->Clang layout consistency checks. There has been
precedent for avoiding such layout-altering attributes to affect
lowering with externally-provided layouts (e.g., packed structs).
This patch proposes to replace the `isZeroSize` checks in
`CGRecordLayoutBuilder` (which roughly means "empty field
with [[no_unique_address]]") with checks for
`CodeGen::isEmptyField`/`CodeGen::isEmptyRecord`.
---
clang/lib/CodeGen/ABIInfoImpl.cpp | 6 ++---
clang/lib/CodeGen/ABIInfoImpl.h | 6 ++---
clang/lib/CodeGen/CGExpr.cpp | 3 ++-
clang/lib/CodeGen/CGRecordLayoutBuilder.cpp | 14 ++--
clang/test/CodeGen/X86/x86_64-vaarg.c | 3 ++-
clang/test/CodeGen/debug-info-packed-struct.c | 2 +-
clang/test/CodeGen/paren-list-agg-init.cpp| 15 +
.../CodeGenCXX/2011-12-19-init-list-ctor.cpp | 6 ++---
clang/test/CodeGenCXX/auto-var-init.cpp | 10 -
.../test/CodeGenCXX/bitfield-access-empty.cpp | 16 +++---
clang/test/CodeGenCXX/class-layout.cpp| 2 +-
clang/test/CodeGenCXX/compound-literals.cpp | 2 +-
clang/test/CodeGenCXX/exceptions.cpp | 7 +++---
.../lambda-deterministic-captures.cpp | 4 +---
clang/test/CodeGenCXX/ms_struct.cpp | 4 +---
clang/test/CodeGenCXX/partial-destruction.cpp | 11 --
clang/test/CodeGenCXX/pr18962.cpp | 5 ++---
clang/test/CodeGenCXX/references.cpp | 4 +---
clang/test/CodeGenCXX/temporaries.cpp | 12 +-
clang/test/CodeGenObjCXX/lambda-to-block.mm | 9
clang/test/OpenMP/irbuilder_for_iterator.cpp | 10 -
clang/test/OpenMP/irbuilder_for_rangefor.cpp | 14 +---
.../test/OpenMP/task_member_call_codegen.cpp | 22 ---
clang/test/Sema/ms_class_layout.cpp | 14 ++--
24 files changed, 85 insertions(+), 116 deletions(-)
diff --git a/clang/lib/CodeGen/ABIInfoImpl.cpp
b/clang/lib/CodeGen/ABIInfoImpl.cpp
index e9a26abb77837..92fcc3bc2c5f4 100644
--- a/clang/lib/CodeGen/ABIInfoImpl.cpp
+++ b/clang/lib/CodeGen/ABIInfoImpl.cpp
@@ -248,7 +248,7 @@ Address CodeGen::emitMergePHI(CodeGenFunction &CGF, Address
Addr1,
return Address(PHI, Addr1.getElementType(), Align);
}
-bool CodeGen::isEmptyField(ASTContext &Context, const FieldDecl *FD,
+bool CodeGen::isEmptyField(const ASTContext &Context, const FieldDecl *FD,
bool AllowArrays, bool AsIfNoUniqueAddr) {
if (FD->isUnnamedBitField())
return true;
@@ -289,8 +289,8 @@ bool CodeGen::isEmptyField(ASTContext &Context, const
FieldDecl *FD,
return isEmptyRecord(Context, FT, AllowArrays, AsIfNoUniqueAddr);
}
-bool CodeGen::isEmptyRecord(ASTContext &Context, QualType T, bool AllowArrays,
-bool AsIfNoUniqueAddr) {
+bool CodeGen::isEmptyRecord(const ASTContext &Context, QualType T,
+bool AllowArrays, bool AsIfNoUniqueAddr) {
const RecordType *RT = T->getAs();
if (!RT)
return false;
diff --git a/clang/lib/CodeGen/ABIInfoImpl.h b/clang/lib/CodeGen/ABIInfoImpl.h
index 92986fb431646..e3f373e39c35a 100644
--- a/clang/lib/CodeGen/ABIInfoImpl.h
+++ b/clang/lib/CodeGen/ABIInfoImpl.h
@@ -126,15 +126,15 @@ Address emitMergePHI(CodeGenFunction &CGF, Address Addr1,
/// is an unnamed bit-field or an (array of) empty record(s). If
/// AsIfNoUniqueAddr is true, then C++ record fields are considered empty if
/// the [[no_unique_address]] attribute would have made them empty.
-bool isEmptyField(ASTContext &Context, const FieldDecl *FD, bool AllowArrays,
- bool AsIfNoUniqueAddr = false);
+bool isEmptyField(const ASTContext &Context, const FieldDecl *FD,
+ bool AllowArrays, bool AsIfNoUniqueAddr = false);
/// isEmptyRecord - Return true iff a structure contains only empty
/// fields. Note that a structure with a flexible array member is not
/// considered empty. If AsIfNoUniqueAddr is true, then C++
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
Michael137 wrote: > I wouldn't be surprised if there's missing coverage here. Added a test for the `memcpy` part in the latest commit. https://github.com/llvm/llvm-project/pull/96422 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
@@ -185,6 +203,18 @@ CALL_AO(PackedMembers)
// CHECK: call void @llvm.memcpy.p0.p0.i64({{.*}} align 1 {{.*}} align 1
{{.*}}i64 16, i1 {{.*}})
// CHECK: ret ptr
+// WithEmptyField copy-assignment:
+// CHECK-LABEL: define linkonce_odr nonnull align {{[0-9]+}}
dereferenceable({{[0-9]+}}) ptr @_ZN14WithEmptyFieldaSERKS_
+// CHECK: call void @llvm.memcpy.p0.p0.i64({{.*}} align 4 {{.*}} align 4
{{.*}}i64 4, i1 {{.*}})
Michael137 wrote:
@efriedma-quic Before this patch, this would've been a `memcpy` of size `5`.
But the structure layout for `WithEmptyField` changed from:
```
%struct.WithEmptyField = type { i32, %struct.Empty, %struct.NonPOD, i32 }
%struct.Empty = type { i8 }
%struct.NonPOD = type { i16 }
```
to
```
struct.Foo = type { i32, %struct.NonPOD, i32 }
struct.NonPOD = type { i16 }
```
As you said, if we kept the `isZeroSize` check when generating this `memcpy` we
would've written too much.
https://github.com/llvm/llvm-project/pull/96422
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
https://github.com/Michael137 edited https://github.com/llvm/llvm-project/pull/96422 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
https://github.com/Michael137 edited https://github.com/llvm/llvm-project/pull/96422 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
https://github.com/Michael137 edited https://github.com/llvm/llvm-project/pull/96422 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
@@ -185,6 +203,18 @@ CALL_AO(PackedMembers)
// CHECK: call void @llvm.memcpy.p0.p0.i64({{.*}} align 1 {{.*}} align 1
{{.*}}i64 16, i1 {{.*}})
// CHECK: ret ptr
+// WithEmptyField copy-assignment:
+// CHECK-LABEL: define linkonce_odr nonnull align {{[0-9]+}}
dereferenceable({{[0-9]+}}) ptr @_ZN14WithEmptyFieldaSERKS_
+// CHECK: call void @llvm.memcpy.p0.p0.i64({{.*}} align 4 {{.*}} align 4
{{.*}}i64 4, i1 {{.*}})
dwblaikie wrote:
Sorry, jumping in here without a lot of context, but the change in
`%struct.WithEmptyField` seems wrong, doesn't it? Like that is a change in
layout/ABI break/bad thing? I guess I'm missing something about why that change
in layout would be acceptable? (maybe padding - if NonPOD gets aligned, such
that the structure isn't changing layout after all?)
https://github.com/llvm/llvm-project/pull/96422
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
@@ -185,6 +203,18 @@ CALL_AO(PackedMembers)
// CHECK: call void @llvm.memcpy.p0.p0.i64({{.*}} align 1 {{.*}} align 1
{{.*}}i64 16, i1 {{.*}})
// CHECK: ret ptr
+// WithEmptyField copy-assignment:
+// CHECK-LABEL: define linkonce_odr nonnull align {{[0-9]+}}
dereferenceable({{[0-9]+}}) ptr @_ZN14WithEmptyFieldaSERKS_
+// CHECK: call void @llvm.memcpy.p0.p0.i64({{.*}} align 4 {{.*}} align 4
{{.*}}i64 4, i1 {{.*}})
rnk wrote:
+1, at least looking at the LLVM struct types that you pasted here, the offset
of NonPOD in WithEmptyField has changed from 6 to 4, and that seems incorrect.
https://github.com/llvm/llvm-project/pull/96422
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
@@ -137,6 +137,16 @@ bool isEmptyField(ASTContext &Context, const FieldDecl *FD, bool AllowArrays, bool isEmptyRecord(ASTContext &Context, QualType T, bool AllowArrays, bool AsIfNoUniqueAddr = false); +/// isEmptyFieldForLayout - Return true iff the field is "empty", that is, +/// either a zero-width bit-field or an \ref isEmptyRecordForLayout. +bool isEmptyFieldForLayout(const ASTContext &Context, const FieldDecl *FD); + +/// isEmptyRecordForLayout - Return true iff a structure contains only empty efriedma-quic wrote: Empty base classes and fields. https://github.com/llvm/llvm-project/pull/96422 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
@@ -1,7 +1,17 @@
-// RUN: %clang_cc1 -emit-llvm < %s | grep "zeroinitializer, i16 16877"
+// RUN: %clang_cc1 %s -emit-llvm -o - | FileCheck %s
// PR4390
struct sysfs_dirent {
- union { struct sysfs_elem_dir {} s_dir; };
+ union { struct sysfs_elem_dir { int x; } s_dir; };
unsigned short s_mode;
};
struct sysfs_dirent sysfs_root = { {}, 16877 };
+
+// CHECK: @sysfs_root = global %struct.sysfs_dirent { %union.anon
zeroinitializer, i16 16877 }
+
+struct Foo {
+ union { struct empty {} x; };
+ unsigned short s_mode;
efriedma-quic wrote:
This looks weird at first glance, but I guess the union has size zero in C, so
this is fine.
https://github.com/llvm/llvm-project/pull/96422
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
@@ -185,6 +203,18 @@ CALL_AO(PackedMembers)
// CHECK: call void @llvm.memcpy.p0.p0.i64({{.*}} align 1 {{.*}} align 1
{{.*}}i64 16, i1 {{.*}})
// CHECK: ret ptr
+// WithEmptyField copy-assignment:
+// CHECK-LABEL: define linkonce_odr nonnull align {{[0-9]+}}
dereferenceable({{[0-9]+}}) ptr @_ZN14WithEmptyFieldaSERKS_
+// CHECK: call void @llvm.memcpy.p0.p0.i64({{.*}} align 4 {{.*}} align 4
{{.*}}i64 4, i1 {{.*}})
efriedma-quic wrote:
I assume that isn't actually what's happening, and there's somehow a typo in
the comment.
https://github.com/llvm/llvm-project/pull/96422
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
@@ -185,6 +203,18 @@ CALL_AO(PackedMembers)
// CHECK: call void @llvm.memcpy.p0.p0.i64({{.*}} align 1 {{.*}} align 1
{{.*}}i64 16, i1 {{.*}})
// CHECK: ret ptr
+// WithEmptyField copy-assignment:
+// CHECK-LABEL: define linkonce_odr nonnull align {{[0-9]+}}
dereferenceable({{[0-9]+}}) ptr @_ZN14WithEmptyFieldaSERKS_
+// CHECK: call void @llvm.memcpy.p0.p0.i64({{.*}} align 4 {{.*}} align 4
{{.*}}i64 4, i1 {{.*}})
Michael137 wrote:
Uhm yea that would be an ABI break, and I'm not sure how I got to this IR,
apologies for the confusion. What actually happens is that we get:
```
%struct.WithEmptyField = type { i32, [4 x i8], i32 }
```
which seems fine. The `memcpy` does change from `5` to 4`
https://github.com/llvm/llvm-project/pull/96422
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
https://github.com/Michael137 edited https://github.com/llvm/llvm-project/pull/96422 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
https://github.com/Michael137 updated
https://github.com/llvm/llvm-project/pull/96422
>From 07e603f7afc98e5af31962a5b1f44f4e4c079ebb Mon Sep 17 00:00:00 2001
From: Michael Buch
Date: Fri, 21 Jun 2024 12:15:07 +0100
Subject: [PATCH 01/11] [clang][CGRecordLayout] Remove dependency on isZeroSize
This is a follow-up from the conversation starting at
https://github.com/llvm/llvm-project/pull/93809#issuecomment-2173729801
The root problem that motivated the change are external AST
sources that compute `ASTRecordLayout`s themselves instead of
letting Clang compute them from the AST. One such examples is
LLDB using DWARF to get the definitive offsets and sizes of C++
structures. Such layouts should be considered correct (modulo
buggy DWARF), but various assertions and lowering logic around
the `CGRecordLayoutBuilder` relies on the AST having
`[[no_unique_address]]` attached to them. This is a layout-altering
attribute which is not encoded in DWARF. This causes us LLDB to trip
over the various LLVM<->Clang layout consistency checks. There has been
precedent for avoiding such layout-altering attributes to affect
lowering with externally-provided layouts (e.g., packed structs).
This patch proposes to replace the `isZeroSize` checks in
`CGRecordLayoutBuilder` (which roughly means "empty field
with [[no_unique_address]]") with checks for
`CodeGen::isEmptyField`/`CodeGen::isEmptyRecord`.
---
clang/lib/CodeGen/ABIInfoImpl.cpp | 6 ++---
clang/lib/CodeGen/ABIInfoImpl.h | 6 ++---
clang/lib/CodeGen/CGExpr.cpp | 3 ++-
clang/lib/CodeGen/CGRecordLayoutBuilder.cpp | 14 ++--
clang/test/CodeGen/X86/x86_64-vaarg.c | 3 ++-
clang/test/CodeGen/debug-info-packed-struct.c | 2 +-
clang/test/CodeGen/paren-list-agg-init.cpp| 15 +
.../CodeGenCXX/2011-12-19-init-list-ctor.cpp | 6 ++---
clang/test/CodeGenCXX/auto-var-init.cpp | 10 -
.../test/CodeGenCXX/bitfield-access-empty.cpp | 16 +++---
clang/test/CodeGenCXX/class-layout.cpp| 2 +-
clang/test/CodeGenCXX/compound-literals.cpp | 2 +-
clang/test/CodeGenCXX/exceptions.cpp | 7 +++---
.../lambda-deterministic-captures.cpp | 4 +---
clang/test/CodeGenCXX/ms_struct.cpp | 4 +---
clang/test/CodeGenCXX/partial-destruction.cpp | 11 --
clang/test/CodeGenCXX/pr18962.cpp | 5 ++---
clang/test/CodeGenCXX/references.cpp | 4 +---
clang/test/CodeGenCXX/temporaries.cpp | 12 +-
clang/test/CodeGenObjCXX/lambda-to-block.mm | 9
clang/test/OpenMP/irbuilder_for_iterator.cpp | 10 -
clang/test/OpenMP/irbuilder_for_rangefor.cpp | 14 +---
.../test/OpenMP/task_member_call_codegen.cpp | 22 ---
clang/test/Sema/ms_class_layout.cpp | 14 ++--
24 files changed, 85 insertions(+), 116 deletions(-)
diff --git a/clang/lib/CodeGen/ABIInfoImpl.cpp
b/clang/lib/CodeGen/ABIInfoImpl.cpp
index e9a26abb77837..92fcc3bc2c5f4 100644
--- a/clang/lib/CodeGen/ABIInfoImpl.cpp
+++ b/clang/lib/CodeGen/ABIInfoImpl.cpp
@@ -248,7 +248,7 @@ Address CodeGen::emitMergePHI(CodeGenFunction &CGF, Address
Addr1,
return Address(PHI, Addr1.getElementType(), Align);
}
-bool CodeGen::isEmptyField(ASTContext &Context, const FieldDecl *FD,
+bool CodeGen::isEmptyField(const ASTContext &Context, const FieldDecl *FD,
bool AllowArrays, bool AsIfNoUniqueAddr) {
if (FD->isUnnamedBitField())
return true;
@@ -289,8 +289,8 @@ bool CodeGen::isEmptyField(ASTContext &Context, const
FieldDecl *FD,
return isEmptyRecord(Context, FT, AllowArrays, AsIfNoUniqueAddr);
}
-bool CodeGen::isEmptyRecord(ASTContext &Context, QualType T, bool AllowArrays,
-bool AsIfNoUniqueAddr) {
+bool CodeGen::isEmptyRecord(const ASTContext &Context, QualType T,
+bool AllowArrays, bool AsIfNoUniqueAddr) {
const RecordType *RT = T->getAs();
if (!RT)
return false;
diff --git a/clang/lib/CodeGen/ABIInfoImpl.h b/clang/lib/CodeGen/ABIInfoImpl.h
index 92986fb431646..e3f373e39c35a 100644
--- a/clang/lib/CodeGen/ABIInfoImpl.h
+++ b/clang/lib/CodeGen/ABIInfoImpl.h
@@ -126,15 +126,15 @@ Address emitMergePHI(CodeGenFunction &CGF, Address Addr1,
/// is an unnamed bit-field or an (array of) empty record(s). If
/// AsIfNoUniqueAddr is true, then C++ record fields are considered empty if
/// the [[no_unique_address]] attribute would have made them empty.
-bool isEmptyField(ASTContext &Context, const FieldDecl *FD, bool AllowArrays,
- bool AsIfNoUniqueAddr = false);
+bool isEmptyField(const ASTContext &Context, const FieldDecl *FD,
+ bool AllowArrays, bool AsIfNoUniqueAddr = false);
/// isEmptyRecord - Return true iff a structure contains only empty
/// fields. Note that a structure with a flexible array member is not
/// considered empty. If AsIfNoUniqueAddr is true, then C++
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
@@ -1,7 +1,17 @@
-// RUN: %clang_cc1 -emit-llvm < %s | grep "zeroinitializer, i16 16877"
+// RUN: %clang_cc1 %s -emit-llvm -o - | FileCheck %s
// PR4390
struct sysfs_dirent {
- union { struct sysfs_elem_dir {} s_dir; };
+ union { struct sysfs_elem_dir { int x; } s_dir; };
unsigned short s_mode;
};
struct sysfs_dirent sysfs_root = { {}, 16877 };
+
+// CHECK: @sysfs_root = global %struct.sysfs_dirent { %union.anon
zeroinitializer, i16 16877 }
+
+struct Foo {
+ union { struct empty {} x; };
+ unsigned short s_mode;
Michael137 wrote:
Yup, can confirm that as C++ we'd get:
```
%struct.sysfs_dirent = type { [2 x i8], i16 }
@sysfs_root = global %struct.sysfs_dirent { [2 x i8] undef, i16 16877 }, align
2
```
https://github.com/llvm/llvm-project/pull/96422
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
https://github.com/Michael137 updated
https://github.com/llvm/llvm-project/pull/96422
>From 07e603f7afc98e5af31962a5b1f44f4e4c079ebb Mon Sep 17 00:00:00 2001
From: Michael Buch
Date: Fri, 21 Jun 2024 12:15:07 +0100
Subject: [PATCH 01/12] [clang][CGRecordLayout] Remove dependency on isZeroSize
This is a follow-up from the conversation starting at
https://github.com/llvm/llvm-project/pull/93809#issuecomment-2173729801
The root problem that motivated the change are external AST
sources that compute `ASTRecordLayout`s themselves instead of
letting Clang compute them from the AST. One such examples is
LLDB using DWARF to get the definitive offsets and sizes of C++
structures. Such layouts should be considered correct (modulo
buggy DWARF), but various assertions and lowering logic around
the `CGRecordLayoutBuilder` relies on the AST having
`[[no_unique_address]]` attached to them. This is a layout-altering
attribute which is not encoded in DWARF. This causes us LLDB to trip
over the various LLVM<->Clang layout consistency checks. There has been
precedent for avoiding such layout-altering attributes to affect
lowering with externally-provided layouts (e.g., packed structs).
This patch proposes to replace the `isZeroSize` checks in
`CGRecordLayoutBuilder` (which roughly means "empty field
with [[no_unique_address]]") with checks for
`CodeGen::isEmptyField`/`CodeGen::isEmptyRecord`.
---
clang/lib/CodeGen/ABIInfoImpl.cpp | 6 ++---
clang/lib/CodeGen/ABIInfoImpl.h | 6 ++---
clang/lib/CodeGen/CGExpr.cpp | 3 ++-
clang/lib/CodeGen/CGRecordLayoutBuilder.cpp | 14 ++--
clang/test/CodeGen/X86/x86_64-vaarg.c | 3 ++-
clang/test/CodeGen/debug-info-packed-struct.c | 2 +-
clang/test/CodeGen/paren-list-agg-init.cpp| 15 +
.../CodeGenCXX/2011-12-19-init-list-ctor.cpp | 6 ++---
clang/test/CodeGenCXX/auto-var-init.cpp | 10 -
.../test/CodeGenCXX/bitfield-access-empty.cpp | 16 +++---
clang/test/CodeGenCXX/class-layout.cpp| 2 +-
clang/test/CodeGenCXX/compound-literals.cpp | 2 +-
clang/test/CodeGenCXX/exceptions.cpp | 7 +++---
.../lambda-deterministic-captures.cpp | 4 +---
clang/test/CodeGenCXX/ms_struct.cpp | 4 +---
clang/test/CodeGenCXX/partial-destruction.cpp | 11 --
clang/test/CodeGenCXX/pr18962.cpp | 5 ++---
clang/test/CodeGenCXX/references.cpp | 4 +---
clang/test/CodeGenCXX/temporaries.cpp | 12 +-
clang/test/CodeGenObjCXX/lambda-to-block.mm | 9
clang/test/OpenMP/irbuilder_for_iterator.cpp | 10 -
clang/test/OpenMP/irbuilder_for_rangefor.cpp | 14 +---
.../test/OpenMP/task_member_call_codegen.cpp | 22 ---
clang/test/Sema/ms_class_layout.cpp | 14 ++--
24 files changed, 85 insertions(+), 116 deletions(-)
diff --git a/clang/lib/CodeGen/ABIInfoImpl.cpp
b/clang/lib/CodeGen/ABIInfoImpl.cpp
index e9a26abb77837..92fcc3bc2c5f4 100644
--- a/clang/lib/CodeGen/ABIInfoImpl.cpp
+++ b/clang/lib/CodeGen/ABIInfoImpl.cpp
@@ -248,7 +248,7 @@ Address CodeGen::emitMergePHI(CodeGenFunction &CGF, Address
Addr1,
return Address(PHI, Addr1.getElementType(), Align);
}
-bool CodeGen::isEmptyField(ASTContext &Context, const FieldDecl *FD,
+bool CodeGen::isEmptyField(const ASTContext &Context, const FieldDecl *FD,
bool AllowArrays, bool AsIfNoUniqueAddr) {
if (FD->isUnnamedBitField())
return true;
@@ -289,8 +289,8 @@ bool CodeGen::isEmptyField(ASTContext &Context, const
FieldDecl *FD,
return isEmptyRecord(Context, FT, AllowArrays, AsIfNoUniqueAddr);
}
-bool CodeGen::isEmptyRecord(ASTContext &Context, QualType T, bool AllowArrays,
-bool AsIfNoUniqueAddr) {
+bool CodeGen::isEmptyRecord(const ASTContext &Context, QualType T,
+bool AllowArrays, bool AsIfNoUniqueAddr) {
const RecordType *RT = T->getAs();
if (!RT)
return false;
diff --git a/clang/lib/CodeGen/ABIInfoImpl.h b/clang/lib/CodeGen/ABIInfoImpl.h
index 92986fb431646..e3f373e39c35a 100644
--- a/clang/lib/CodeGen/ABIInfoImpl.h
+++ b/clang/lib/CodeGen/ABIInfoImpl.h
@@ -126,15 +126,15 @@ Address emitMergePHI(CodeGenFunction &CGF, Address Addr1,
/// is an unnamed bit-field or an (array of) empty record(s). If
/// AsIfNoUniqueAddr is true, then C++ record fields are considered empty if
/// the [[no_unique_address]] attribute would have made them empty.
-bool isEmptyField(ASTContext &Context, const FieldDecl *FD, bool AllowArrays,
- bool AsIfNoUniqueAddr = false);
+bool isEmptyField(const ASTContext &Context, const FieldDecl *FD,
+ bool AllowArrays, bool AsIfNoUniqueAddr = false);
/// isEmptyRecord - Return true iff a structure contains only empty
/// fields. Note that a structure with a flexible array member is not
/// considered empty. If AsIfNoUniqueAddr is true, then C++
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
https://github.com/Michael137 updated
https://github.com/llvm/llvm-project/pull/96422
>From 07e603f7afc98e5af31962a5b1f44f4e4c079ebb Mon Sep 17 00:00:00 2001
From: Michael Buch
Date: Fri, 21 Jun 2024 12:15:07 +0100
Subject: [PATCH 01/13] [clang][CGRecordLayout] Remove dependency on isZeroSize
This is a follow-up from the conversation starting at
https://github.com/llvm/llvm-project/pull/93809#issuecomment-2173729801
The root problem that motivated the change are external AST
sources that compute `ASTRecordLayout`s themselves instead of
letting Clang compute them from the AST. One such examples is
LLDB using DWARF to get the definitive offsets and sizes of C++
structures. Such layouts should be considered correct (modulo
buggy DWARF), but various assertions and lowering logic around
the `CGRecordLayoutBuilder` relies on the AST having
`[[no_unique_address]]` attached to them. This is a layout-altering
attribute which is not encoded in DWARF. This causes us LLDB to trip
over the various LLVM<->Clang layout consistency checks. There has been
precedent for avoiding such layout-altering attributes to affect
lowering with externally-provided layouts (e.g., packed structs).
This patch proposes to replace the `isZeroSize` checks in
`CGRecordLayoutBuilder` (which roughly means "empty field
with [[no_unique_address]]") with checks for
`CodeGen::isEmptyField`/`CodeGen::isEmptyRecord`.
---
clang/lib/CodeGen/ABIInfoImpl.cpp | 6 ++---
clang/lib/CodeGen/ABIInfoImpl.h | 6 ++---
clang/lib/CodeGen/CGExpr.cpp | 3 ++-
clang/lib/CodeGen/CGRecordLayoutBuilder.cpp | 14 ++--
clang/test/CodeGen/X86/x86_64-vaarg.c | 3 ++-
clang/test/CodeGen/debug-info-packed-struct.c | 2 +-
clang/test/CodeGen/paren-list-agg-init.cpp| 15 +
.../CodeGenCXX/2011-12-19-init-list-ctor.cpp | 6 ++---
clang/test/CodeGenCXX/auto-var-init.cpp | 10 -
.../test/CodeGenCXX/bitfield-access-empty.cpp | 16 +++---
clang/test/CodeGenCXX/class-layout.cpp| 2 +-
clang/test/CodeGenCXX/compound-literals.cpp | 2 +-
clang/test/CodeGenCXX/exceptions.cpp | 7 +++---
.../lambda-deterministic-captures.cpp | 4 +---
clang/test/CodeGenCXX/ms_struct.cpp | 4 +---
clang/test/CodeGenCXX/partial-destruction.cpp | 11 --
clang/test/CodeGenCXX/pr18962.cpp | 5 ++---
clang/test/CodeGenCXX/references.cpp | 4 +---
clang/test/CodeGenCXX/temporaries.cpp | 12 +-
clang/test/CodeGenObjCXX/lambda-to-block.mm | 9
clang/test/OpenMP/irbuilder_for_iterator.cpp | 10 -
clang/test/OpenMP/irbuilder_for_rangefor.cpp | 14 +---
.../test/OpenMP/task_member_call_codegen.cpp | 22 ---
clang/test/Sema/ms_class_layout.cpp | 14 ++--
24 files changed, 85 insertions(+), 116 deletions(-)
diff --git a/clang/lib/CodeGen/ABIInfoImpl.cpp
b/clang/lib/CodeGen/ABIInfoImpl.cpp
index e9a26abb77837..92fcc3bc2c5f4 100644
--- a/clang/lib/CodeGen/ABIInfoImpl.cpp
+++ b/clang/lib/CodeGen/ABIInfoImpl.cpp
@@ -248,7 +248,7 @@ Address CodeGen::emitMergePHI(CodeGenFunction &CGF, Address
Addr1,
return Address(PHI, Addr1.getElementType(), Align);
}
-bool CodeGen::isEmptyField(ASTContext &Context, const FieldDecl *FD,
+bool CodeGen::isEmptyField(const ASTContext &Context, const FieldDecl *FD,
bool AllowArrays, bool AsIfNoUniqueAddr) {
if (FD->isUnnamedBitField())
return true;
@@ -289,8 +289,8 @@ bool CodeGen::isEmptyField(ASTContext &Context, const
FieldDecl *FD,
return isEmptyRecord(Context, FT, AllowArrays, AsIfNoUniqueAddr);
}
-bool CodeGen::isEmptyRecord(ASTContext &Context, QualType T, bool AllowArrays,
-bool AsIfNoUniqueAddr) {
+bool CodeGen::isEmptyRecord(const ASTContext &Context, QualType T,
+bool AllowArrays, bool AsIfNoUniqueAddr) {
const RecordType *RT = T->getAs();
if (!RT)
return false;
diff --git a/clang/lib/CodeGen/ABIInfoImpl.h b/clang/lib/CodeGen/ABIInfoImpl.h
index 92986fb431646..e3f373e39c35a 100644
--- a/clang/lib/CodeGen/ABIInfoImpl.h
+++ b/clang/lib/CodeGen/ABIInfoImpl.h
@@ -126,15 +126,15 @@ Address emitMergePHI(CodeGenFunction &CGF, Address Addr1,
/// is an unnamed bit-field or an (array of) empty record(s). If
/// AsIfNoUniqueAddr is true, then C++ record fields are considered empty if
/// the [[no_unique_address]] attribute would have made them empty.
-bool isEmptyField(ASTContext &Context, const FieldDecl *FD, bool AllowArrays,
- bool AsIfNoUniqueAddr = false);
+bool isEmptyField(const ASTContext &Context, const FieldDecl *FD,
+ bool AllowArrays, bool AsIfNoUniqueAddr = false);
/// isEmptyRecord - Return true iff a structure contains only empty
/// fields. Note that a structure with a flexible array member is not
/// considered empty. If AsIfNoUniqueAddr is true, then C++
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
@@ -1,7 +1,19 @@
-// RUN: %clang_cc1 -emit-llvm < %s | grep "zeroinitializer, i16 16877"
+// RUN: %clang_cc1 %s -emit-llvm -triple x86_64-linux-gnu -o - | FileCheck %s
--check-prefixes=CHECK,EMPTY
+// RUN: %clang_cc1 %s -emit-llvm -triple x86_64-windows-msvc -o - | FileCheck
%s --check-prefixes=CHECK,EMPTY-MSVC
// PR4390
struct sysfs_dirent {
- union { struct sysfs_elem_dir {} s_dir; };
+ union { struct sysfs_elem_dir { int x; } s_dir; };
unsigned short s_mode;
};
struct sysfs_dirent sysfs_root = { {}, 16877 };
+
+// CHECK: @sysfs_root = {{.*}}global %struct.sysfs_dirent { %union.anon
zeroinitializer, i16 16877 }
+
+struct Foo {
+ union { struct empty {} x; };
+ unsigned short s_mode;
+};
+struct Foo foo = { {}, 16877 };
+
+// EMPTY: @foo = {{.*}}global %struct.Foo { i16 16877 }
+// EMPTY-MSVC: @foo = {{.*}}global %struct.Foo { [4 x i8] undef, i16 16877 }
Michael137 wrote:
@efriedma-quic For MSVC targets we don't emit a zeroinitializer here now
despite the empty union occupying some storage. Do we expect there to be a
zeroinitializer here?
https://github.com/llvm/llvm-project/pull/96422
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
https://github.com/Michael137 edited https://github.com/llvm/llvm-project/pull/96422 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
https://github.com/Michael137 edited https://github.com/llvm/llvm-project/pull/96422 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
https://github.com/efriedma-quic approved this pull request. LGTM https://github.com/llvm/llvm-project/pull/96422 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
https://github.com/efriedma-quic edited https://github.com/llvm/llvm-project/pull/96422 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
@@ -1,7 +1,19 @@
-// RUN: %clang_cc1 -emit-llvm < %s | grep "zeroinitializer, i16 16877"
+// RUN: %clang_cc1 %s -emit-llvm -triple x86_64-linux-gnu -o - | FileCheck %s
--check-prefixes=CHECK,EMPTY
+// RUN: %clang_cc1 %s -emit-llvm -triple x86_64-windows-msvc -o - | FileCheck
%s --check-prefixes=CHECK,EMPTY-MSVC
// PR4390
struct sysfs_dirent {
- union { struct sysfs_elem_dir {} s_dir; };
+ union { struct sysfs_elem_dir { int x; } s_dir; };
unsigned short s_mode;
};
struct sysfs_dirent sysfs_root = { {}, 16877 };
+
+// CHECK: @sysfs_root = {{.*}}global %struct.sysfs_dirent { %union.anon
zeroinitializer, i16 16877 }
+
+struct Foo {
+ union { struct empty {} x; };
+ unsigned short s_mode;
+};
+struct Foo foo = { {}, 16877 };
+
+// EMPTY: @foo = {{.*}}global %struct.Foo { i16 16877 }
+// EMPTY-MSVC: @foo = {{.*}}global %struct.Foo { [4 x i8] undef, i16 16877 }
efriedma-quic wrote:
clang doesn't currently implement the C23 rules for empty-brace
initialization... but I guess if you extend those rules to empty structs/unions
(which aren't valid C, but ignoring that), we arguably should zero-init? But
really, nobody should notice; this weird stuff is only realistically used for
stuff like FAMs.
Testcase which MSVC accepts:
```
struct Foo {
unsigned short s_mode;
struct empty {int a[0];} x;
};
struct Foo f() { return (struct Foo){16877}; };
```
See #78034/#97121.
https://github.com/llvm/llvm-project/pull/96422
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
Michael137 wrote:
Some of the libc++ tests seem to be crashing on the x86_64 bot (can repro this
on my x86_64 at home, not aarch64 though). Looks like they're segfaulting
trying to access the member of an empty class. E.g., in
```
Process 1370440 stopped
* thread #1, name = 't.tmp.exe', stop reason = signal SIGSEGV: invalid address
(fault address: 0x20)
frame #0: 0x60c3
t.tmp.exe`std::__1::operator==[abi:se19](__x=0x7fffdd10,
__y=0x7fffdcd0) at error_code.h:97:25
94 }
95
96 inline _LIBCPP_HIDE_FROM_ABI bool operator==(const error_code& __x,
const error_condition& __y) _NOEXCEPT {
-> 97 return __x.category().equivalent(__x.value(), __y) ||
__y.category().equivalent(__x, __y.value());
98 }
99
100 #if _LIBCPP_STD_VER <= 17
(lldb) dis -pc
t.tmp.exe`std::__1::operator==[abi:se19](std::__1::error_code const&,
std::__1::error_condition const&):
-> 0x60c3 <+51>: callq *0x20(%rax)
0x60c6 <+54>: movb %al, %cl
0x60c8 <+56>: movb $0x1, %al
0x60ca <+58>: testb $0x1, %cl
(lldb)
```
https://github.com/llvm/llvm-project/pull/96422
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
Michael137 wrote:
> Some of the libc++ tests seem to be crashing on the x86_64 bot (can repro
> locally, on aarch64 too). Looks like they're segfaulting trying to access the
> member of an empty class. E.g., in
>
> ```
> Process 1370440 stopped
> * thread #1, name = 't.tmp.exe', stop reason = signal SIGSEGV: invalid
> address (fault address: 0x20)
> frame #0: 0x60c3
> t.tmp.exe`std::__1::operator==[abi:se19](__x=0x7fffdd10,
> __y=0x7fffdcd0) at error_code.h:97:25
>94 }
>95
>96 inline _LIBCPP_HIDE_FROM_ABI bool operator==(const error_code& __x,
> const error_condition& __y) _NOEXCEPT {
> -> 97 return __x.category().equivalent(__x.value(), __y) ||
> __y.category().equivalent(__x, __y.value());
>98 }
>99
>100 #if _LIBCPP_STD_VER <= 17
> (lldb) dis -pc
> t.tmp.exe`std::__1::operator==[abi:se19](std::__1::error_code const&,
> std::__1::error_condition const&):
> -> 0x60c3 <+51>: callq *0x20(%rax)
> 0x60c6 <+54>: movb %al, %cl
> 0x60c8 <+56>: movb $0x1, %al
> 0x60ca <+58>: testb $0x1, %cl
> (lldb)
> ```
>
> Investigating..
Reduced it to:
```
// We only crash if bar::func is virtual and Holder has a constexpr constructor
struct bar {
virtual void func() const {}
};
struct Holder {
bar m_bar;
constexpr explicit Holder() : m_bar() {}
~Holder() {}
};
const bar &foo() noexcept {
static Holder holder;
return holder.m_bar;
}
int main() {
foo().func();
return 0;
}
```
What was happening is that `Holder` was being lowered to:
```
internal global %struct.Holder undef, align 8
```
instead of:
```
internal global %struct.Holder { %struct.bar { ptr getelementptr inbounds
inrange(-16, 8) ({ [3 x ptr] }, ptr @_ZTV3bar, i32 0, i32 0, i32 2) } }, align 8
```
This was happening because `isZeroSize` considers types with virtual
bases/functions to not be zero-size (whereas the `isEmptyFieldForLayout` didn't
account for that. Fixed this in the latest commit by checking whether the
`CXXRecordDecl` is polymorphic
https://github.com/llvm/llvm-project/pull/96422
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
https://github.com/Michael137 updated
https://github.com/llvm/llvm-project/pull/96422
>From 07e603f7afc98e5af31962a5b1f44f4e4c079ebb Mon Sep 17 00:00:00 2001
From: Michael Buch
Date: Fri, 21 Jun 2024 12:15:07 +0100
Subject: [PATCH 01/15] [clang][CGRecordLayout] Remove dependency on isZeroSize
This is a follow-up from the conversation starting at
https://github.com/llvm/llvm-project/pull/93809#issuecomment-2173729801
The root problem that motivated the change are external AST
sources that compute `ASTRecordLayout`s themselves instead of
letting Clang compute them from the AST. One such examples is
LLDB using DWARF to get the definitive offsets and sizes of C++
structures. Such layouts should be considered correct (modulo
buggy DWARF), but various assertions and lowering logic around
the `CGRecordLayoutBuilder` relies on the AST having
`[[no_unique_address]]` attached to them. This is a layout-altering
attribute which is not encoded in DWARF. This causes us LLDB to trip
over the various LLVM<->Clang layout consistency checks. There has been
precedent for avoiding such layout-altering attributes to affect
lowering with externally-provided layouts (e.g., packed structs).
This patch proposes to replace the `isZeroSize` checks in
`CGRecordLayoutBuilder` (which roughly means "empty field
with [[no_unique_address]]") with checks for
`CodeGen::isEmptyField`/`CodeGen::isEmptyRecord`.
---
clang/lib/CodeGen/ABIInfoImpl.cpp | 6 ++---
clang/lib/CodeGen/ABIInfoImpl.h | 6 ++---
clang/lib/CodeGen/CGExpr.cpp | 3 ++-
clang/lib/CodeGen/CGRecordLayoutBuilder.cpp | 14 ++--
clang/test/CodeGen/X86/x86_64-vaarg.c | 3 ++-
clang/test/CodeGen/debug-info-packed-struct.c | 2 +-
clang/test/CodeGen/paren-list-agg-init.cpp| 15 +
.../CodeGenCXX/2011-12-19-init-list-ctor.cpp | 6 ++---
clang/test/CodeGenCXX/auto-var-init.cpp | 10 -
.../test/CodeGenCXX/bitfield-access-empty.cpp | 16 +++---
clang/test/CodeGenCXX/class-layout.cpp| 2 +-
clang/test/CodeGenCXX/compound-literals.cpp | 2 +-
clang/test/CodeGenCXX/exceptions.cpp | 7 +++---
.../lambda-deterministic-captures.cpp | 4 +---
clang/test/CodeGenCXX/ms_struct.cpp | 4 +---
clang/test/CodeGenCXX/partial-destruction.cpp | 11 --
clang/test/CodeGenCXX/pr18962.cpp | 5 ++---
clang/test/CodeGenCXX/references.cpp | 4 +---
clang/test/CodeGenCXX/temporaries.cpp | 12 +-
clang/test/CodeGenObjCXX/lambda-to-block.mm | 9
clang/test/OpenMP/irbuilder_for_iterator.cpp | 10 -
clang/test/OpenMP/irbuilder_for_rangefor.cpp | 14 +---
.../test/OpenMP/task_member_call_codegen.cpp | 22 ---
clang/test/Sema/ms_class_layout.cpp | 14 ++--
24 files changed, 85 insertions(+), 116 deletions(-)
diff --git a/clang/lib/CodeGen/ABIInfoImpl.cpp
b/clang/lib/CodeGen/ABIInfoImpl.cpp
index e9a26abb77837..92fcc3bc2c5f4 100644
--- a/clang/lib/CodeGen/ABIInfoImpl.cpp
+++ b/clang/lib/CodeGen/ABIInfoImpl.cpp
@@ -248,7 +248,7 @@ Address CodeGen::emitMergePHI(CodeGenFunction &CGF, Address
Addr1,
return Address(PHI, Addr1.getElementType(), Align);
}
-bool CodeGen::isEmptyField(ASTContext &Context, const FieldDecl *FD,
+bool CodeGen::isEmptyField(const ASTContext &Context, const FieldDecl *FD,
bool AllowArrays, bool AsIfNoUniqueAddr) {
if (FD->isUnnamedBitField())
return true;
@@ -289,8 +289,8 @@ bool CodeGen::isEmptyField(ASTContext &Context, const
FieldDecl *FD,
return isEmptyRecord(Context, FT, AllowArrays, AsIfNoUniqueAddr);
}
-bool CodeGen::isEmptyRecord(ASTContext &Context, QualType T, bool AllowArrays,
-bool AsIfNoUniqueAddr) {
+bool CodeGen::isEmptyRecord(const ASTContext &Context, QualType T,
+bool AllowArrays, bool AsIfNoUniqueAddr) {
const RecordType *RT = T->getAs();
if (!RT)
return false;
diff --git a/clang/lib/CodeGen/ABIInfoImpl.h b/clang/lib/CodeGen/ABIInfoImpl.h
index 92986fb431646..e3f373e39c35a 100644
--- a/clang/lib/CodeGen/ABIInfoImpl.h
+++ b/clang/lib/CodeGen/ABIInfoImpl.h
@@ -126,15 +126,15 @@ Address emitMergePHI(CodeGenFunction &CGF, Address Addr1,
/// is an unnamed bit-field or an (array of) empty record(s). If
/// AsIfNoUniqueAddr is true, then C++ record fields are considered empty if
/// the [[no_unique_address]] attribute would have made them empty.
-bool isEmptyField(ASTContext &Context, const FieldDecl *FD, bool AllowArrays,
- bool AsIfNoUniqueAddr = false);
+bool isEmptyField(const ASTContext &Context, const FieldDecl *FD,
+ bool AllowArrays, bool AsIfNoUniqueAddr = false);
/// isEmptyRecord - Return true iff a structure contains only empty
/// fields. Note that a structure with a flexible array member is not
/// considered empty. If AsIfNoUniqueAddr is true, then C++
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
@@ -310,6 +310,41 @@ bool CodeGen::isEmptyRecord(ASTContext &Context, QualType
T, bool AllowArrays,
return true;
}
+bool CodeGen::isEmptyFieldForLayout(const ASTContext &Context,
+const FieldDecl *FD) {
+ if (FD->isZeroLengthBitField(Context))
+return true;
+
+ if (FD->isUnnamedBitField())
+return false;
+
+ return isEmptyRecordForLayout(Context, FD->getType());
+}
+
+bool CodeGen::isEmptyRecordForLayout(const ASTContext &Context, QualType T) {
+ const RecordType *RT = T->getAs();
+ if (!RT)
+return false;
+
+ const RecordDecl *RD = RT->getDecl();
+
+ // If this is a C++ record, check the bases first.
+ if (const CXXRecordDecl *CXXRD = dyn_cast(RD)) {
+if (CXXRD->isPolymorphic())
efriedma-quic wrote:
`isDynamicClass()`, to also check for virtual base classes
https://github.com/llvm/llvm-project/pull/96422
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
https://github.com/Michael137 updated
https://github.com/llvm/llvm-project/pull/96422
>From 07e603f7afc98e5af31962a5b1f44f4e4c079ebb Mon Sep 17 00:00:00 2001
From: Michael Buch
Date: Fri, 21 Jun 2024 12:15:07 +0100
Subject: [PATCH 01/16] [clang][CGRecordLayout] Remove dependency on isZeroSize
This is a follow-up from the conversation starting at
https://github.com/llvm/llvm-project/pull/93809#issuecomment-2173729801
The root problem that motivated the change are external AST
sources that compute `ASTRecordLayout`s themselves instead of
letting Clang compute them from the AST. One such examples is
LLDB using DWARF to get the definitive offsets and sizes of C++
structures. Such layouts should be considered correct (modulo
buggy DWARF), but various assertions and lowering logic around
the `CGRecordLayoutBuilder` relies on the AST having
`[[no_unique_address]]` attached to them. This is a layout-altering
attribute which is not encoded in DWARF. This causes us LLDB to trip
over the various LLVM<->Clang layout consistency checks. There has been
precedent for avoiding such layout-altering attributes to affect
lowering with externally-provided layouts (e.g., packed structs).
This patch proposes to replace the `isZeroSize` checks in
`CGRecordLayoutBuilder` (which roughly means "empty field
with [[no_unique_address]]") with checks for
`CodeGen::isEmptyField`/`CodeGen::isEmptyRecord`.
---
clang/lib/CodeGen/ABIInfoImpl.cpp | 6 ++---
clang/lib/CodeGen/ABIInfoImpl.h | 6 ++---
clang/lib/CodeGen/CGExpr.cpp | 3 ++-
clang/lib/CodeGen/CGRecordLayoutBuilder.cpp | 14 ++--
clang/test/CodeGen/X86/x86_64-vaarg.c | 3 ++-
clang/test/CodeGen/debug-info-packed-struct.c | 2 +-
clang/test/CodeGen/paren-list-agg-init.cpp| 15 +
.../CodeGenCXX/2011-12-19-init-list-ctor.cpp | 6 ++---
clang/test/CodeGenCXX/auto-var-init.cpp | 10 -
.../test/CodeGenCXX/bitfield-access-empty.cpp | 16 +++---
clang/test/CodeGenCXX/class-layout.cpp| 2 +-
clang/test/CodeGenCXX/compound-literals.cpp | 2 +-
clang/test/CodeGenCXX/exceptions.cpp | 7 +++---
.../lambda-deterministic-captures.cpp | 4 +---
clang/test/CodeGenCXX/ms_struct.cpp | 4 +---
clang/test/CodeGenCXX/partial-destruction.cpp | 11 --
clang/test/CodeGenCXX/pr18962.cpp | 5 ++---
clang/test/CodeGenCXX/references.cpp | 4 +---
clang/test/CodeGenCXX/temporaries.cpp | 12 +-
clang/test/CodeGenObjCXX/lambda-to-block.mm | 9
clang/test/OpenMP/irbuilder_for_iterator.cpp | 10 -
clang/test/OpenMP/irbuilder_for_rangefor.cpp | 14 +---
.../test/OpenMP/task_member_call_codegen.cpp | 22 ---
clang/test/Sema/ms_class_layout.cpp | 14 ++--
24 files changed, 85 insertions(+), 116 deletions(-)
diff --git a/clang/lib/CodeGen/ABIInfoImpl.cpp
b/clang/lib/CodeGen/ABIInfoImpl.cpp
index e9a26abb77837..92fcc3bc2c5f4 100644
--- a/clang/lib/CodeGen/ABIInfoImpl.cpp
+++ b/clang/lib/CodeGen/ABIInfoImpl.cpp
@@ -248,7 +248,7 @@ Address CodeGen::emitMergePHI(CodeGenFunction &CGF, Address
Addr1,
return Address(PHI, Addr1.getElementType(), Align);
}
-bool CodeGen::isEmptyField(ASTContext &Context, const FieldDecl *FD,
+bool CodeGen::isEmptyField(const ASTContext &Context, const FieldDecl *FD,
bool AllowArrays, bool AsIfNoUniqueAddr) {
if (FD->isUnnamedBitField())
return true;
@@ -289,8 +289,8 @@ bool CodeGen::isEmptyField(ASTContext &Context, const
FieldDecl *FD,
return isEmptyRecord(Context, FT, AllowArrays, AsIfNoUniqueAddr);
}
-bool CodeGen::isEmptyRecord(ASTContext &Context, QualType T, bool AllowArrays,
-bool AsIfNoUniqueAddr) {
+bool CodeGen::isEmptyRecord(const ASTContext &Context, QualType T,
+bool AllowArrays, bool AsIfNoUniqueAddr) {
const RecordType *RT = T->getAs();
if (!RT)
return false;
diff --git a/clang/lib/CodeGen/ABIInfoImpl.h b/clang/lib/CodeGen/ABIInfoImpl.h
index 92986fb431646..e3f373e39c35a 100644
--- a/clang/lib/CodeGen/ABIInfoImpl.h
+++ b/clang/lib/CodeGen/ABIInfoImpl.h
@@ -126,15 +126,15 @@ Address emitMergePHI(CodeGenFunction &CGF, Address Addr1,
/// is an unnamed bit-field or an (array of) empty record(s). If
/// AsIfNoUniqueAddr is true, then C++ record fields are considered empty if
/// the [[no_unique_address]] attribute would have made them empty.
-bool isEmptyField(ASTContext &Context, const FieldDecl *FD, bool AllowArrays,
- bool AsIfNoUniqueAddr = false);
+bool isEmptyField(const ASTContext &Context, const FieldDecl *FD,
+ bool AllowArrays, bool AsIfNoUniqueAddr = false);
/// isEmptyRecord - Return true iff a structure contains only empty
/// fields. Note that a structure with a flexible array member is not
/// considered empty. If AsIfNoUniqueAddr is true, then C++
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
https://github.com/efriedma-quic approved this pull request. https://github.com/llvm/llvm-project/pull/96422 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
https://github.com/Michael137 updated
https://github.com/llvm/llvm-project/pull/96422
>From 07e603f7afc98e5af31962a5b1f44f4e4c079ebb Mon Sep 17 00:00:00 2001
From: Michael Buch
Date: Fri, 21 Jun 2024 12:15:07 +0100
Subject: [PATCH 01/19] [clang][CGRecordLayout] Remove dependency on isZeroSize
This is a follow-up from the conversation starting at
https://github.com/llvm/llvm-project/pull/93809#issuecomment-2173729801
The root problem that motivated the change are external AST
sources that compute `ASTRecordLayout`s themselves instead of
letting Clang compute them from the AST. One such examples is
LLDB using DWARF to get the definitive offsets and sizes of C++
structures. Such layouts should be considered correct (modulo
buggy DWARF), but various assertions and lowering logic around
the `CGRecordLayoutBuilder` relies on the AST having
`[[no_unique_address]]` attached to them. This is a layout-altering
attribute which is not encoded in DWARF. This causes us LLDB to trip
over the various LLVM<->Clang layout consistency checks. There has been
precedent for avoiding such layout-altering attributes to affect
lowering with externally-provided layouts (e.g., packed structs).
This patch proposes to replace the `isZeroSize` checks in
`CGRecordLayoutBuilder` (which roughly means "empty field
with [[no_unique_address]]") with checks for
`CodeGen::isEmptyField`/`CodeGen::isEmptyRecord`.
---
clang/lib/CodeGen/ABIInfoImpl.cpp | 6 ++---
clang/lib/CodeGen/ABIInfoImpl.h | 6 ++---
clang/lib/CodeGen/CGExpr.cpp | 3 ++-
clang/lib/CodeGen/CGRecordLayoutBuilder.cpp | 14 ++--
clang/test/CodeGen/X86/x86_64-vaarg.c | 3 ++-
clang/test/CodeGen/debug-info-packed-struct.c | 2 +-
clang/test/CodeGen/paren-list-agg-init.cpp| 15 +
.../CodeGenCXX/2011-12-19-init-list-ctor.cpp | 6 ++---
clang/test/CodeGenCXX/auto-var-init.cpp | 10 -
.../test/CodeGenCXX/bitfield-access-empty.cpp | 16 +++---
clang/test/CodeGenCXX/class-layout.cpp| 2 +-
clang/test/CodeGenCXX/compound-literals.cpp | 2 +-
clang/test/CodeGenCXX/exceptions.cpp | 7 +++---
.../lambda-deterministic-captures.cpp | 4 +---
clang/test/CodeGenCXX/ms_struct.cpp | 4 +---
clang/test/CodeGenCXX/partial-destruction.cpp | 11 --
clang/test/CodeGenCXX/pr18962.cpp | 5 ++---
clang/test/CodeGenCXX/references.cpp | 4 +---
clang/test/CodeGenCXX/temporaries.cpp | 12 +-
clang/test/CodeGenObjCXX/lambda-to-block.mm | 9
clang/test/OpenMP/irbuilder_for_iterator.cpp | 10 -
clang/test/OpenMP/irbuilder_for_rangefor.cpp | 14 +---
.../test/OpenMP/task_member_call_codegen.cpp | 22 ---
clang/test/Sema/ms_class_layout.cpp | 14 ++--
24 files changed, 85 insertions(+), 116 deletions(-)
diff --git a/clang/lib/CodeGen/ABIInfoImpl.cpp
b/clang/lib/CodeGen/ABIInfoImpl.cpp
index e9a26abb77837..92fcc3bc2c5f4 100644
--- a/clang/lib/CodeGen/ABIInfoImpl.cpp
+++ b/clang/lib/CodeGen/ABIInfoImpl.cpp
@@ -248,7 +248,7 @@ Address CodeGen::emitMergePHI(CodeGenFunction &CGF, Address
Addr1,
return Address(PHI, Addr1.getElementType(), Align);
}
-bool CodeGen::isEmptyField(ASTContext &Context, const FieldDecl *FD,
+bool CodeGen::isEmptyField(const ASTContext &Context, const FieldDecl *FD,
bool AllowArrays, bool AsIfNoUniqueAddr) {
if (FD->isUnnamedBitField())
return true;
@@ -289,8 +289,8 @@ bool CodeGen::isEmptyField(ASTContext &Context, const
FieldDecl *FD,
return isEmptyRecord(Context, FT, AllowArrays, AsIfNoUniqueAddr);
}
-bool CodeGen::isEmptyRecord(ASTContext &Context, QualType T, bool AllowArrays,
-bool AsIfNoUniqueAddr) {
+bool CodeGen::isEmptyRecord(const ASTContext &Context, QualType T,
+bool AllowArrays, bool AsIfNoUniqueAddr) {
const RecordType *RT = T->getAs();
if (!RT)
return false;
diff --git a/clang/lib/CodeGen/ABIInfoImpl.h b/clang/lib/CodeGen/ABIInfoImpl.h
index 92986fb431646..e3f373e39c35a 100644
--- a/clang/lib/CodeGen/ABIInfoImpl.h
+++ b/clang/lib/CodeGen/ABIInfoImpl.h
@@ -126,15 +126,15 @@ Address emitMergePHI(CodeGenFunction &CGF, Address Addr1,
/// is an unnamed bit-field or an (array of) empty record(s). If
/// AsIfNoUniqueAddr is true, then C++ record fields are considered empty if
/// the [[no_unique_address]] attribute would have made them empty.
-bool isEmptyField(ASTContext &Context, const FieldDecl *FD, bool AllowArrays,
- bool AsIfNoUniqueAddr = false);
+bool isEmptyField(const ASTContext &Context, const FieldDecl *FD,
+ bool AllowArrays, bool AsIfNoUniqueAddr = false);
/// isEmptyRecord - Return true iff a structure contains only empty
/// fields. Note that a structure with a flexible array member is not
/// considered empty. If AsIfNoUniqueAddr is true, then C++
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
https://github.com/Michael137 created
https://github.com/llvm/llvm-project/pull/96422
This is a follow-up from the conversation starting at
https://github.com/llvm/llvm-project/pull/93809#issuecomment-2173729801
The root problem that motivated the change are external AST sources that
compute `ASTRecordLayout`s themselves instead of letting Clang compute them
from the AST. One such example is LLDB using DWARF to get the definitive
offsets and sizes of C++ structures. Such layouts should be considered correct
(modulo buggy DWARF), but various assertions and lowering logic around the
`CGRecordLayoutBuilder` relies on the AST having `[[no_unique_address]]`
attached to them. This is a layout-altering attribute which is not encoded in
DWARF. This causes us LLDB to trip over the various LLVM<->Clang layout
consistency checks. There has been precedent for avoiding such layout-altering
attributes from affecting lowering with externally-provided layouts (e.g.,
packed structs).
This patch proposes to replace the `isZeroSize` checks in
`CGRecordLayoutBuilder` (which roughly means "empty field with
[[no_unique_address]]") with checks for
`CodeGen::isEmptyField`/`CodeGen::isEmptyRecord`.
>From f5938919b3a0060db6b373bead1c52f4bb65c841 Mon Sep 17 00:00:00 2001
From: Michael Buch
Date: Fri, 21 Jun 2024 12:15:07 +0100
Subject: [PATCH] [clang][CGRecordLayout] Remove dependency on isZeroSize
This is a follow-up from the conversation starting at
https://github.com/llvm/llvm-project/pull/93809#issuecomment-2173729801
The root problem that motivated the change are external AST
sources that compute `ASTRecordLayout`s themselves instead of
letting Clang compute them from the AST. One such examples is
LLDB using DWARF to get the definitive offsets and sizes of C++
structures. Such layouts should be considered correct (modulo
buggy DWARF), but various assertions and lowering logic around
the `CGRecordLayoutBuilder` relies on the AST having
`[[no_unique_address]]` attached to them. This is a layout-altering
attribute which is not encoded in DWARF. This causes us LLDB to trip
over the various LLVM<->Clang layout consistency checks. There has been
precedent for avoiding such layout-altering attributes to affect
lowering with externally-provided layouts (e.g., packed structs).
This patch proposes to replace the `isZeroSize` checks in
`CGRecordLayoutBuilder` (which roughly means "empty field
with [[no_unique_address]]") with checks for
`CodeGen::isEmptyField`/`CodeGen::isEmptyRecord`.
---
clang/lib/CodeGen/ABIInfoImpl.cpp | 6 ++---
clang/lib/CodeGen/ABIInfoImpl.h | 6 ++---
clang/lib/CodeGen/CGExpr.cpp | 3 ++-
clang/lib/CodeGen/CGRecordLayoutBuilder.cpp | 14 ++--
clang/test/CodeGen/X86/x86_64-vaarg.c | 3 ++-
clang/test/CodeGen/debug-info-packed-struct.c | 2 +-
clang/test/CodeGen/paren-list-agg-init.cpp| 15 +
.../CodeGenCXX/2011-12-19-init-list-ctor.cpp | 6 ++---
clang/test/CodeGenCXX/auto-var-init.cpp | 10 -
.../test/CodeGenCXX/bitfield-access-empty.cpp | 16 +++---
clang/test/CodeGenCXX/class-layout.cpp| 2 +-
clang/test/CodeGenCXX/compound-literals.cpp | 2 +-
clang/test/CodeGenCXX/exceptions.cpp | 7 +++---
.../lambda-deterministic-captures.cpp | 4 +---
clang/test/CodeGenCXX/ms_struct.cpp | 4 +---
clang/test/CodeGenCXX/partial-destruction.cpp | 11 --
clang/test/CodeGenCXX/pr18962.cpp | 5 ++---
clang/test/CodeGenCXX/references.cpp | 4 +---
clang/test/CodeGenCXX/temporaries.cpp | 12 +-
clang/test/CodeGenObjCXX/lambda-to-block.mm | 9
clang/test/OpenMP/irbuilder_for_iterator.cpp | 10 -
clang/test/OpenMP/irbuilder_for_rangefor.cpp | 14 +---
.../test/OpenMP/task_member_call_codegen.cpp | 22 ---
clang/test/Sema/ms_class_layout.cpp | 14 ++--
24 files changed, 85 insertions(+), 116 deletions(-)
diff --git a/clang/lib/CodeGen/ABIInfoImpl.cpp
b/clang/lib/CodeGen/ABIInfoImpl.cpp
index e9a26abb77837..92fcc3bc2c5f4 100644
--- a/clang/lib/CodeGen/ABIInfoImpl.cpp
+++ b/clang/lib/CodeGen/ABIInfoImpl.cpp
@@ -248,7 +248,7 @@ Address CodeGen::emitMergePHI(CodeGenFunction &CGF, Address
Addr1,
return Address(PHI, Addr1.getElementType(), Align);
}
-bool CodeGen::isEmptyField(ASTContext &Context, const FieldDecl *FD,
+bool CodeGen::isEmptyField(const ASTContext &Context, const FieldDecl *FD,
bool AllowArrays, bool AsIfNoUniqueAddr) {
if (FD->isUnnamedBitField())
return true;
@@ -289,8 +289,8 @@ bool CodeGen::isEmptyField(ASTContext &Context, const
FieldDecl *FD,
return isEmptyRecord(Context, FT, AllowArrays, AsIfNoUniqueAddr);
}
-bool CodeGen::isEmptyRecord(ASTContext &Context, QualType T, bool AllowArrays,
-bool AsIfNoUniqueAddr) {
+bool CodeGen::isEmptyRecord(const ASTContext &Co
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
Michael137 wrote: In draft for now because I'm still wrapping my head around some of the IR implications of this. Some of them seem suspect. https://github.com/llvm/llvm-project/pull/96422 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
https://github.com/Michael137 updated
https://github.com/llvm/llvm-project/pull/96422
>From f5938919b3a0060db6b373bead1c52f4bb65c841 Mon Sep 17 00:00:00 2001
From: Michael Buch
Date: Fri, 21 Jun 2024 12:15:07 +0100
Subject: [PATCH 1/2] [clang][CGRecordLayout] Remove dependency on isZeroSize
This is a follow-up from the conversation starting at
https://github.com/llvm/llvm-project/pull/93809#issuecomment-2173729801
The root problem that motivated the change are external AST
sources that compute `ASTRecordLayout`s themselves instead of
letting Clang compute them from the AST. One such examples is
LLDB using DWARF to get the definitive offsets and sizes of C++
structures. Such layouts should be considered correct (modulo
buggy DWARF), but various assertions and lowering logic around
the `CGRecordLayoutBuilder` relies on the AST having
`[[no_unique_address]]` attached to them. This is a layout-altering
attribute which is not encoded in DWARF. This causes us LLDB to trip
over the various LLVM<->Clang layout consistency checks. There has been
precedent for avoiding such layout-altering attributes to affect
lowering with externally-provided layouts (e.g., packed structs).
This patch proposes to replace the `isZeroSize` checks in
`CGRecordLayoutBuilder` (which roughly means "empty field
with [[no_unique_address]]") with checks for
`CodeGen::isEmptyField`/`CodeGen::isEmptyRecord`.
---
clang/lib/CodeGen/ABIInfoImpl.cpp | 6 ++---
clang/lib/CodeGen/ABIInfoImpl.h | 6 ++---
clang/lib/CodeGen/CGExpr.cpp | 3 ++-
clang/lib/CodeGen/CGRecordLayoutBuilder.cpp | 14 ++--
clang/test/CodeGen/X86/x86_64-vaarg.c | 3 ++-
clang/test/CodeGen/debug-info-packed-struct.c | 2 +-
clang/test/CodeGen/paren-list-agg-init.cpp| 15 +
.../CodeGenCXX/2011-12-19-init-list-ctor.cpp | 6 ++---
clang/test/CodeGenCXX/auto-var-init.cpp | 10 -
.../test/CodeGenCXX/bitfield-access-empty.cpp | 16 +++---
clang/test/CodeGenCXX/class-layout.cpp| 2 +-
clang/test/CodeGenCXX/compound-literals.cpp | 2 +-
clang/test/CodeGenCXX/exceptions.cpp | 7 +++---
.../lambda-deterministic-captures.cpp | 4 +---
clang/test/CodeGenCXX/ms_struct.cpp | 4 +---
clang/test/CodeGenCXX/partial-destruction.cpp | 11 --
clang/test/CodeGenCXX/pr18962.cpp | 5 ++---
clang/test/CodeGenCXX/references.cpp | 4 +---
clang/test/CodeGenCXX/temporaries.cpp | 12 +-
clang/test/CodeGenObjCXX/lambda-to-block.mm | 9
clang/test/OpenMP/irbuilder_for_iterator.cpp | 10 -
clang/test/OpenMP/irbuilder_for_rangefor.cpp | 14 +---
.../test/OpenMP/task_member_call_codegen.cpp | 22 ---
clang/test/Sema/ms_class_layout.cpp | 14 ++--
24 files changed, 85 insertions(+), 116 deletions(-)
diff --git a/clang/lib/CodeGen/ABIInfoImpl.cpp
b/clang/lib/CodeGen/ABIInfoImpl.cpp
index e9a26abb77837..92fcc3bc2c5f4 100644
--- a/clang/lib/CodeGen/ABIInfoImpl.cpp
+++ b/clang/lib/CodeGen/ABIInfoImpl.cpp
@@ -248,7 +248,7 @@ Address CodeGen::emitMergePHI(CodeGenFunction &CGF, Address
Addr1,
return Address(PHI, Addr1.getElementType(), Align);
}
-bool CodeGen::isEmptyField(ASTContext &Context, const FieldDecl *FD,
+bool CodeGen::isEmptyField(const ASTContext &Context, const FieldDecl *FD,
bool AllowArrays, bool AsIfNoUniqueAddr) {
if (FD->isUnnamedBitField())
return true;
@@ -289,8 +289,8 @@ bool CodeGen::isEmptyField(ASTContext &Context, const
FieldDecl *FD,
return isEmptyRecord(Context, FT, AllowArrays, AsIfNoUniqueAddr);
}
-bool CodeGen::isEmptyRecord(ASTContext &Context, QualType T, bool AllowArrays,
-bool AsIfNoUniqueAddr) {
+bool CodeGen::isEmptyRecord(const ASTContext &Context, QualType T,
+bool AllowArrays, bool AsIfNoUniqueAddr) {
const RecordType *RT = T->getAs();
if (!RT)
return false;
diff --git a/clang/lib/CodeGen/ABIInfoImpl.h b/clang/lib/CodeGen/ABIInfoImpl.h
index 92986fb431646..e3f373e39c35a 100644
--- a/clang/lib/CodeGen/ABIInfoImpl.h
+++ b/clang/lib/CodeGen/ABIInfoImpl.h
@@ -126,15 +126,15 @@ Address emitMergePHI(CodeGenFunction &CGF, Address Addr1,
/// is an unnamed bit-field or an (array of) empty record(s). If
/// AsIfNoUniqueAddr is true, then C++ record fields are considered empty if
/// the [[no_unique_address]] attribute would have made them empty.
-bool isEmptyField(ASTContext &Context, const FieldDecl *FD, bool AllowArrays,
- bool AsIfNoUniqueAddr = false);
+bool isEmptyField(const ASTContext &Context, const FieldDecl *FD,
+ bool AllowArrays, bool AsIfNoUniqueAddr = false);
/// isEmptyRecord - Return true iff a structure contains only empty
/// fields. Note that a structure with a flexible array member is not
/// considered empty. If AsIfNoUniqueAddr is true, then C++ r
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
https://github.com/Michael137 updated
https://github.com/llvm/llvm-project/pull/96422
>From f5938919b3a0060db6b373bead1c52f4bb65c841 Mon Sep 17 00:00:00 2001
From: Michael Buch
Date: Fri, 21 Jun 2024 12:15:07 +0100
Subject: [PATCH 1/3] [clang][CGRecordLayout] Remove dependency on isZeroSize
This is a follow-up from the conversation starting at
https://github.com/llvm/llvm-project/pull/93809#issuecomment-2173729801
The root problem that motivated the change are external AST
sources that compute `ASTRecordLayout`s themselves instead of
letting Clang compute them from the AST. One such examples is
LLDB using DWARF to get the definitive offsets and sizes of C++
structures. Such layouts should be considered correct (modulo
buggy DWARF), but various assertions and lowering logic around
the `CGRecordLayoutBuilder` relies on the AST having
`[[no_unique_address]]` attached to them. This is a layout-altering
attribute which is not encoded in DWARF. This causes us LLDB to trip
over the various LLVM<->Clang layout consistency checks. There has been
precedent for avoiding such layout-altering attributes to affect
lowering with externally-provided layouts (e.g., packed structs).
This patch proposes to replace the `isZeroSize` checks in
`CGRecordLayoutBuilder` (which roughly means "empty field
with [[no_unique_address]]") with checks for
`CodeGen::isEmptyField`/`CodeGen::isEmptyRecord`.
---
clang/lib/CodeGen/ABIInfoImpl.cpp | 6 ++---
clang/lib/CodeGen/ABIInfoImpl.h | 6 ++---
clang/lib/CodeGen/CGExpr.cpp | 3 ++-
clang/lib/CodeGen/CGRecordLayoutBuilder.cpp | 14 ++--
clang/test/CodeGen/X86/x86_64-vaarg.c | 3 ++-
clang/test/CodeGen/debug-info-packed-struct.c | 2 +-
clang/test/CodeGen/paren-list-agg-init.cpp| 15 +
.../CodeGenCXX/2011-12-19-init-list-ctor.cpp | 6 ++---
clang/test/CodeGenCXX/auto-var-init.cpp | 10 -
.../test/CodeGenCXX/bitfield-access-empty.cpp | 16 +++---
clang/test/CodeGenCXX/class-layout.cpp| 2 +-
clang/test/CodeGenCXX/compound-literals.cpp | 2 +-
clang/test/CodeGenCXX/exceptions.cpp | 7 +++---
.../lambda-deterministic-captures.cpp | 4 +---
clang/test/CodeGenCXX/ms_struct.cpp | 4 +---
clang/test/CodeGenCXX/partial-destruction.cpp | 11 --
clang/test/CodeGenCXX/pr18962.cpp | 5 ++---
clang/test/CodeGenCXX/references.cpp | 4 +---
clang/test/CodeGenCXX/temporaries.cpp | 12 +-
clang/test/CodeGenObjCXX/lambda-to-block.mm | 9
clang/test/OpenMP/irbuilder_for_iterator.cpp | 10 -
clang/test/OpenMP/irbuilder_for_rangefor.cpp | 14 +---
.../test/OpenMP/task_member_call_codegen.cpp | 22 ---
clang/test/Sema/ms_class_layout.cpp | 14 ++--
24 files changed, 85 insertions(+), 116 deletions(-)
diff --git a/clang/lib/CodeGen/ABIInfoImpl.cpp
b/clang/lib/CodeGen/ABIInfoImpl.cpp
index e9a26abb77837..92fcc3bc2c5f4 100644
--- a/clang/lib/CodeGen/ABIInfoImpl.cpp
+++ b/clang/lib/CodeGen/ABIInfoImpl.cpp
@@ -248,7 +248,7 @@ Address CodeGen::emitMergePHI(CodeGenFunction &CGF, Address
Addr1,
return Address(PHI, Addr1.getElementType(), Align);
}
-bool CodeGen::isEmptyField(ASTContext &Context, const FieldDecl *FD,
+bool CodeGen::isEmptyField(const ASTContext &Context, const FieldDecl *FD,
bool AllowArrays, bool AsIfNoUniqueAddr) {
if (FD->isUnnamedBitField())
return true;
@@ -289,8 +289,8 @@ bool CodeGen::isEmptyField(ASTContext &Context, const
FieldDecl *FD,
return isEmptyRecord(Context, FT, AllowArrays, AsIfNoUniqueAddr);
}
-bool CodeGen::isEmptyRecord(ASTContext &Context, QualType T, bool AllowArrays,
-bool AsIfNoUniqueAddr) {
+bool CodeGen::isEmptyRecord(const ASTContext &Context, QualType T,
+bool AllowArrays, bool AsIfNoUniqueAddr) {
const RecordType *RT = T->getAs();
if (!RT)
return false;
diff --git a/clang/lib/CodeGen/ABIInfoImpl.h b/clang/lib/CodeGen/ABIInfoImpl.h
index 92986fb431646..e3f373e39c35a 100644
--- a/clang/lib/CodeGen/ABIInfoImpl.h
+++ b/clang/lib/CodeGen/ABIInfoImpl.h
@@ -126,15 +126,15 @@ Address emitMergePHI(CodeGenFunction &CGF, Address Addr1,
/// is an unnamed bit-field or an (array of) empty record(s). If
/// AsIfNoUniqueAddr is true, then C++ record fields are considered empty if
/// the [[no_unique_address]] attribute would have made them empty.
-bool isEmptyField(ASTContext &Context, const FieldDecl *FD, bool AllowArrays,
- bool AsIfNoUniqueAddr = false);
+bool isEmptyField(const ASTContext &Context, const FieldDecl *FD,
+ bool AllowArrays, bool AsIfNoUniqueAddr = false);
/// isEmptyRecord - Return true iff a structure contains only empty
/// fields. Note that a structure with a flexible array member is not
/// considered empty. If AsIfNoUniqueAddr is true, then C++ r
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
https://github.com/Michael137 updated
https://github.com/llvm/llvm-project/pull/96422
>From f5938919b3a0060db6b373bead1c52f4bb65c841 Mon Sep 17 00:00:00 2001
From: Michael Buch
Date: Fri, 21 Jun 2024 12:15:07 +0100
Subject: [PATCH 1/4] [clang][CGRecordLayout] Remove dependency on isZeroSize
This is a follow-up from the conversation starting at
https://github.com/llvm/llvm-project/pull/93809#issuecomment-2173729801
The root problem that motivated the change are external AST
sources that compute `ASTRecordLayout`s themselves instead of
letting Clang compute them from the AST. One such examples is
LLDB using DWARF to get the definitive offsets and sizes of C++
structures. Such layouts should be considered correct (modulo
buggy DWARF), but various assertions and lowering logic around
the `CGRecordLayoutBuilder` relies on the AST having
`[[no_unique_address]]` attached to them. This is a layout-altering
attribute which is not encoded in DWARF. This causes us LLDB to trip
over the various LLVM<->Clang layout consistency checks. There has been
precedent for avoiding such layout-altering attributes to affect
lowering with externally-provided layouts (e.g., packed structs).
This patch proposes to replace the `isZeroSize` checks in
`CGRecordLayoutBuilder` (which roughly means "empty field
with [[no_unique_address]]") with checks for
`CodeGen::isEmptyField`/`CodeGen::isEmptyRecord`.
---
clang/lib/CodeGen/ABIInfoImpl.cpp | 6 ++---
clang/lib/CodeGen/ABIInfoImpl.h | 6 ++---
clang/lib/CodeGen/CGExpr.cpp | 3 ++-
clang/lib/CodeGen/CGRecordLayoutBuilder.cpp | 14 ++--
clang/test/CodeGen/X86/x86_64-vaarg.c | 3 ++-
clang/test/CodeGen/debug-info-packed-struct.c | 2 +-
clang/test/CodeGen/paren-list-agg-init.cpp| 15 +
.../CodeGenCXX/2011-12-19-init-list-ctor.cpp | 6 ++---
clang/test/CodeGenCXX/auto-var-init.cpp | 10 -
.../test/CodeGenCXX/bitfield-access-empty.cpp | 16 +++---
clang/test/CodeGenCXX/class-layout.cpp| 2 +-
clang/test/CodeGenCXX/compound-literals.cpp | 2 +-
clang/test/CodeGenCXX/exceptions.cpp | 7 +++---
.../lambda-deterministic-captures.cpp | 4 +---
clang/test/CodeGenCXX/ms_struct.cpp | 4 +---
clang/test/CodeGenCXX/partial-destruction.cpp | 11 --
clang/test/CodeGenCXX/pr18962.cpp | 5 ++---
clang/test/CodeGenCXX/references.cpp | 4 +---
clang/test/CodeGenCXX/temporaries.cpp | 12 +-
clang/test/CodeGenObjCXX/lambda-to-block.mm | 9
clang/test/OpenMP/irbuilder_for_iterator.cpp | 10 -
clang/test/OpenMP/irbuilder_for_rangefor.cpp | 14 +---
.../test/OpenMP/task_member_call_codegen.cpp | 22 ---
clang/test/Sema/ms_class_layout.cpp | 14 ++--
24 files changed, 85 insertions(+), 116 deletions(-)
diff --git a/clang/lib/CodeGen/ABIInfoImpl.cpp
b/clang/lib/CodeGen/ABIInfoImpl.cpp
index e9a26abb77837..92fcc3bc2c5f4 100644
--- a/clang/lib/CodeGen/ABIInfoImpl.cpp
+++ b/clang/lib/CodeGen/ABIInfoImpl.cpp
@@ -248,7 +248,7 @@ Address CodeGen::emitMergePHI(CodeGenFunction &CGF, Address
Addr1,
return Address(PHI, Addr1.getElementType(), Align);
}
-bool CodeGen::isEmptyField(ASTContext &Context, const FieldDecl *FD,
+bool CodeGen::isEmptyField(const ASTContext &Context, const FieldDecl *FD,
bool AllowArrays, bool AsIfNoUniqueAddr) {
if (FD->isUnnamedBitField())
return true;
@@ -289,8 +289,8 @@ bool CodeGen::isEmptyField(ASTContext &Context, const
FieldDecl *FD,
return isEmptyRecord(Context, FT, AllowArrays, AsIfNoUniqueAddr);
}
-bool CodeGen::isEmptyRecord(ASTContext &Context, QualType T, bool AllowArrays,
-bool AsIfNoUniqueAddr) {
+bool CodeGen::isEmptyRecord(const ASTContext &Context, QualType T,
+bool AllowArrays, bool AsIfNoUniqueAddr) {
const RecordType *RT = T->getAs();
if (!RT)
return false;
diff --git a/clang/lib/CodeGen/ABIInfoImpl.h b/clang/lib/CodeGen/ABIInfoImpl.h
index 92986fb431646..e3f373e39c35a 100644
--- a/clang/lib/CodeGen/ABIInfoImpl.h
+++ b/clang/lib/CodeGen/ABIInfoImpl.h
@@ -126,15 +126,15 @@ Address emitMergePHI(CodeGenFunction &CGF, Address Addr1,
/// is an unnamed bit-field or an (array of) empty record(s). If
/// AsIfNoUniqueAddr is true, then C++ record fields are considered empty if
/// the [[no_unique_address]] attribute would have made them empty.
-bool isEmptyField(ASTContext &Context, const FieldDecl *FD, bool AllowArrays,
- bool AsIfNoUniqueAddr = false);
+bool isEmptyField(const ASTContext &Context, const FieldDecl *FD,
+ bool AllowArrays, bool AsIfNoUniqueAddr = false);
/// isEmptyRecord - Return true iff a structure contains only empty
/// fields. Note that a structure with a flexible array member is not
/// considered empty. If AsIfNoUniqueAddr is true, then C++ r
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
https://github.com/Michael137 updated
https://github.com/llvm/llvm-project/pull/96422
>From f5938919b3a0060db6b373bead1c52f4bb65c841 Mon Sep 17 00:00:00 2001
From: Michael Buch
Date: Fri, 21 Jun 2024 12:15:07 +0100
Subject: [PATCH 1/4] [clang][CGRecordLayout] Remove dependency on isZeroSize
This is a follow-up from the conversation starting at
https://github.com/llvm/llvm-project/pull/93809#issuecomment-2173729801
The root problem that motivated the change are external AST
sources that compute `ASTRecordLayout`s themselves instead of
letting Clang compute them from the AST. One such examples is
LLDB using DWARF to get the definitive offsets and sizes of C++
structures. Such layouts should be considered correct (modulo
buggy DWARF), but various assertions and lowering logic around
the `CGRecordLayoutBuilder` relies on the AST having
`[[no_unique_address]]` attached to them. This is a layout-altering
attribute which is not encoded in DWARF. This causes us LLDB to trip
over the various LLVM<->Clang layout consistency checks. There has been
precedent for avoiding such layout-altering attributes to affect
lowering with externally-provided layouts (e.g., packed structs).
This patch proposes to replace the `isZeroSize` checks in
`CGRecordLayoutBuilder` (which roughly means "empty field
with [[no_unique_address]]") with checks for
`CodeGen::isEmptyField`/`CodeGen::isEmptyRecord`.
---
clang/lib/CodeGen/ABIInfoImpl.cpp | 6 ++---
clang/lib/CodeGen/ABIInfoImpl.h | 6 ++---
clang/lib/CodeGen/CGExpr.cpp | 3 ++-
clang/lib/CodeGen/CGRecordLayoutBuilder.cpp | 14 ++--
clang/test/CodeGen/X86/x86_64-vaarg.c | 3 ++-
clang/test/CodeGen/debug-info-packed-struct.c | 2 +-
clang/test/CodeGen/paren-list-agg-init.cpp| 15 +
.../CodeGenCXX/2011-12-19-init-list-ctor.cpp | 6 ++---
clang/test/CodeGenCXX/auto-var-init.cpp | 10 -
.../test/CodeGenCXX/bitfield-access-empty.cpp | 16 +++---
clang/test/CodeGenCXX/class-layout.cpp| 2 +-
clang/test/CodeGenCXX/compound-literals.cpp | 2 +-
clang/test/CodeGenCXX/exceptions.cpp | 7 +++---
.../lambda-deterministic-captures.cpp | 4 +---
clang/test/CodeGenCXX/ms_struct.cpp | 4 +---
clang/test/CodeGenCXX/partial-destruction.cpp | 11 --
clang/test/CodeGenCXX/pr18962.cpp | 5 ++---
clang/test/CodeGenCXX/references.cpp | 4 +---
clang/test/CodeGenCXX/temporaries.cpp | 12 +-
clang/test/CodeGenObjCXX/lambda-to-block.mm | 9
clang/test/OpenMP/irbuilder_for_iterator.cpp | 10 -
clang/test/OpenMP/irbuilder_for_rangefor.cpp | 14 +---
.../test/OpenMP/task_member_call_codegen.cpp | 22 ---
clang/test/Sema/ms_class_layout.cpp | 14 ++--
24 files changed, 85 insertions(+), 116 deletions(-)
diff --git a/clang/lib/CodeGen/ABIInfoImpl.cpp
b/clang/lib/CodeGen/ABIInfoImpl.cpp
index e9a26abb77837..92fcc3bc2c5f4 100644
--- a/clang/lib/CodeGen/ABIInfoImpl.cpp
+++ b/clang/lib/CodeGen/ABIInfoImpl.cpp
@@ -248,7 +248,7 @@ Address CodeGen::emitMergePHI(CodeGenFunction &CGF, Address
Addr1,
return Address(PHI, Addr1.getElementType(), Align);
}
-bool CodeGen::isEmptyField(ASTContext &Context, const FieldDecl *FD,
+bool CodeGen::isEmptyField(const ASTContext &Context, const FieldDecl *FD,
bool AllowArrays, bool AsIfNoUniqueAddr) {
if (FD->isUnnamedBitField())
return true;
@@ -289,8 +289,8 @@ bool CodeGen::isEmptyField(ASTContext &Context, const
FieldDecl *FD,
return isEmptyRecord(Context, FT, AllowArrays, AsIfNoUniqueAddr);
}
-bool CodeGen::isEmptyRecord(ASTContext &Context, QualType T, bool AllowArrays,
-bool AsIfNoUniqueAddr) {
+bool CodeGen::isEmptyRecord(const ASTContext &Context, QualType T,
+bool AllowArrays, bool AsIfNoUniqueAddr) {
const RecordType *RT = T->getAs();
if (!RT)
return false;
diff --git a/clang/lib/CodeGen/ABIInfoImpl.h b/clang/lib/CodeGen/ABIInfoImpl.h
index 92986fb431646..e3f373e39c35a 100644
--- a/clang/lib/CodeGen/ABIInfoImpl.h
+++ b/clang/lib/CodeGen/ABIInfoImpl.h
@@ -126,15 +126,15 @@ Address emitMergePHI(CodeGenFunction &CGF, Address Addr1,
/// is an unnamed bit-field or an (array of) empty record(s). If
/// AsIfNoUniqueAddr is true, then C++ record fields are considered empty if
/// the [[no_unique_address]] attribute would have made them empty.
-bool isEmptyField(ASTContext &Context, const FieldDecl *FD, bool AllowArrays,
- bool AsIfNoUniqueAddr = false);
+bool isEmptyField(const ASTContext &Context, const FieldDecl *FD,
+ bool AllowArrays, bool AsIfNoUniqueAddr = false);
/// isEmptyRecord - Return true iff a structure contains only empty
/// fields. Note that a structure with a flexible array member is not
/// considered empty. If AsIfNoUniqueAddr is true, then C++ r
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
https://github.com/Michael137 updated
https://github.com/llvm/llvm-project/pull/96422
>From f5938919b3a0060db6b373bead1c52f4bb65c841 Mon Sep 17 00:00:00 2001
From: Michael Buch
Date: Fri, 21 Jun 2024 12:15:07 +0100
Subject: [PATCH 1/4] [clang][CGRecordLayout] Remove dependency on isZeroSize
This is a follow-up from the conversation starting at
https://github.com/llvm/llvm-project/pull/93809#issuecomment-2173729801
The root problem that motivated the change are external AST
sources that compute `ASTRecordLayout`s themselves instead of
letting Clang compute them from the AST. One such examples is
LLDB using DWARF to get the definitive offsets and sizes of C++
structures. Such layouts should be considered correct (modulo
buggy DWARF), but various assertions and lowering logic around
the `CGRecordLayoutBuilder` relies on the AST having
`[[no_unique_address]]` attached to them. This is a layout-altering
attribute which is not encoded in DWARF. This causes us LLDB to trip
over the various LLVM<->Clang layout consistency checks. There has been
precedent for avoiding such layout-altering attributes to affect
lowering with externally-provided layouts (e.g., packed structs).
This patch proposes to replace the `isZeroSize` checks in
`CGRecordLayoutBuilder` (which roughly means "empty field
with [[no_unique_address]]") with checks for
`CodeGen::isEmptyField`/`CodeGen::isEmptyRecord`.
---
clang/lib/CodeGen/ABIInfoImpl.cpp | 6 ++---
clang/lib/CodeGen/ABIInfoImpl.h | 6 ++---
clang/lib/CodeGen/CGExpr.cpp | 3 ++-
clang/lib/CodeGen/CGRecordLayoutBuilder.cpp | 14 ++--
clang/test/CodeGen/X86/x86_64-vaarg.c | 3 ++-
clang/test/CodeGen/debug-info-packed-struct.c | 2 +-
clang/test/CodeGen/paren-list-agg-init.cpp| 15 +
.../CodeGenCXX/2011-12-19-init-list-ctor.cpp | 6 ++---
clang/test/CodeGenCXX/auto-var-init.cpp | 10 -
.../test/CodeGenCXX/bitfield-access-empty.cpp | 16 +++---
clang/test/CodeGenCXX/class-layout.cpp| 2 +-
clang/test/CodeGenCXX/compound-literals.cpp | 2 +-
clang/test/CodeGenCXX/exceptions.cpp | 7 +++---
.../lambda-deterministic-captures.cpp | 4 +---
clang/test/CodeGenCXX/ms_struct.cpp | 4 +---
clang/test/CodeGenCXX/partial-destruction.cpp | 11 --
clang/test/CodeGenCXX/pr18962.cpp | 5 ++---
clang/test/CodeGenCXX/references.cpp | 4 +---
clang/test/CodeGenCXX/temporaries.cpp | 12 +-
clang/test/CodeGenObjCXX/lambda-to-block.mm | 9
clang/test/OpenMP/irbuilder_for_iterator.cpp | 10 -
clang/test/OpenMP/irbuilder_for_rangefor.cpp | 14 +---
.../test/OpenMP/task_member_call_codegen.cpp | 22 ---
clang/test/Sema/ms_class_layout.cpp | 14 ++--
24 files changed, 85 insertions(+), 116 deletions(-)
diff --git a/clang/lib/CodeGen/ABIInfoImpl.cpp
b/clang/lib/CodeGen/ABIInfoImpl.cpp
index e9a26abb77837..92fcc3bc2c5f4 100644
--- a/clang/lib/CodeGen/ABIInfoImpl.cpp
+++ b/clang/lib/CodeGen/ABIInfoImpl.cpp
@@ -248,7 +248,7 @@ Address CodeGen::emitMergePHI(CodeGenFunction &CGF, Address
Addr1,
return Address(PHI, Addr1.getElementType(), Align);
}
-bool CodeGen::isEmptyField(ASTContext &Context, const FieldDecl *FD,
+bool CodeGen::isEmptyField(const ASTContext &Context, const FieldDecl *FD,
bool AllowArrays, bool AsIfNoUniqueAddr) {
if (FD->isUnnamedBitField())
return true;
@@ -289,8 +289,8 @@ bool CodeGen::isEmptyField(ASTContext &Context, const
FieldDecl *FD,
return isEmptyRecord(Context, FT, AllowArrays, AsIfNoUniqueAddr);
}
-bool CodeGen::isEmptyRecord(ASTContext &Context, QualType T, bool AllowArrays,
-bool AsIfNoUniqueAddr) {
+bool CodeGen::isEmptyRecord(const ASTContext &Context, QualType T,
+bool AllowArrays, bool AsIfNoUniqueAddr) {
const RecordType *RT = T->getAs();
if (!RT)
return false;
diff --git a/clang/lib/CodeGen/ABIInfoImpl.h b/clang/lib/CodeGen/ABIInfoImpl.h
index 92986fb431646..e3f373e39c35a 100644
--- a/clang/lib/CodeGen/ABIInfoImpl.h
+++ b/clang/lib/CodeGen/ABIInfoImpl.h
@@ -126,15 +126,15 @@ Address emitMergePHI(CodeGenFunction &CGF, Address Addr1,
/// is an unnamed bit-field or an (array of) empty record(s). If
/// AsIfNoUniqueAddr is true, then C++ record fields are considered empty if
/// the [[no_unique_address]] attribute would have made them empty.
-bool isEmptyField(ASTContext &Context, const FieldDecl *FD, bool AllowArrays,
- bool AsIfNoUniqueAddr = false);
+bool isEmptyField(const ASTContext &Context, const FieldDecl *FD,
+ bool AllowArrays, bool AsIfNoUniqueAddr = false);
/// isEmptyRecord - Return true iff a structure contains only empty
/// fields. Note that a structure with a flexible array member is not
/// considered empty. If AsIfNoUniqueAddr is true, then C++ r
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
https://github.com/Michael137 updated
https://github.com/llvm/llvm-project/pull/96422
>From f5938919b3a0060db6b373bead1c52f4bb65c841 Mon Sep 17 00:00:00 2001
From: Michael Buch
Date: Fri, 21 Jun 2024 12:15:07 +0100
Subject: [PATCH 1/4] [clang][CGRecordLayout] Remove dependency on isZeroSize
This is a follow-up from the conversation starting at
https://github.com/llvm/llvm-project/pull/93809#issuecomment-2173729801
The root problem that motivated the change are external AST
sources that compute `ASTRecordLayout`s themselves instead of
letting Clang compute them from the AST. One such examples is
LLDB using DWARF to get the definitive offsets and sizes of C++
structures. Such layouts should be considered correct (modulo
buggy DWARF), but various assertions and lowering logic around
the `CGRecordLayoutBuilder` relies on the AST having
`[[no_unique_address]]` attached to them. This is a layout-altering
attribute which is not encoded in DWARF. This causes us LLDB to trip
over the various LLVM<->Clang layout consistency checks. There has been
precedent for avoiding such layout-altering attributes to affect
lowering with externally-provided layouts (e.g., packed structs).
This patch proposes to replace the `isZeroSize` checks in
`CGRecordLayoutBuilder` (which roughly means "empty field
with [[no_unique_address]]") with checks for
`CodeGen::isEmptyField`/`CodeGen::isEmptyRecord`.
---
clang/lib/CodeGen/ABIInfoImpl.cpp | 6 ++---
clang/lib/CodeGen/ABIInfoImpl.h | 6 ++---
clang/lib/CodeGen/CGExpr.cpp | 3 ++-
clang/lib/CodeGen/CGRecordLayoutBuilder.cpp | 14 ++--
clang/test/CodeGen/X86/x86_64-vaarg.c | 3 ++-
clang/test/CodeGen/debug-info-packed-struct.c | 2 +-
clang/test/CodeGen/paren-list-agg-init.cpp| 15 +
.../CodeGenCXX/2011-12-19-init-list-ctor.cpp | 6 ++---
clang/test/CodeGenCXX/auto-var-init.cpp | 10 -
.../test/CodeGenCXX/bitfield-access-empty.cpp | 16 +++---
clang/test/CodeGenCXX/class-layout.cpp| 2 +-
clang/test/CodeGenCXX/compound-literals.cpp | 2 +-
clang/test/CodeGenCXX/exceptions.cpp | 7 +++---
.../lambda-deterministic-captures.cpp | 4 +---
clang/test/CodeGenCXX/ms_struct.cpp | 4 +---
clang/test/CodeGenCXX/partial-destruction.cpp | 11 --
clang/test/CodeGenCXX/pr18962.cpp | 5 ++---
clang/test/CodeGenCXX/references.cpp | 4 +---
clang/test/CodeGenCXX/temporaries.cpp | 12 +-
clang/test/CodeGenObjCXX/lambda-to-block.mm | 9
clang/test/OpenMP/irbuilder_for_iterator.cpp | 10 -
clang/test/OpenMP/irbuilder_for_rangefor.cpp | 14 +---
.../test/OpenMP/task_member_call_codegen.cpp | 22 ---
clang/test/Sema/ms_class_layout.cpp | 14 ++--
24 files changed, 85 insertions(+), 116 deletions(-)
diff --git a/clang/lib/CodeGen/ABIInfoImpl.cpp
b/clang/lib/CodeGen/ABIInfoImpl.cpp
index e9a26abb77837..92fcc3bc2c5f4 100644
--- a/clang/lib/CodeGen/ABIInfoImpl.cpp
+++ b/clang/lib/CodeGen/ABIInfoImpl.cpp
@@ -248,7 +248,7 @@ Address CodeGen::emitMergePHI(CodeGenFunction &CGF, Address
Addr1,
return Address(PHI, Addr1.getElementType(), Align);
}
-bool CodeGen::isEmptyField(ASTContext &Context, const FieldDecl *FD,
+bool CodeGen::isEmptyField(const ASTContext &Context, const FieldDecl *FD,
bool AllowArrays, bool AsIfNoUniqueAddr) {
if (FD->isUnnamedBitField())
return true;
@@ -289,8 +289,8 @@ bool CodeGen::isEmptyField(ASTContext &Context, const
FieldDecl *FD,
return isEmptyRecord(Context, FT, AllowArrays, AsIfNoUniqueAddr);
}
-bool CodeGen::isEmptyRecord(ASTContext &Context, QualType T, bool AllowArrays,
-bool AsIfNoUniqueAddr) {
+bool CodeGen::isEmptyRecord(const ASTContext &Context, QualType T,
+bool AllowArrays, bool AsIfNoUniqueAddr) {
const RecordType *RT = T->getAs();
if (!RT)
return false;
diff --git a/clang/lib/CodeGen/ABIInfoImpl.h b/clang/lib/CodeGen/ABIInfoImpl.h
index 92986fb431646..e3f373e39c35a 100644
--- a/clang/lib/CodeGen/ABIInfoImpl.h
+++ b/clang/lib/CodeGen/ABIInfoImpl.h
@@ -126,15 +126,15 @@ Address emitMergePHI(CodeGenFunction &CGF, Address Addr1,
/// is an unnamed bit-field or an (array of) empty record(s). If
/// AsIfNoUniqueAddr is true, then C++ record fields are considered empty if
/// the [[no_unique_address]] attribute would have made them empty.
-bool isEmptyField(ASTContext &Context, const FieldDecl *FD, bool AllowArrays,
- bool AsIfNoUniqueAddr = false);
+bool isEmptyField(const ASTContext &Context, const FieldDecl *FD,
+ bool AllowArrays, bool AsIfNoUniqueAddr = false);
/// isEmptyRecord - Return true iff a structure contains only empty
/// fields. Note that a structure with a flexible array member is not
/// considered empty. If AsIfNoUniqueAddr is true, then C++ r
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
efriedma-quic wrote: This is roughly what I expected the patch to look like. Maybe consider adding a couple small wrapper functions around isEmptyRecord/isEmptyField to simplify the uses (and so we can use a name that indicates why the checks exist). https://github.com/llvm/llvm-project/pull/96422 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
efriedma-quic wrote: (Please move out of "draft" when you think this is ready.) https://github.com/llvm/llvm-project/pull/96422 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
https://github.com/Michael137 edited https://github.com/llvm/llvm-project/pull/96422 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
https://github.com/Michael137 edited https://github.com/llvm/llvm-project/pull/96422 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
Michael137 wrote: > (Please move out of "draft" when you think this is ready.) Updated the PR with some more context. There's still a few instances of `FieldDecl::isZeroSize`/`CXXRecordDecl::isEmpty` around `CodeGen` (see `CGClass`, `CGExprConstant`), but it wasn't obvious to me whether those needed replacing as well. Particularly `ConstStructBuilder::Build` has `[[no_unique_address]]` specific logic. https://github.com/llvm/llvm-project/pull/96422 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
https://github.com/Michael137 ready_for_review https://github.com/llvm/llvm-project/pull/96422 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
llvmbot wrote:
@llvm/pr-subscribers-clang-codegen
Author: Michael Buch (Michael137)
Changes
This is a follow-up from the conversation starting at
https://github.com/llvm/llvm-project/pull/93809#issuecomment-2173729801
The root problem that motivated the change are external AST sources that
compute `ASTRecordLayout`s themselves instead of letting Clang compute them
from the AST. One such example is LLDB using DWARF to get the definitive
offsets and sizes of C++ structures. Such layouts should be considered correct
(modulo buggy DWARF), but various assertions and lowering logic around the
`CGRecordLayoutBuilder` relies on the AST having `[[no_unique_address]]`
attached to them. This is a layout-altering attribute which is not encoded in
DWARF. This causes us LLDB to trip over the various LLVM<->Clang layout
consistency checks. There has been precedent for avoiding such layout-altering
attributes from affecting lowering with externally-provided layouts (e.g.,
packed structs).
This patch proposes to replace the `isZeroSize` checks in
`CGRecordLayoutBuilder` (which roughly means "empty field with
[[no_unique_address]]") with checks for
`CodeGen::isEmptyField`/`CodeGen::isEmptyRecord`.
**Details**
The main strategy here was to change the `isZeroSize` check in
`CGRecordLowering::accumulateFields` and `CGRecordLowering::accumulateBases` to
use the `isEmptyXXX` APIs instead, preventing empty fields from being added to
the `Members` and `Bases` structures. The rest of the changes fall out from
here, to prevent lookups into these structures (for field numbers or base
indices) from failing.
Added `isEmptyRecordForLayout` and `isEmptyFieldForLayout` (open to better
naming suggestions), which mostly just dispatch to the
`isEmptyRecord`/`isEmptyField` respectively. The main difference is that
`isEmptyFieldForLayout` doesn't treat `unnamed bitfields` as unconditionally
empty (unlike `isEmptyField`).
---
Patch is 46.65 KiB, truncated to 20.00 KiB below, full version:
https://github.com/llvm/llvm-project/pull/96422.diff
26 Files Affected:
- (modified) clang/lib/CodeGen/ABIInfoImpl.cpp (+20-3)
- (modified) clang/lib/CodeGen/ABIInfoImpl.h (+12-3)
- (modified) clang/lib/CodeGen/CGExpr.cpp (+2-1)
- (modified) clang/lib/CodeGen/CGExprConstant.cpp (+6-3)
- (modified) clang/lib/CodeGen/CGRecordLayoutBuilder.cpp (+8-7)
- (modified) clang/test/CodeGen/X86/x86_64-vaarg.c (+2-1)
- (modified) clang/test/CodeGen/debug-info-packed-struct.c (+1-1)
- (modified) clang/test/CodeGen/paren-list-agg-init.cpp (+6-9)
- (modified) clang/test/CodeGen/voidptr-vaarg.c (+2-1)
- (modified) clang/test/CodeGenCXX/2011-12-19-init-list-ctor.cpp (+3-3)
- (modified) clang/test/CodeGenCXX/auto-var-init.cpp (+9-6)
- (modified) clang/test/CodeGenCXX/bitfield-access-empty.cpp (+8-8)
- (modified) clang/test/CodeGenCXX/class-layout.cpp (+1-1)
- (modified) clang/test/CodeGenCXX/compound-literals.cpp (+1-1)
- (modified) clang/test/CodeGenCXX/exceptions.cpp (+3-4)
- (modified) clang/test/CodeGenCXX/lambda-deterministic-captures.cpp (+1-3)
- (modified) clang/test/CodeGenCXX/ms_struct.cpp (+1-3)
- (modified) clang/test/CodeGenCXX/partial-destruction.cpp (+4-7)
- (modified) clang/test/CodeGenCXX/pr18962.cpp (+2-3)
- (modified) clang/test/CodeGenCXX/references.cpp (+1-3)
- (modified) clang/test/CodeGenCXX/temporaries.cpp (+6-6)
- (modified) clang/test/CodeGenObjCXX/lambda-to-block.mm (+4-5)
- (modified) clang/test/OpenMP/irbuilder_for_iterator.cpp (+4-6)
- (modified) clang/test/OpenMP/irbuilder_for_rangefor.cpp (+6-8)
- (modified) clang/test/OpenMP/task_member_call_codegen.cpp (+9-13)
- (modified) clang/test/Sema/ms_class_layout.cpp (+2-12)
``diff
diff --git a/clang/lib/CodeGen/ABIInfoImpl.cpp
b/clang/lib/CodeGen/ABIInfoImpl.cpp
index e9a26abb77837..703e69e889967 100644
--- a/clang/lib/CodeGen/ABIInfoImpl.cpp
+++ b/clang/lib/CodeGen/ABIInfoImpl.cpp
@@ -248,7 +248,7 @@ Address CodeGen::emitMergePHI(CodeGenFunction &CGF, Address
Addr1,
return Address(PHI, Addr1.getElementType(), Align);
}
-bool CodeGen::isEmptyField(ASTContext &Context, const FieldDecl *FD,
+bool CodeGen::isEmptyField(const ASTContext &Context, const FieldDecl *FD,
bool AllowArrays, bool AsIfNoUniqueAddr) {
if (FD->isUnnamedBitField())
return true;
@@ -289,8 +289,20 @@ bool CodeGen::isEmptyField(ASTContext &Context, const
FieldDecl *FD,
return isEmptyRecord(Context, FT, AllowArrays, AsIfNoUniqueAddr);
}
-bool CodeGen::isEmptyRecord(ASTContext &Context, QualType T, bool AllowArrays,
-bool AsIfNoUniqueAddr) {
+bool CodeGen::isEmptyFieldForLayout(const ASTContext &Context,
+const FieldDecl *FD) {
+ if (FD->isZeroLengthBitField(Context))
+return true;
+
+ if (FD->isUnnamedBitField())
+return false;
+
+ return isEmptyField(Context, FD, /*AllowArrays=*/false,
+ /*AsIfNoUniqueAddr=*/true);
+}
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
rnk wrote: IIUC, this effectively removes all empty records from LLVM struct types. This makes the IR less "pretty", but these subobjects are notionally empty and consist of only padding bytes (i8 and i8 arrays) at the end of the day. I think that's acceptable. + @rjmccall , does that sound good to you? https://github.com/llvm/llvm-project/pull/96422 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
rjmccall wrote: That sounds fine to me as long as we're still emitting projections of them properly (i.e. not just assuming "oh, it's an empty record, we can use whatever pointer we want because it'll never be dereferenced"). https://github.com/llvm/llvm-project/pull/96422 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
efriedma-quic wrote: For CGExprConstant, the code is checking "empty" in the sense of whether there's a corresponding LLVM field. So almost certainly needs changes. Not sure how that isn't causing test failures; maybe there's missing test coverage. For CGClass, it's not directly tied to the LLVM structure layout, but I'm not sure the generated code would be semantically correct if an "empty" field that isn't isEmpty() overlaps with actual data. https://github.com/llvm/llvm-project/pull/96422 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
https://github.com/Michael137 updated
https://github.com/llvm/llvm-project/pull/96422
>From 46d15431cae1123282f4c0856360c9e3ce7322fc Mon Sep 17 00:00:00 2001
From: Michael Buch
Date: Fri, 21 Jun 2024 12:15:07 +0100
Subject: [PATCH 01/20] [clang][CGRecordLayout] Remove dependency on isZeroSize
This is a follow-up from the conversation starting at
https://github.com/llvm/llvm-project/pull/93809#issuecomment-2173729801
The root problem that motivated the change are external AST
sources that compute `ASTRecordLayout`s themselves instead of
letting Clang compute them from the AST. One such examples is
LLDB using DWARF to get the definitive offsets and sizes of C++
structures. Such layouts should be considered correct (modulo
buggy DWARF), but various assertions and lowering logic around
the `CGRecordLayoutBuilder` relies on the AST having
`[[no_unique_address]]` attached to them. This is a layout-altering
attribute which is not encoded in DWARF. This causes us LLDB to trip
over the various LLVM<->Clang layout consistency checks. There has been
precedent for avoiding such layout-altering attributes to affect
lowering with externally-provided layouts (e.g., packed structs).
This patch proposes to replace the `isZeroSize` checks in
`CGRecordLayoutBuilder` (which roughly means "empty field
with [[no_unique_address]]") with checks for
`CodeGen::isEmptyField`/`CodeGen::isEmptyRecord`.
---
clang/lib/CodeGen/ABIInfoImpl.cpp | 6 ++---
clang/lib/CodeGen/ABIInfoImpl.h | 6 ++---
clang/lib/CodeGen/CGExpr.cpp | 3 ++-
clang/lib/CodeGen/CGRecordLayoutBuilder.cpp | 14 ++--
clang/test/CodeGen/X86/x86_64-vaarg.c | 3 ++-
clang/test/CodeGen/debug-info-packed-struct.c | 2 +-
clang/test/CodeGen/paren-list-agg-init.cpp| 15 +
.../CodeGenCXX/2011-12-19-init-list-ctor.cpp | 6 ++---
clang/test/CodeGenCXX/auto-var-init.cpp | 10 -
.../test/CodeGenCXX/bitfield-access-empty.cpp | 16 +++---
clang/test/CodeGenCXX/class-layout.cpp| 2 +-
clang/test/CodeGenCXX/compound-literals.cpp | 2 +-
clang/test/CodeGenCXX/exceptions.cpp | 7 +++---
.../lambda-deterministic-captures.cpp | 4 +---
clang/test/CodeGenCXX/ms_struct.cpp | 4 +---
clang/test/CodeGenCXX/partial-destruction.cpp | 11 --
clang/test/CodeGenCXX/pr18962.cpp | 5 ++---
clang/test/CodeGenCXX/references.cpp | 4 +---
clang/test/CodeGenCXX/temporaries.cpp | 12 +-
clang/test/CodeGenObjCXX/lambda-to-block.mm | 9
clang/test/OpenMP/irbuilder_for_iterator.cpp | 10 -
clang/test/OpenMP/irbuilder_for_rangefor.cpp | 14 +---
.../test/OpenMP/task_member_call_codegen.cpp | 22 ---
clang/test/Sema/ms_class_layout.cpp | 14 ++--
24 files changed, 85 insertions(+), 116 deletions(-)
diff --git a/clang/lib/CodeGen/ABIInfoImpl.cpp
b/clang/lib/CodeGen/ABIInfoImpl.cpp
index e9a26abb77837..92fcc3bc2c5f4 100644
--- a/clang/lib/CodeGen/ABIInfoImpl.cpp
+++ b/clang/lib/CodeGen/ABIInfoImpl.cpp
@@ -248,7 +248,7 @@ Address CodeGen::emitMergePHI(CodeGenFunction &CGF, Address
Addr1,
return Address(PHI, Addr1.getElementType(), Align);
}
-bool CodeGen::isEmptyField(ASTContext &Context, const FieldDecl *FD,
+bool CodeGen::isEmptyField(const ASTContext &Context, const FieldDecl *FD,
bool AllowArrays, bool AsIfNoUniqueAddr) {
if (FD->isUnnamedBitField())
return true;
@@ -289,8 +289,8 @@ bool CodeGen::isEmptyField(ASTContext &Context, const
FieldDecl *FD,
return isEmptyRecord(Context, FT, AllowArrays, AsIfNoUniqueAddr);
}
-bool CodeGen::isEmptyRecord(ASTContext &Context, QualType T, bool AllowArrays,
-bool AsIfNoUniqueAddr) {
+bool CodeGen::isEmptyRecord(const ASTContext &Context, QualType T,
+bool AllowArrays, bool AsIfNoUniqueAddr) {
const RecordType *RT = T->getAs();
if (!RT)
return false;
diff --git a/clang/lib/CodeGen/ABIInfoImpl.h b/clang/lib/CodeGen/ABIInfoImpl.h
index 92986fb431646..e3f373e39c35a 100644
--- a/clang/lib/CodeGen/ABIInfoImpl.h
+++ b/clang/lib/CodeGen/ABIInfoImpl.h
@@ -126,15 +126,15 @@ Address emitMergePHI(CodeGenFunction &CGF, Address Addr1,
/// is an unnamed bit-field or an (array of) empty record(s). If
/// AsIfNoUniqueAddr is true, then C++ record fields are considered empty if
/// the [[no_unique_address]] attribute would have made them empty.
-bool isEmptyField(ASTContext &Context, const FieldDecl *FD, bool AllowArrays,
- bool AsIfNoUniqueAddr = false);
+bool isEmptyField(const ASTContext &Context, const FieldDecl *FD,
+ bool AllowArrays, bool AsIfNoUniqueAddr = false);
/// isEmptyRecord - Return true iff a structure contains only empty
/// fields. Note that a structure with a flexible array member is not
/// considered empty. If AsIfNoUniqueAddr is true, then C++
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
https://github.com/Michael137 closed https://github.com/llvm/llvm-project/pull/96422 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
@@ -137,6 +137,16 @@ bool isEmptyField(ASTContext &Context, const FieldDecl *FD, bool AllowArrays, bool isEmptyRecord(ASTContext &Context, QualType T, bool AllowArrays, bool AsIfNoUniqueAddr = false); +/// isEmptyFieldForLayout - Return true iff the field is "empty", that is, +/// either a zero-width bit-field or an \ref isEmptyRecordForLayout. +bool isEmptyFieldForLayout(const ASTContext &Context, const FieldDecl *FD); + +/// isEmptyRecordForLayout - Return true iff a structure contains only empty +/// base classes (per \ref isEmptyRecordForLayout) and fields (per +/// \ref isEmptyFieldForLayout). Note, C++ record fields are considered empty +/// if the [[no_unique_address]] attribute would have made them empty. +bool isEmptyRecordForLayout(const ASTContext &Context, QualType T); s-barannikov wrote: These functions don't belong here. This file contains helper functions for use in implementations of ABIInfo class. I guess CGRecordLayout* could be a better place for them. https://github.com/llvm/llvm-project/pull/96422 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][CGRecordLayout] Remove dependency on isZeroSize (PR #96422)
@@ -137,6 +137,16 @@ bool isEmptyField(ASTContext &Context, const FieldDecl *FD, bool AllowArrays, bool isEmptyRecord(ASTContext &Context, QualType T, bool AllowArrays, bool AsIfNoUniqueAddr = false); +/// isEmptyFieldForLayout - Return true iff the field is "empty", that is, +/// either a zero-width bit-field or an \ref isEmptyRecordForLayout. +bool isEmptyFieldForLayout(const ASTContext &Context, const FieldDecl *FD); + +/// isEmptyRecordForLayout - Return true iff a structure contains only empty +/// base classes (per \ref isEmptyRecordForLayout) and fields (per +/// \ref isEmptyFieldForLayout). Note, C++ record fields are considered empty +/// if the [[no_unique_address]] attribute would have made them empty. +bool isEmptyRecordForLayout(const ASTContext &Context, QualType T); Michael137 wrote: I put those here just to have them next to `isEmptyRecord`/`isEmptyField`, which are used outside the implementation of `ABIInfo`. But agree with your point. These should probably all live in `CGRecordLayout` instead https://github.com/llvm/llvm-project/pull/96422 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
