Michael137 wrote:
> This change leads to a crash in `ConstStructBuilder::Build()` for the
> following program:
>
> ```
> struct S {
> };
>
> union U {
> struct S s;
> int x;
> };
>
> void foo() {
> union U bar = {};
> }
> ```
>
> `isEmptyRecordForLayout` returns false for `union
hnrklssn wrote:
This change leads to a crash in the following case:
```
struct S {
};
union U {
struct S s;
int x;
};
void foo() {
union U bar = {};
}
```
`isEmptyRecordForLayout` returns false for `union U` because the recursive call
for `U::x` returns false. This means we call `I
@@ -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 i
@@ -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 i
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
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
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
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
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
@@ -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(Con
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
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 =
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 re
@@ -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
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
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
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
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
@@ -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
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
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
@@ -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; };
unsi
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
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
@@ -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({
@@ -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({
@@ -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; };
unsi
@@ -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 i
@@ -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({
@@ -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({
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
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
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
@@ -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({
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://list
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
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
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
Th
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-b
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 referri
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
Th
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 unna
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/listinf
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
Th
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 adju
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
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
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 go
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 th
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
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
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
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
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
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/
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
Th
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
Th
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
Th
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
Th
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
Th
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
Th
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:
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
63 matches
Mail list logo
