[PATCH] testsuite: analyzer: expect alignment warning with -fshort-enums

[PATCH] testsuite: analyzer: expect alignment warning with -fshort-enums

[Alexandre Oliva]

On targets that have -fshort-enums enabled by default, the type casts
in the pr108251 analyzer tests warn that the byte-aligned enums may
not be sufficiently aligned to be a struct connection *.  The function
can't know better, the warning is reasonable, the code doesn't
expected enums to be shorter and less aligned than the struct.

Rather than use -fno-short-enums, I decided to embrace the warning on
targets that have short_enums enabled by default.

However, C++ doesn't issue the warning, because even with
-fshort-enums, enumeration types are not TYPE_PACKED, and the
expression is not sufficiently simplified by the C++ front-end for
check_and_warn_address_or_pointer_of_packed_member to identify the
insufficiently aligned pointer.  So don't expect the warning there.

(I've got followup patches in testing to get the same warnings in C++)

Regstrapped on x86_64-linux-gnu, also tested on arm-eabi with default
cpu on trunk, and with tms570 on gcc-13.  Ok to install?


for  gcc/testsuite/ChangeLog

	* c-c++-common/analyzer/null-deref-pr108251-smp_fetch_ssl_fc_has_early-O2.c:
	Expect "unaligned pointer value" warning on short_enums
	targets, but not in c++.
	* c-c++-common/analyzer/null-deref-pr108251-smp_fetch_ssl_fc_has_early.c:
	Likewise.
---
 ...-deref-pr108251-smp_fetch_ssl_fc_has_early-O2.c |    2 +-
 ...ull-deref-pr108251-smp_fetch_ssl_fc_has_early.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/gcc/testsuite/c-c++-common/analyzer/null-deref-pr108251-smp_fetch_ssl_fc_has_early-O2.c b/gcc/testsuite/c-c++-common/analyzer/null-deref-pr108251-smp_fetch_ssl_fc_has_early-O2.c
index c46ffe91a6b46..aaa2031b6dca4 100644
--- a/gcc/testsuite/c-c++-common/analyzer/null-deref-pr108251-smp_fetch_ssl_fc_has_early-O2.c
+++ b/gcc/testsuite/c-c++-common/analyzer/null-deref-pr108251-smp_fetch_ssl_fc_has_early-O2.c
@@ -61,7 +61,7 @@ static inline enum obj_type obj_type(const enum obj_type *t)
 }
 static inline struct connection *__objt_conn(enum obj_type *t)
 {
- return ((struct connection *)(((char *)(t)) - ((long)&((struct connection *)0)->obj_type)));
+ return ((struct connection *)(((char *)(t)) - ((long)&((struct connection *)0)->obj_type))); /* { dg-warning "unaligned pointer value" "warning" { target { short_enums && { ! c++ } } } } */
 }
 static inline struct connection *objt_conn(enum obj_type *t)
 {
diff --git a/gcc/testsuite/c-c++-common/analyzer/null-deref-pr108251-smp_fetch_ssl_fc_has_early.c b/gcc/testsuite/c-c++-common/analyzer/null-deref-pr108251-smp_fetch_ssl_fc_has_early.c
index ef34a76c50d63..6c96f5a76ef1c 100644
--- a/gcc/testsuite/c-c++-common/analyzer/null-deref-pr108251-smp_fetch_ssl_fc_has_early.c
+++ b/gcc/testsuite/c-c++-common/analyzer/null-deref-pr108251-smp_fetch_ssl_fc_has_early.c
@@ -60,7 +60,7 @@ static inline enum obj_type obj_type(const enum obj_type *t)
 }
 static inline struct connection *__objt_conn(enum obj_type *t)
 {
- return ((struct connection *)(((char *)(t)) - ((long)&((struct connection *)0)->obj_type)));
+ return ((struct connection *)(((char *)(t)) - ((long)&((struct connection *)0)->obj_type))); /* { dg-warning "unaligned pointer value" "warning" { target { short_enums && { ! c++ } } } } */
 }
 static inline struct connection *objt_conn(enum obj_type *t)
 {


-- 
Alexandre Oliva, happy hacker            https://FSFLA.org/blogs/lxo/
   Free Software Activist                   GNU Toolchain Engineer
More tolerance and less prejudice are key for inclusion and diversity
Excluding neuro-others for not behaving ""normal"" is *not* inclusive

Re: [PATCH] testsuite: analyzer: expect alignment warning with -fshort-enums

[Jeff Law]

On 11/19/23 00:36, Alexandre Oliva wrote:
> 
> On targets that have -fshort-enums enabled by default, the type casts
> in the pr108251 analyzer tests warn that the byte-aligned enums may
> not be sufficiently aligned to be a struct connection *.  The function
> can't know better, the warning is reasonable, the code doesn't
> expected enums to be shorter and less aligned than the struct.
> 
> Rather than use -fno-short-enums, I decided to embrace the warning on
> targets that have short_enums enabled by default.
> 
> However, C++ doesn't issue the warning, because even with
> -fshort-enums, enumeration types are not TYPE_PACKED, and the
> expression is not sufficiently simplified by the C++ front-end for
> check_and_warn_address_or_pointer_of_packed_member to identify the
> insufficiently aligned pointer.  So don't expect the warning there.
> 
> (I've got followup patches in testing to get the same warnings in C++)
> 
> Regstrapped on x86_64-linux-gnu, also tested on arm-eabi with default
> cpu on trunk, and with tms570 on gcc-13.  Ok to install?
> 
> 
> for  gcc/testsuite/ChangeLog
> 
> 	* c-c++-common/analyzer/null-deref-pr108251-smp_fetch_ssl_fc_has_early-O2.c:
> 	Expect "unaligned pointer value" warning on short_enums
> 	targets, but not in c++.
> 	* c-c++-common/analyzer/null-deref-pr108251-smp_fetch_ssl_fc_has_early.c:
> 	Likewise.
OK.  Hell of a filename for a single test :-)

jeff

[PATCH #2/4] c++: mark short-enums as packed

[Alexandre Oliva]

Unlike C, C++ doesn't mark enums shortened by -fshort-enums as packed.
This makes for undesirable warning differences between C and C++,
e.g. c-c++-common/analyzer/null-deref-pr108251-smp_fetch_ssl_fc_has_early*.c
triggers a warning about a type cast from a pointer to enum that, when
packed, might not be sufficiently aligned.

This change is not enough for that warning to trigger.  The tree
expression generated by the C++ front-end is also a little too
complicated for us get to the base pointer.  A separate patch takes
care of that.

Regstrapped on x86_64-linux-gnu, also tested on arm-eabi with default
cpu on trunk, and with tms570 on gcc-13.  Ok to install?


for  gcc/cp/ChangeLog

	* decl.cc (finish_enum_value_list): Set TYPE_PACKED if
	use_short_enum, and propagate it to variants.
---
 gcc/cp/decl.cc |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc
index 038c5ab71f201..f6d5645d5080f 100644
--- a/gcc/cp/decl.cc
+++ b/gcc/cp/decl.cc
@@ -16881,6 +16881,12 @@ finish_enum_value_list (tree enumtype)
       /* If -fstrict-enums, still constrain TYPE_MIN/MAX_VALUE.  */
       if (flag_strict_enums)
 	set_min_and_max_values_for_integral_type (enumtype, precision, sgn);
+
+      if (use_short_enum)
+	{
+	  TYPE_PACKED (enumtype) = use_short_enum;
+	  fixup_attribute_variants (enumtype);
+	}
     }
   else
     underlying_type = ENUM_UNDERLYING_TYPE (enumtype);


-- 
Alexandre Oliva, happy hacker            https://FSFLA.org/blogs/lxo/
   Free Software Activist                   GNU Toolchain Engineer
More tolerance and less prejudice are key for inclusion and diversity
Excluding neuro-others for not behaving ""normal"" is *not* inclusive

Re: [PATCH #3/4] warn on cast of pointer to packed plus constant

[Alexandre Oliva]

c-c++-common/analyzer/null-deref-pr108251-smp-fetch_ssl_fc_has_early.c
gets an unaligned pointer value warning on -fshort-enums targets in C,
but not in C++.  The former simplifies the offset-and-cast expression
enough that check_and_warn_address_or_pointer_of_packed_member finds
no more than a type cast of the base pointer, but in C++, the entire
expression, with cast, constant offsetting, and cast again, is
retained, and that's too much for the warning code.

Or rather it was.  It's easy enough to take the base pointer from
PLUS_POINTER_EXPR, and a constant offset can't possibly increase
alignment for just any pointer of laxer alignment, so we can safely
disregard the offset.

This should improve the warning even in C, if the packed enum is at a
nonzero offset into the containing struct.

Regstrapped on x86_64-linux-gnu, also tested on arm-eabi with default
cpu on trunk, and with tms570 on gcc-13.  Ok to install?


for  gcc/c-family/ChangeLog

	* c-warn.cc
	(check_and_warn_address_or_pointer_of_packed_member): Take the
	base pointer from PLUS_POINTER_EXPR when the addend is
	constant.
---
 gcc/c-family/c-warn.cc |   16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/gcc/c-family/c-warn.cc b/gcc/c-family/c-warn.cc
index d2938b91043d3..2ef73d7088f22 100644
--- a/gcc/c-family/c-warn.cc
+++ b/gcc/c-family/c-warn.cc
@@ -3108,10 +3108,20 @@ check_and_warn_address_or_pointer_of_packed_member (tree type, tree rhs)
 
   do
     {
-      while (TREE_CODE (rhs) == COMPOUND_EXPR)
-	rhs = TREE_OPERAND (rhs, 1);
-      orig_rhs = rhs;
+      do
+	{
+	  orig_rhs = rhs;
+	  while (TREE_CODE (rhs) == COMPOUND_EXPR)
+	    rhs = TREE_OPERAND (rhs, 1);
+	  /* Constants can't increase the alignment.  */
+	  while (TREE_CODE (rhs) == POINTER_PLUS_EXPR
+		 && TREE_CONSTANT (TREE_OPERAND (rhs, 1)))
+	    rhs = TREE_OPERAND (rhs, 0);
+	}
+      while (orig_rhs != rhs);
       STRIP_NOPS (rhs);
+      while (TREE_CODE (rhs) == VIEW_CONVERT_EXPR)
+	rhs = TREE_OPERAND (rhs, 0);
       nop_p |= orig_rhs != rhs;
     }
   while (orig_rhs != rhs);

-- 
Alexandre Oliva, happy hacker            https://FSFLA.org/blogs/lxo/
   Free Software Activist                   GNU Toolchain Engineer
More tolerance and less prejudice are key for inclusion and diversity
Excluding neuro-others for not behaving ""normal"" is *not* inclusive

[PATCH #4/4] testsuite: discard c++ exclusion on underaligned pointer warning

[Alexandre Oliva]

Having extended check_and_warn_address_or_pointer_of_packed_member to
find the packed (short) enum pointer in the cast expression coming
from the C++ front-end, and amended the C++ front end to mark short
enums as TYPE_PACKED, C++ issues the same warning that C does for
c-c++-common/analyzer/null-deref-pr108251-smp_fetch_ssl_fc_has_early.c,
so drop the exclusion.

Regstrapped on x86_64-linux-gnu, also tested on arm-eabi with default
cpu on trunk, and with tms570 on gcc-13.  Ok to install?


for  gcc/testsuite/ChangeLog

	* c-c++-common/analyzer/null-deref-pr108251-smp_fetch_ssl_fc_has_early-O2.c:
	Expect warning in C++ as well.
	* c-c++-common/analyzer/null-deref-pr108251-smp_fetch_ssl_fc_has_early.c:
	Likewise.
---
 ...-deref-pr108251-smp_fetch_ssl_fc_has_early-O2.c |    2 +-
 ...ull-deref-pr108251-smp_fetch_ssl_fc_has_early.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/gcc/testsuite/c-c++-common/analyzer/null-deref-pr108251-smp_fetch_ssl_fc_has_early-O2.c b/gcc/testsuite/c-c++-common/analyzer/null-deref-pr108251-smp_fetch_ssl_fc_has_early-O2.c
index aaa2031b6dca4..bf5bf5cc2e278 100644
--- a/gcc/testsuite/c-c++-common/analyzer/null-deref-pr108251-smp_fetch_ssl_fc_has_early-O2.c
+++ b/gcc/testsuite/c-c++-common/analyzer/null-deref-pr108251-smp_fetch_ssl_fc_has_early-O2.c
@@ -61,7 +61,7 @@ static inline enum obj_type obj_type(const enum obj_type *t)
 }
 static inline struct connection *__objt_conn(enum obj_type *t)
 {
- return ((struct connection *)(((char *)(t)) - ((long)&((struct connection *)0)->obj_type))); /* { dg-warning "unaligned pointer value" "warning" { target { short_enums && { ! c++ } } } } */
+ return ((struct connection *)(((char *)(t)) - ((long)&((struct connection *)0)->obj_type))); /* { dg-warning "unaligned pointer value" "warning" { target short_enums } } */
 }
 static inline struct connection *objt_conn(enum obj_type *t)
 {
diff --git a/gcc/testsuite/c-c++-common/analyzer/null-deref-pr108251-smp_fetch_ssl_fc_has_early.c b/gcc/testsuite/c-c++-common/analyzer/null-deref-pr108251-smp_fetch_ssl_fc_has_early.c
index 6c96f5a76ef1c..7c2710c64d35e 100644
--- a/gcc/testsuite/c-c++-common/analyzer/null-deref-pr108251-smp_fetch_ssl_fc_has_early.c
+++ b/gcc/testsuite/c-c++-common/analyzer/null-deref-pr108251-smp_fetch_ssl_fc_has_early.c
@@ -60,7 +60,7 @@ static inline enum obj_type obj_type(const enum obj_type *t)
 }
 static inline struct connection *__objt_conn(enum obj_type *t)
 {
- return ((struct connection *)(((char *)(t)) - ((long)&((struct connection *)0)->obj_type))); /* { dg-warning "unaligned pointer value" "warning" { target { short_enums && { ! c++ } } } } */
+ return ((struct connection *)(((char *)(t)) - ((long)&((struct connection *)0)->obj_type))); /* { dg-warning "unaligned pointer value" "warning" { target short_enums } } */
 }
 static inline struct connection *objt_conn(enum obj_type *t)
 {

-- 
Alexandre Oliva, happy hacker            https://FSFLA.org/blogs/lxo/
   Free Software Activist                   GNU Toolchain Engineer
More tolerance and less prejudice are key for inclusion and diversity
Excluding neuro-others for not behaving ""normal"" is *not* inclusive

Re: [PATCH #2/4] c++: mark short-enums as packed

[Jason Merrill]

On 11/19/23 21:33, Alexandre Oliva wrote:
> 
> Unlike C, C++ doesn't mark enums shortened by -fshort-enums as packed.
> This makes for undesirable warning differences between C and C++,
> e.g. c-c++-common/analyzer/null-deref-pr108251-smp_fetch_ssl_fc_has_early*.c
> triggers a warning about a type cast from a pointer to enum that, when
> packed, might not be sufficiently aligned.

I think the warning is wrong here.  The example given when it was added 
in r9-5005-gda77eace90fd99 was

struct pair_t
{
   char c;
   int i;
} __attribute__ ((packed));

extern struct pair_t p;
int *addr = &p.i;

in this case, we're assigning a pointer to unaligned int to a normal 
pointer to int, and that's what the warning is for; conversion from a 
misaligned member.  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.

Also, -fshort-enums has nothing to do with structure packing, it just 
affects the underlying type of the enum.  We don't warn about conversion 
from pointer to char to pointer to struct with char member, and this is 
the exact same situation.

And unlike a struct with __attribute__ ((packed)),
   enum A { a=1000 };
gets 16-bit alignment with -fshort-enums.

So it seems to me that setting TYPE_PACKED from -fshort-enums is wrong.
Or that the warning's use of TYPE_PACKED is wrong (e.g. it should look 
for a COMPONENT_REF of DECL_PACKED instead).  Or both.

Either way, I'm opposed to changing things so that the C++ front-end 
starts emitting the warning.

Jason

Re: [PATCH] testsuite: analyzer: expect alignment warning with -fshort-enums

[David Malcolm]

On Sun, 2023-11-19 at 08:13 -0700, Jeff Law wrote:
> 
> 
> On 11/19/23 00:36, Alexandre Oliva wrote:
> > 
> > On targets that have -fshort-enums enabled by default, the type
> > casts
> > in the pr108251 analyzer tests warn that the byte-aligned enums may
> > not be sufficiently aligned to be a struct connection *.  The
> > function
> > can't know better, the warning is reasonable, the code doesn't
> > expected enums to be shorter and less aligned than the struct.
> > 
> > Rather than use -fno-short-enums, I decided to embrace the warning
> > on
> > targets that have short_enums enabled by default.
> > 
> > However, C++ doesn't issue the warning, because even with
> > -fshort-enums, enumeration types are not TYPE_PACKED, and the
> > expression is not sufficiently simplified by the C++ front-end for
> > check_and_warn_address_or_pointer_of_packed_member to identify the
> > insufficiently aligned pointer.  So don't expect the warning there.
> > 
> > (I've got followup patches in testing to get the same warnings in
> > C++)
> > 
> > Regstrapped on x86_64-linux-gnu, also tested on arm-eabi with
> > default
> > cpu on trunk, and with tms570 on gcc-13.  Ok to install?
> > 
> > 
> > for  gcc/testsuite/ChangeLog
> > 
> >         * c-c++-common/analyzer/null-deref-pr108251-
> > smp_fetch_ssl_fc_has_early-O2.c:
> >         Expect "unaligned pointer value" warning on short_enums
> >         targets, but not in c++.
> >         * c-c++-common/analyzer/null-deref-pr108251-
> > smp_fetch_ssl_fc_has_early.c:
> >         Likewise.
> OK.  Hell of a filename for a single test :-)

Sorry about that, I have lots of test names of the form prNNNNNN and
wanted something more descriptive.  Clearly I overcorrected :)

Dave

Re: [PATCH #2/4] c++: mark short-enums as packed

[Alexandre Oliva]

On Nov 20, 2023, Jason Merrill <jason@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.

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?

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)

> 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?

> 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? :-)


