[clang] [CLANG][MS-STRUCT] bitfield padding warning presents padding to exact bit count (PR #136062)
=?utf-8?q?Théo?= de Magalhaes Message-ID: In-Reply-To: https://github.com/efriedma-quic closed https://github.com/llvm/llvm-project/pull/136062 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [CLANG][MS-STRUCT] bitfield padding warning presents padding to exact bit count (PR #136062)
=?utf-8?q?Th=C3=A9o?= de Magalhaes Message-ID: In-Reply-To: https://github.com/efriedma-quic approved this pull request. Oh, I see, I missed the nesting. LGTM https://github.com/llvm/llvm-project/pull/136062 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [CLANG][MS-STRUCT] bitfield padding warning presents padding to exact bit count (PR #136062)
=?utf-8?q?Théo?= de Magalhaes Message-ID: In-Reply-To: theomagellan wrote: It doesn't affect non-ms_struct definitions! The new `PaddingInLastUnit` variable only gets a non-zero value when the following condition is met: `IsMsStruct && (LastBitfieldStorageUnitSize != StorageUnitSize || UnfilledBitsInLastUnit < FieldSize)` Otherwise, it remains zero and doesn't impact any calculations. I could still add tests for that but I am not sure what they would look like as so far the tests only make sure both ABIs (MSVC and Itanium under ms_struct) produce the correct padding warnings. I'm not entirely sure what new cases you'd like to see covered. Do you have something specific in mind? I gave the `Foo` structure a more descriptive name, thank you. https://github.com/llvm/llvm-project/pull/136062 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [CLANG][MS-STRUCT] bitfield padding warning presents padding to exact bit count (PR #136062)
=?utf-8?q?Théo?= de Magalhaes
Message-ID:
In-Reply-To:
https://github.com/theomagellan updated
https://github.com/llvm/llvm-project/pull/136062
>From 842f0fbed0043ad0aa1679d8a30bc13d64eb25cf Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Th=C3=A9o=20de=20Magalhaes?=
Date: Thu, 17 Apr 2025 01:45:12 +0200
Subject: [PATCH 1/2] fix(ms_struct): bitfield padding warning presents padding
to exact bit count
---
clang/lib/AST/RecordLayoutBuilder.cpp | 4 +++-
clang/test/SemaCXX/windows-Wpadded-bitfield.cpp | 15 +++
2 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/clang/lib/AST/RecordLayoutBuilder.cpp
b/clang/lib/AST/RecordLayoutBuilder.cpp
index ea353f88a8aec..ca08e186f4ff2 100644
--- a/clang/lib/AST/RecordLayoutBuilder.cpp
+++ b/clang/lib/AST/RecordLayoutBuilder.cpp
@@ -1538,6 +1538,7 @@ void ItaniumRecordLayoutBuilder::LayoutBitField(const
FieldDecl *D) {
uint64_t StorageUnitSize = FieldInfo.Width;
unsigned FieldAlign = FieldInfo.Align;
bool AlignIsRequired = FieldInfo.isAlignRequired();
+ unsigned char PaddingInLastUnit = 0;
// UnfilledBitsInLastUnit is the difference between the end of the
// last allocated bitfield (i.e. the first bit offset available for
@@ -1610,6 +1611,7 @@ void ItaniumRecordLayoutBuilder::LayoutBitField(const
FieldDecl *D) {
if (!LastBitfieldStorageUnitSize && !FieldSize)
FieldAlign = 1;
+ PaddingInLastUnit = UnfilledBitsInLastUnit;
UnfilledBitsInLastUnit = 0;
LastBitfieldStorageUnitSize = 0;
}
@@ -1706,7 +1708,7 @@ void ItaniumRecordLayoutBuilder::LayoutBitField(const
FieldDecl *D) {
// For purposes of diagnostics, we're going to simultaneously
// compute the field offsets that we would have used if we weren't
// adding any alignment padding or if the field weren't packed.
- uint64_t UnpaddedFieldOffset = FieldOffset;
+ uint64_t UnpaddedFieldOffset = FieldOffset - PaddingInLastUnit;
uint64_t UnpackedFieldOffset = FieldOffset;
// Check if we need to add padding to fit the bitfield within an
diff --git a/clang/test/SemaCXX/windows-Wpadded-bitfield.cpp
b/clang/test/SemaCXX/windows-Wpadded-bitfield.cpp
index ee5a57124eca5..0b88b4c170617 100644
--- a/clang/test/SemaCXX/windows-Wpadded-bitfield.cpp
+++ b/clang/test/SemaCXX/windows-Wpadded-bitfield.cpp
@@ -1,4 +1,5 @@
// RUN: %clang_cc1 -triple x86_64-windows-msvc -fsyntax-only -verify -Wpadded
%s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -verify -Wpadded %s
struct __attribute__((ms_struct)) BitfieldStruct { // expected-warning
{{padding size of 'BitfieldStruct' with 3 bytes to alignment boundary}}
char c : 1;
@@ -24,9 +25,23 @@ struct __attribute__((ms_struct)) DifferentUnitSizeBitfield
{ // expected-warnin
char i; // expected-warning {{padding struct 'DifferentUnitSizeBitfield'
with 31 bits to align 'i'}}
};
+struct __attribute__((ms_struct)) Foo { // expected-warning {{padding size of
'Foo' with 63 bits to alignment boundary}}
+ long long x;
+ char a : 1;
+ long long b : 1; // expected-warning {{padding struct 'Foo' with 63 bits to
align 'b'}}
+};
+
+struct __attribute__((ms_struct)) SameUnitSizeMultiple { // expected-warning
{{padding size of 'SameUnitSizeMultiple' with 2 bits to alignment boundary}}
+ char c : 1;
+ char cc : 2;
+ char ccc : 3;
+};
+
int main() {
BitfieldStruct b;
SevenBitfieldStruct s;
SameUnitSizeBitfield su;
DifferentUnitSizeBitfield du;
+ Foo f;
+ SameUnitSizeMultiple susm;
}
>From 7d003a1d49867bd7e0ea25f597d03944b1e1ef7b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Th=C3=A9o=20de=20Magalhaes?=
Date: Tue, 22 Apr 2025 00:06:39 +0200
Subject: [PATCH 2/2] fix(bitfield-test): changed struct name to describe test
case
---
clang/test/SemaCXX/windows-Wpadded-bitfield.cpp | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/clang/test/SemaCXX/windows-Wpadded-bitfield.cpp
b/clang/test/SemaCXX/windows-Wpadded-bitfield.cpp
index 0b88b4c170617..28917ffaadb6f 100644
--- a/clang/test/SemaCXX/windows-Wpadded-bitfield.cpp
+++ b/clang/test/SemaCXX/windows-Wpadded-bitfield.cpp
@@ -25,10 +25,10 @@ struct __attribute__((ms_struct)) DifferentUnitSizeBitfield
{ // expected-warnin
char i; // expected-warning {{padding struct 'DifferentUnitSizeBitfield'
with 31 bits to align 'i'}}
};
-struct __attribute__((ms_struct)) Foo { // expected-warning {{padding size of
'Foo' with 63 bits to alignment boundary}}
+struct __attribute__((ms_struct)) BitfieldBigPadding { // expected-warning
{{padding size of 'BitfieldBigPadding' with 63 bits to alignment boundary}}
long long x;
char a : 1;
- long long b : 1; // expected-warning {{padding struct 'Foo' with 63 bits to
align 'b'}}
+ long long b : 1; // expected-warning {{padding struct 'BitfieldBigPadding'
with 63 bits to align 'b'}}
};
struct __attribute__((ms_struct)) SameUnitSizeMultiple { // expected-warning
{{padding size of 'SameUnitSizeMultiple' with 2 bits to alignment boun
[clang] [CLANG][MS-STRUCT] bitfield padding warning presents padding to exact bit count (PR #136062)
efriedma-quic wrote: If I'm following correctly, this also affects non-ms_struct definitions? Can you also add test coverage for that? I'm not sure people will be happy to see "padding struct 'Foo' with 1 bit to align 'xx'", but I guess we can see how it goes. Looks like gcc's -Wpadded does warn. https://github.com/llvm/llvm-project/pull/136062 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [CLANG][MS-STRUCT] bitfield padding warning presents padding to exact bit count (PR #136062)
theomagellan wrote: Hello @efriedma-quic, hope you don’t mind the ping. This patch addresses the padded bitfield warning, which was reporting the wrong number of padded bits when using `ms_struct` and two consecutive bit-fields under the Itanium ABI. I originally planned to merge `SemaCXX/windows-Wpadded.cc` and `SemaCXX/windows-Wpadded-bitfield.cc` into a single test file, but after some consideration, I think keeping them separate makes the coverage clearer. What do you think? Thank you! https://github.com/llvm/llvm-project/pull/136062 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [CLANG][MS-STRUCT] bitfield padding warning presents padding to exact bit count (PR #136062)
https://github.com/theomagellan ready_for_review https://github.com/llvm/llvm-project/pull/136062 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [CLANG][MS-STRUCT] bitfield padding warning presents padding to exact bit count (PR #136062)
https://github.com/theomagellan updated
https://github.com/llvm/llvm-project/pull/136062
>From 842f0fbed0043ad0aa1679d8a30bc13d64eb25cf Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Th=C3=A9o=20de=20Magalhaes?=
Date: Thu, 17 Apr 2025 01:45:12 +0200
Subject: [PATCH] fix(ms_struct): bitfield padding warning presents padding to
exact bit count
---
clang/lib/AST/RecordLayoutBuilder.cpp | 4 +++-
clang/test/SemaCXX/windows-Wpadded-bitfield.cpp | 15 +++
2 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/clang/lib/AST/RecordLayoutBuilder.cpp
b/clang/lib/AST/RecordLayoutBuilder.cpp
index ea353f88a8aec..ca08e186f4ff2 100644
--- a/clang/lib/AST/RecordLayoutBuilder.cpp
+++ b/clang/lib/AST/RecordLayoutBuilder.cpp
@@ -1538,6 +1538,7 @@ void ItaniumRecordLayoutBuilder::LayoutBitField(const
FieldDecl *D) {
uint64_t StorageUnitSize = FieldInfo.Width;
unsigned FieldAlign = FieldInfo.Align;
bool AlignIsRequired = FieldInfo.isAlignRequired();
+ unsigned char PaddingInLastUnit = 0;
// UnfilledBitsInLastUnit is the difference between the end of the
// last allocated bitfield (i.e. the first bit offset available for
@@ -1610,6 +1611,7 @@ void ItaniumRecordLayoutBuilder::LayoutBitField(const
FieldDecl *D) {
if (!LastBitfieldStorageUnitSize && !FieldSize)
FieldAlign = 1;
+ PaddingInLastUnit = UnfilledBitsInLastUnit;
UnfilledBitsInLastUnit = 0;
LastBitfieldStorageUnitSize = 0;
}
@@ -1706,7 +1708,7 @@ void ItaniumRecordLayoutBuilder::LayoutBitField(const
FieldDecl *D) {
// For purposes of diagnostics, we're going to simultaneously
// compute the field offsets that we would have used if we weren't
// adding any alignment padding or if the field weren't packed.
- uint64_t UnpaddedFieldOffset = FieldOffset;
+ uint64_t UnpaddedFieldOffset = FieldOffset - PaddingInLastUnit;
uint64_t UnpackedFieldOffset = FieldOffset;
// Check if we need to add padding to fit the bitfield within an
diff --git a/clang/test/SemaCXX/windows-Wpadded-bitfield.cpp
b/clang/test/SemaCXX/windows-Wpadded-bitfield.cpp
index ee5a57124eca5..0b88b4c170617 100644
--- a/clang/test/SemaCXX/windows-Wpadded-bitfield.cpp
+++ b/clang/test/SemaCXX/windows-Wpadded-bitfield.cpp
@@ -1,4 +1,5 @@
// RUN: %clang_cc1 -triple x86_64-windows-msvc -fsyntax-only -verify -Wpadded
%s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -verify -Wpadded %s
struct __attribute__((ms_struct)) BitfieldStruct { // expected-warning
{{padding size of 'BitfieldStruct' with 3 bytes to alignment boundary}}
char c : 1;
@@ -24,9 +25,23 @@ struct __attribute__((ms_struct)) DifferentUnitSizeBitfield
{ // expected-warnin
char i; // expected-warning {{padding struct 'DifferentUnitSizeBitfield'
with 31 bits to align 'i'}}
};
+struct __attribute__((ms_struct)) Foo { // expected-warning {{padding size of
'Foo' with 63 bits to alignment boundary}}
+ long long x;
+ char a : 1;
+ long long b : 1; // expected-warning {{padding struct 'Foo' with 63 bits to
align 'b'}}
+};
+
+struct __attribute__((ms_struct)) SameUnitSizeMultiple { // expected-warning
{{padding size of 'SameUnitSizeMultiple' with 2 bits to alignment boundary}}
+ char c : 1;
+ char cc : 2;
+ char ccc : 3;
+};
+
int main() {
BitfieldStruct b;
SevenBitfieldStruct s;
SameUnitSizeBitfield su;
DifferentUnitSizeBitfield du;
+ Foo f;
+ SameUnitSizeMultiple susm;
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [CLANG][MS-STRUCT] bitfield padding warning presents padding to exact bit count (PR #136062)
https://github.com/theomagellan converted_to_draft https://github.com/llvm/llvm-project/pull/136062 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [CLANG][MS-STRUCT] bitfield padding warning presents padding to exact bit count (PR #136062)
llvmbot wrote:
@llvm/pr-subscribers-clang
Author: Theo de Magalhaes (theomagellan)
Changes
Aims to fix #131647.
---
Full diff: https://github.com/llvm/llvm-project/pull/136062.diff
2 Files Affected:
- (modified) clang/lib/AST/RecordLayoutBuilder.cpp (+3-1)
- (modified) clang/test/SemaCXX/windows-Wpadded-bitfield.cpp (+15)
``diff
diff --git a/clang/lib/AST/RecordLayoutBuilder.cpp
b/clang/lib/AST/RecordLayoutBuilder.cpp
index ea353f88a8aec..ca08e186f4ff2 100644
--- a/clang/lib/AST/RecordLayoutBuilder.cpp
+++ b/clang/lib/AST/RecordLayoutBuilder.cpp
@@ -1538,6 +1538,7 @@ void ItaniumRecordLayoutBuilder::LayoutBitField(const
FieldDecl *D) {
uint64_t StorageUnitSize = FieldInfo.Width;
unsigned FieldAlign = FieldInfo.Align;
bool AlignIsRequired = FieldInfo.isAlignRequired();
+ unsigned char PaddingInLastUnit = 0;
// UnfilledBitsInLastUnit is the difference between the end of the
// last allocated bitfield (i.e. the first bit offset available for
@@ -1610,6 +1611,7 @@ void ItaniumRecordLayoutBuilder::LayoutBitField(const
FieldDecl *D) {
if (!LastBitfieldStorageUnitSize && !FieldSize)
FieldAlign = 1;
+ PaddingInLastUnit = UnfilledBitsInLastUnit;
UnfilledBitsInLastUnit = 0;
LastBitfieldStorageUnitSize = 0;
}
@@ -1706,7 +1708,7 @@ void ItaniumRecordLayoutBuilder::LayoutBitField(const
FieldDecl *D) {
// For purposes of diagnostics, we're going to simultaneously
// compute the field offsets that we would have used if we weren't
// adding any alignment padding or if the field weren't packed.
- uint64_t UnpaddedFieldOffset = FieldOffset;
+ uint64_t UnpaddedFieldOffset = FieldOffset - PaddingInLastUnit;
uint64_t UnpackedFieldOffset = FieldOffset;
// Check if we need to add padding to fit the bitfield within an
diff --git a/clang/test/SemaCXX/windows-Wpadded-bitfield.cpp
b/clang/test/SemaCXX/windows-Wpadded-bitfield.cpp
index ee5a57124eca5..0b88b4c170617 100644
--- a/clang/test/SemaCXX/windows-Wpadded-bitfield.cpp
+++ b/clang/test/SemaCXX/windows-Wpadded-bitfield.cpp
@@ -1,4 +1,5 @@
// RUN: %clang_cc1 -triple x86_64-windows-msvc -fsyntax-only -verify -Wpadded
%s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -verify -Wpadded %s
struct __attribute__((ms_struct)) BitfieldStruct { // expected-warning
{{padding size of 'BitfieldStruct' with 3 bytes to alignment boundary}}
char c : 1;
@@ -24,9 +25,23 @@ struct __attribute__((ms_struct)) DifferentUnitSizeBitfield
{ // expected-warnin
char i; // expected-warning {{padding struct 'DifferentUnitSizeBitfield'
with 31 bits to align 'i'}}
};
+struct __attribute__((ms_struct)) Foo { // expected-warning {{padding size of
'Foo' with 63 bits to alignment boundary}}
+ long long x;
+ char a : 1;
+ long long b : 1; // expected-warning {{padding struct 'Foo' with 63 bits to
align 'b'}}
+};
+
+struct __attribute__((ms_struct)) SameUnitSizeMultiple { // expected-warning
{{padding size of 'SameUnitSizeMultiple' with 2 bits to alignment boundary}}
+ char c : 1;
+ char cc : 2;
+ char ccc : 3;
+};
+
int main() {
BitfieldStruct b;
SevenBitfieldStruct s;
SameUnitSizeBitfield su;
DifferentUnitSizeBitfield du;
+ Foo f;
+ SameUnitSizeMultiple susm;
}
``
https://github.com/llvm/llvm-project/pull/136062
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [CLANG][MS-STRUCT] bitfield padding warning presents padding to exact bit count (PR #136062)
https://github.com/theomagellan created
https://github.com/llvm/llvm-project/pull/136062
Aims to fix #131647.
>From 842f0fbed0043ad0aa1679d8a30bc13d64eb25cf Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Th=C3=A9o=20de=20Magalhaes?=
Date: Thu, 17 Apr 2025 01:45:12 +0200
Subject: [PATCH] fix(ms_struct): bitfield padding warning presents padding to
exact bit count
---
clang/lib/AST/RecordLayoutBuilder.cpp | 4 +++-
clang/test/SemaCXX/windows-Wpadded-bitfield.cpp | 15 +++
2 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/clang/lib/AST/RecordLayoutBuilder.cpp
b/clang/lib/AST/RecordLayoutBuilder.cpp
index ea353f88a8aec..ca08e186f4ff2 100644
--- a/clang/lib/AST/RecordLayoutBuilder.cpp
+++ b/clang/lib/AST/RecordLayoutBuilder.cpp
@@ -1538,6 +1538,7 @@ void ItaniumRecordLayoutBuilder::LayoutBitField(const
FieldDecl *D) {
uint64_t StorageUnitSize = FieldInfo.Width;
unsigned FieldAlign = FieldInfo.Align;
bool AlignIsRequired = FieldInfo.isAlignRequired();
+ unsigned char PaddingInLastUnit = 0;
// UnfilledBitsInLastUnit is the difference between the end of the
// last allocated bitfield (i.e. the first bit offset available for
@@ -1610,6 +1611,7 @@ void ItaniumRecordLayoutBuilder::LayoutBitField(const
FieldDecl *D) {
if (!LastBitfieldStorageUnitSize && !FieldSize)
FieldAlign = 1;
+ PaddingInLastUnit = UnfilledBitsInLastUnit;
UnfilledBitsInLastUnit = 0;
LastBitfieldStorageUnitSize = 0;
}
@@ -1706,7 +1708,7 @@ void ItaniumRecordLayoutBuilder::LayoutBitField(const
FieldDecl *D) {
// For purposes of diagnostics, we're going to simultaneously
// compute the field offsets that we would have used if we weren't
// adding any alignment padding or if the field weren't packed.
- uint64_t UnpaddedFieldOffset = FieldOffset;
+ uint64_t UnpaddedFieldOffset = FieldOffset - PaddingInLastUnit;
uint64_t UnpackedFieldOffset = FieldOffset;
// Check if we need to add padding to fit the bitfield within an
diff --git a/clang/test/SemaCXX/windows-Wpadded-bitfield.cpp
b/clang/test/SemaCXX/windows-Wpadded-bitfield.cpp
index ee5a57124eca5..0b88b4c170617 100644
--- a/clang/test/SemaCXX/windows-Wpadded-bitfield.cpp
+++ b/clang/test/SemaCXX/windows-Wpadded-bitfield.cpp
@@ -1,4 +1,5 @@
// RUN: %clang_cc1 -triple x86_64-windows-msvc -fsyntax-only -verify -Wpadded
%s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -verify -Wpadded %s
struct __attribute__((ms_struct)) BitfieldStruct { // expected-warning
{{padding size of 'BitfieldStruct' with 3 bytes to alignment boundary}}
char c : 1;
@@ -24,9 +25,23 @@ struct __attribute__((ms_struct)) DifferentUnitSizeBitfield
{ // expected-warnin
char i; // expected-warning {{padding struct 'DifferentUnitSizeBitfield'
with 31 bits to align 'i'}}
};
+struct __attribute__((ms_struct)) Foo { // expected-warning {{padding size of
'Foo' with 63 bits to alignment boundary}}
+ long long x;
+ char a : 1;
+ long long b : 1; // expected-warning {{padding struct 'Foo' with 63 bits to
align 'b'}}
+};
+
+struct __attribute__((ms_struct)) SameUnitSizeMultiple { // expected-warning
{{padding size of 'SameUnitSizeMultiple' with 2 bits to alignment boundary}}
+ char c : 1;
+ char cc : 2;
+ char ccc : 3;
+};
+
int main() {
BitfieldStruct b;
SevenBitfieldStruct s;
SameUnitSizeBitfield su;
DifferentUnitSizeBitfield du;
+ Foo f;
+ SameUnitSizeMultiple susm;
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
