On 11/22/23 13:12, Jason Merrill wrote:
On 11/22/23 03:17, Alexandre Oliva wrote:
On Nov 20, 2023, Jason Merrill <ja...@redhat.com> wrote:
I think the warning is wrong here.
Interesting... Yeah, your analysis makes perfect sense.
Still, we're left with a divergence WRT the TYPE_PACKED status of enum
types between C and C++.
It sort of kind of makes sense to mark short enums as packed, because,
well, they are.
The enum is conceptually packed into a smaller integer type, sure.
Even enum types with explicit attribute packed, that IIUC uses the same
underlying type selection as -fshort-enums, IIRC are not be marked with
TYPE_PACKED in C++, at least not at the place where I proposed to set
it. Do you consider that behavior correct?
Since attribute ((packed)) has this meaning, it seems reasonable to set
TYPE_PACKED to express it.
Even if the warning happens to be buggy in this regard, it is at best
(or worst) accessory to this patch, in that it makes that difference
between languages apparent, and I worry that there might be other middle
end tests involving TYPE_PACKED that would get things different in C vs
C++. (admittedly, I haven't searched for occurrences of TYPE_PACKED in
the tree, but I could, to alleviate my concerns, in case there's a
decision to keep them different)
The middle-end doesn't seem to use TYPE_PACKED for anything other than
structure layout.
In the analyzer testcase, we have a cast from an
enum pointer that we don't know what it points to, and even if it did
point to the obj_type member of struct connection, that wouldn't be a
problem because it's at offset 0.
Maybe I misunderstand the point of the warning, but ISTM that the
circumstance it's warning about is real: the member is not as aligned as
the enclosing struct, so the cast is risky. Now, I suppose the idiom of
finding the enclosing struct given a member is common enough that we
don't want to warn about it in general. I'm not sure what makes packed
structs special in this regard, though. I don't really see much
difference, more laxly-aligned fields seem equally warn-worthy, whether
the enclosing struct is packed or not, but what do I know?
Exactly. If we want to warn about casting from pointer to less-aligned
type to pointer to more-aligned type, that's already
-Wcast-align=strict; whether the lower alignment is due to TYPE_PACKED
seems irrelevant.
The observation that the type-based warning is a subset of
-Wcast-align=strict was previously made in the discussion of the patch
for PR88928.
And the motivating testcase for the warning was about converting from
unaligned int* to aligned int*, not to a different type at all. And
that warning doesn't involve TYPE_PACKED.
The clang -Waddress-of-packed-member doesn't seem to include the
type-based warning.
Also, -fshort-enums has nothing to do with structure packing
*nod*, it's about packing of the enum type itself. It is some sort of a
degenerated aggregate type ;-) But yeah, I guess it doesn't fit the
circumstance the warning was meant to catch, and the fact that in C is
does is a consequence of marking C short enums as TYPE_PACKED.
Which might be a bug in C.
But wouldn't it be a bug in C++ if an enum with attribute packed weren't
markd as TYPE_PACKED? Or is TYPE_PACKED really meant to say something
about the enclosing struct rather than about the enclosed type itself?
(am I getting too philosophical here? :-)
I'm coming to the conclusion that your C++ patch is fine but we should
remove the TYPE_PACKED warning from
check_address_or_pointer_of_packed_member. And maybe add
-Wcast-align=strict to -Wextra.
Since I seem to have opinions, I'm preparing a patch for this.
Jason