Thanks,

-- 
Alexandre Oliva, happy hacker            https://FSFLA.org/blogs/lxo/
   Free Software Activist                   GNU Toolchain Engineer
More tolerance and less prejudice are key for inclusion and diversity
Excluding neuro-others for not behaving ""normal"" is *not* inclusive

Re: [PATCH #2/4] c++: mark short-enums as packed

[Jason Merrill]

On 11/22/23 03:17, Alexandre Oliva wrote:
> On Nov 20, 2023, Jason Merrill <jason@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.

Jason

Re: [PATCH #2/4] c++: mark short-enums as packed

[Jason Merrill]

On 11/22/23 13:12, Jason Merrill wrote:
> On 11/22/23 03:17, Alexandre Oliva wrote:
>> On Nov 20, 2023, Jason Merrill <jason@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

Re: [PATCH #4/4] testsuite: discard c++ exclusion on underaligned pointer warning

[Mike Stump]

On Nov 19, 2023, at 6:34 PM, Alexandre Oliva <oliva@adacore.com> wrote:
> 
> Having extended check_and_warn_address_or_pointer_of_packed_member to
> find the packed (short) enum pointer in the cast expression coming
> from the C++ front-end, and amended the C++ front end to mark short
> enums as TYPE_PACKED, C++ issues the same warning that C does for
> c-c++-common/analyzer/null-deref-pr108251-smp_fetch_ssl_fc_has_early.c,
> so drop the exclusion.
> 
> Regstrapped on x86_64-linux-gnu, also tested on arm-eabi with default
> cpu on trunk, and with tms570 on gcc-13.  Ok to install?

Ok.

Re: [PATCH #2/4] c++: mark short-enums as packed

[Alexandre Oliva]

Hello, Jason,

On Nov 22, 2023, Jason Merrill <jason@redhat.com> wrote:

> On 11/22/23 13:12, Jason Merrill wrote:
>> 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.

Thanks for that patch.  It makes sense to me, but I suppose that, if
it goes in, I should revert the already-installed #1/4 in this series
https://gcc.gnu.org/pipermail/gcc-patches/2023-November/637244.html
rather than install #4/4 that Mike approved.
https://gcc.gnu.org/pipermail/gcc-patches/2023-November/637336.html

I wasn't sure whether your earlier conclusion (quoted above) was meant
as an 'Ok' for the C++ patch.  Please confirm if so.  TIA,

-- 
Alexandre Oliva, happy hacker            https://FSFLA.org/blogs/lxo/
   Free Software Activist                   GNU Toolchain Engineer
More tolerance and less prejudice are key for inclusion and diversity
Excluding neuro-others for not behaving ""normal"" is *not* inclusive

Re: [PATCH #2/4] c++: mark short-enums as packed

[Jason Merrill]

On 11/29/23 04:39, Alexandre Oliva wrote:
> Hello, Jason,
> 
> On Nov 22, 2023, Jason Merrill <jason@redhat.com> wrote:
> 
>> On 11/22/23 13:12, Jason Merrill wrote:
>>> 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.
> 
> Thanks for that patch.  It makes sense to me, but I suppose that, if
> it goes in, I should revert the already-installed #1/4 in this series
> https://gcc.gnu.org/pipermail/gcc-patches/2023-November/637244.html
> rather than install #4/4 that Mike approved.
> https://gcc.gnu.org/pipermail/gcc-patches/2023-November/637336.html
> 
> I wasn't sure whether your earlier conclusion (quoted above) was meant
> as an 'Ok' for the C++ patch.  Please confirm if so.  TIA,

Yes.

Jason

Re: [PATCH #2/4] c++: mark short-enums as packed

[Alexandre Oliva]

On Nov 29, 2023, Jason Merrill <jason@redhat.com> wrote:

> On 11/29/23 04:39, Alexandre Oliva wrote:
>> Hello, Jason,
>> On Nov 22, 2023, Jason Merrill <jason@redhat.com> wrote:
>> 
>>> On 11/22/23 13:12, Jason Merrill wrote:
>>>> 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.
>> Thanks for that patch.  It makes sense to me, but I suppose that, if
>> it goes in, I should revert the already-installed #1/4 in this series
>> https://gcc.gnu.org/pipermail/gcc-patches/2023-November/637244.html
>> rather than install #4/4 that Mike approved.
>> https://gcc.gnu.org/pipermail/gcc-patches/2023-November/637336.html
>> I wasn't sure whether your earlier conclusion (quoted above) was
>> meant
>> as an 'Ok' for the C++ patch.  Please confirm if so.  TIA,

> Yes.

Thanks, the C++ patch is now in, and so is the testsuite patch reversal.
The pr108251 analyzer tests are again failing on -fshort-enum platforms,
in hope that your patchset is going to make it.

-- 
Alexandre Oliva, happy hacker            https://FSFLA.org/blogs/lxo/
   Free Software Activist                   GNU Toolchain Engineer
More tolerance and less prejudice are key for inclusion and diversity
Excluding neuro-others for not behaving ""normal"" is *not* inclusive