llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang
Author: Matt Turner (mattst88)
<details>
<summary>Changes</summary>
`-Waddress-of-packed-member` keys on the attribute spelling.
`Sema::RefersToMemberWithReducedAlignment` bails out unless some link of the
member chain carries a `PackedAttr`, and `#pragma pack` attaches a
`MaxFieldAlignmentAttr` instead. The alignment computation it bails out of
would have got this right; it is simply never reached:
```c
#pragma pack(1)
struct S { char c; int x; };
int *f(struct S *p) { return &p->x; } // not diagnosed
```
Recognize both attributes, in a helper now shared by the bailout and by the
later search for the field to blame, so the two cannot disagree and the
`"We did not find a packed FieldDecl!"` assertion still holds.
`__attribute__((packed))` reduces every field; `#pragma pack(N)` only reduces
fields that require more than N, so `#pragma pack(4)` over a record of `int`s
stays quiet.
Also report the least alignment left by any link of the chain rather than the
one left by the first link that reduced it, since an outer packed record can
reduce a field further than the `#pragma pack` on its own record did. That
alignment only feeds `DiscardMisalignedMemberAddress`, which drops the warning
when the destination pointer needs no more, so lowering it cannot lose a
warning that is issued today.
This traces back to #<!-- -->23195, which was closed on the premise that "clang
does
not accept `#pragma pack`" -- the reproducer there was rewritten to use the
attribute, so the fix only ever covered `PackedAttr`.
Approach suggested by @<!-- -->aaronpuchert in
https://github.com/llvm/llvm-project/issues/97091#issuecomment-5417768399.
### Why this is more than a missing diagnostic
The pointer-mediated access is emitted with the alignment of the member's
declared type rather than of the object, so on a target without unaligned
access the direct access is open-coded and the pointer access is not
(`clang --target=riscv32-none-elf -O2`):
```asm
viaptr: # align 2 -- one 16-bit store
lui a1, %hi(b1+4)
sh a0, %lo(b1+4)(a1)
ret
direct: # align 1 -- correctly split into byte stores
lui a1, %hi(b1)
addi a1, a1, %lo(b1)
srli a2, a0, 8
sb a0, 4(a1)
sb a2, 5(a1)
ret
```
Details and the full reproducer are in
https://github.com/llvm/llvm-project/issues/97091#issuecomment-5416535924.
### Known limitation, not addressed here
A field inherited from an unpacked base through a class defined under
`#pragma pack` stays undiagnosed. The pack does cap the alignment of the base
subobject, but the chain the check walks holds a link per member access and
none for the derived-to-base conversion. Recorded in
`clang/test/SemaCXX/address-packed.cpp` rather than fixed.
Fixes #<!-- -->97091.
---
Full diff: https://github.com/llvm/llvm-project/pull/219096.diff
4 Files Affected:
- (modified) clang/docs/ReleaseNotes.md (+3)
- (modified) clang/lib/Sema/SemaChecking.cpp (+30-9)
- (modified) clang/test/Sema/address-packed.c (+64)
- (modified) clang/test/SemaCXX/address-packed.cpp (+28)
``````````diff
diff --git a/clang/docs/ReleaseNotes.md b/clang/docs/ReleaseNotes.md
index 332e0bfdb3a8b..80e1ef4d28f67 100644
--- a/clang/docs/ReleaseNotes.md
+++ b/clang/docs/ReleaseNotes.md
@@ -418,6 +418,9 @@ features cannot lower the translation-unit ABI level;
- Clang now diagnoses more details when a constraint evaluates to false.
+- `-Waddress-of-packed-member` now also diagnoses members of records packed
+ by `#pragma pack`. (#GH97091)
+
### Improvements to Clang's time-trace
### Improvements to Coverage Mapping
diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index 7f3ccea82e8af..717476a779426 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -16983,6 +16983,26 @@ void Sema::DiscardMisalignedMemberAddress(const Type
*T, Expr *E) {
}
}
+/// If packing reduces \p FD below the alignment required by its type, return
+/// the alignment it is reduced to. __attribute__((packed)) reduces every
+/// field; #pragma pack(N) only reduces fields that require more than N.
+static std::optional<CharUnits> getPackedFieldAlignment(const ASTContext &Ctx,
+ const FieldDecl *FD) {
+ const RecordDecl *RD = FD->getParent();
+ bool IsPacked = FD->hasAttr<PackedAttr>() || RD->hasAttr<PackedAttr>();
+ const auto *MFAA = RD->getAttr<MaxFieldAlignmentAttr>();
+ if (!IsPacked && !MFAA)
+ return std::nullopt;
+
+ CharUnits TypeAlignment = Ctx.getTypeAlignInChars(FD->getType());
+ if (!IsPacked &&
+ Ctx.toCharUnitsFromBits(MFAA->getAlignment()) >= TypeAlignment)
+ return std::nullopt;
+
+ return std::min(TypeAlignment,
+ Ctx.getTypeAlignInChars(Ctx.getCanonicalTagType(RD)));
+}
+
void Sema::RefersToMemberWithReducedAlignment(
Expr *E,
llvm::function_ref<void(Expr *, RecordDecl *, FieldDecl *, CharUnits)>
@@ -17017,7 +17037,7 @@ void Sema::RefersToMemberWithReducedAlignment(
return;
AnyIsPacked =
- AnyIsPacked || (RD->hasAttr<PackedAttr>() ||
MD->hasAttr<PackedAttr>());
+ AnyIsPacked || getPackedFieldAlignment(Context, FD).has_value();
ReverseMemberChain.push_back(FD);
TopME = ME;
@@ -17076,16 +17096,17 @@ void Sema::RefersToMemberWithReducedAlignment(
// FieldDecl that either is packed or else its RecordDecl is,
// seems reasonable.
FieldDecl *FD = nullptr;
- CharUnits Alignment;
+ // Take the least alignment left by any link of the chain, not the one
+ // left by the first link that reduced it: an outer packed record can
+ // reduce a field further than the #pragma pack on its own record did.
+ CharUnits Alignment = ExpectedAlignment;
for (FieldDecl *FDI : ReverseMemberChain) {
- if (FDI->hasAttr<PackedAttr>() ||
- FDI->getParent()->hasAttr<PackedAttr>()) {
+ std::optional<CharUnits> Packed = getPackedFieldAlignment(Context, FDI);
+ if (!FD && Packed)
FD = FDI;
- Alignment = std::min(Context.getTypeAlignInChars(FD->getType()),
- Context.getTypeAlignInChars(
-
Context.getCanonicalTagType(FD->getParent())));
- break;
- }
+ Alignment = std::min(
+ Alignment,
+ Packed.value_or(Context.getTypeAlignInChars(FDI->getType())));
}
assert(FD && "We did not find a packed FieldDecl!");
Action(E, FD->getParent(), FD, Alignment);
diff --git a/clang/test/Sema/address-packed.c b/clang/test/Sema/address-packed.c
index f826b7d57d91c..11966e74c73d8 100644
--- a/clang/test/Sema/address-packed.c
+++ b/clang/test/Sema/address-packed.c
@@ -346,3 +346,67 @@ void g15(void) {
to_void_with_expr(&arguable.x, 3); // no-warning
to_void_with_expr(&arguable.x, ({3;})); // no-warning
}
+
+#pragma pack(push, 1)
+struct PragmaPack1 {
+ char c0;
+ int x;
+};
+#pragma pack(pop)
+
+void g16(struct PragmaPack1 *p) {
+ f1(&p->x); // expected-warning {{packed member 'x' of class or structure
'PragmaPack1'}}
+ f2(&p->c0); // no-warning
+}
+
+// #pragma pack(4) does not reduce an int.
+#pragma pack(push, 4)
+struct PragmaPack4 {
+ char c0;
+ int x;
+ char c1;
+};
+#pragma pack(pop)
+
+void g17(struct PragmaPack4 *p) {
+ f1(&p->x); // no-warning
+}
+
+// The record is two-aligned, so x is misaligned despite an offset of four.
+#pragma pack(push, 2)
+struct PragmaPack2 {
+ char c0;
+ char c1;
+ int x;
+};
+#pragma pack(pop)
+
+void g18(struct PragmaPack2 *p) {
+ f1(&p->x); // expected-warning {{packed member 'x' of class or structure
'PragmaPack2'}}
+}
+
+struct PragmaPackOuter {
+ char c0;
+ struct PragmaPack1 inner;
+};
+
+void g19(struct PragmaPackOuter *p) {
+ f1(&p->inner.x); // expected-warning {{packed member 'x' of class or
structure 'PragmaPack1'}}
+}
+
+extern void f3(short *);
+
+// short is two-aligned and so is PragmaPack2.
+void g20(struct PragmaPack2 *p) {
+ f3((short *)&p->x); // no-warning
+}
+
+// The outer packed record lowers it to one.
+struct __attribute__((packed)) PragmaPackInPacked {
+ char c0;
+ struct PragmaPack2 inner;
+};
+
+void g21(struct PragmaPackInPacked *p) {
+ f3((short *)&p->inner.x); // expected-warning {{packed member 'x' of class
or structure 'PragmaPack2'}}
+}
diff --git a/clang/test/SemaCXX/address-packed.cpp
b/clang/test/SemaCXX/address-packed.cpp
index f0d1496fd8928..f020f0b1355e9 100644
--- a/clang/test/SemaCXX/address-packed.cpp
+++ b/clang/test/SemaCXX/address-packed.cpp
@@ -121,3 +121,31 @@ struct S2 {
Incomplete *e() const;
} __attribute__((packed));
Incomplete *S2::e() const { return (Incomplete *)&d; } // no-warning
+
+#pragma pack(push, 1)
+struct PragmaPacked {
+ char c;
+ int x;
+ int *get() { return &x; } // expected-warning {{packed member 'x' of class
or structure 'PragmaPacked'}}
+};
+#pragma pack(pop)
+
+void g2(PragmaPacked *p) {
+ f1(&p->x); // expected-warning {{packed member 'x' of class or structure
'PragmaPacked'}}
+ f2(&p->c); // no-warning
+}
+
+// #pragma pack caps the base subobject too, so alignof(DerivedFromUnpacked)
+// is one and &p->x really is a one-aligned int *. Not diagnosed: the chain
+// has a link per member access and none for the derived-to-base conversion.
+struct Unpacked {
+ char c;
+ int x;
+};
+#pragma pack(push, 1)
+struct DerivedFromUnpacked : Unpacked {};
+#pragma pack(pop)
+
+void g3(DerivedFromUnpacked *p) {
+ f1(&p->x); // no-warning
+}
``````````
</details>
https://github.com/llvm/llvm-project/pull/219096
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
