* [PATCH] testsuite: analyzer: expect alignment warning with -fshort-enums @ 2023-11-19 7:36 Alexandre Oliva 2023-11-19 15:13 ` Jeff Law ` (3 more replies) 0 siblings, 4 replies; 14+ messages in thread From: Alexandre Oliva @ 2023-11-19 7:36 UTC (permalink / raw) To: gcc-patches Cc: David Malcolm, Rainer Orth, Mike Stump, Jason Merrill, Nathan Sidwell 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] testsuite: analyzer: expect alignment warning with -fshort-enums 2023-11-19 7:36 [PATCH] testsuite: analyzer: expect alignment warning with -fshort-enums Alexandre Oliva @ 2023-11-19 15:13 ` Jeff Law 2023-11-20 22:19 ` David Malcolm 2023-11-20 2:33 ` [PATCH #2/4] c++: mark short-enums as packed Alexandre Oliva ` (2 subsequent siblings) 3 siblings, 1 reply; 14+ messages in thread From: Jeff Law @ 2023-11-19 15:13 UTC (permalink / raw) To: Alexandre Oliva, gcc-patches Cc: David Malcolm, Rainer Orth, Mike Stump, Jason Merrill, Nathan Sidwell 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] testsuite: analyzer: expect alignment warning with -fshort-enums 2023-11-19 15:13 ` Jeff Law @ 2023-11-20 22:19 ` David Malcolm 0 siblings, 0 replies; 14+ messages in thread From: David Malcolm @ 2023-11-20 22:19 UTC (permalink / raw) To: Jeff Law, Alexandre Oliva, gcc-patches Cc: Rainer Orth, Mike Stump, Jason Merrill, Nathan Sidwell 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH #2/4] c++: mark short-enums as packed 2023-11-19 7:36 [PATCH] testsuite: analyzer: expect alignment warning with -fshort-enums Alexandre Oliva 2023-11-19 15:13 ` Jeff Law @ 2023-11-20 2:33 ` Alexandre Oliva 2023-11-20 19:55 ` Jason Merrill 2023-11-20 2:34 ` [PATCH #3/4] warn on cast of pointer to packed plus constant Alexandre Oliva 2023-11-20 2:34 ` [PATCH #4/4] testsuite: discard c++ exclusion on underaligned pointer warning Alexandre Oliva 3 siblings, 1 reply; 14+ messages in thread From: Alexandre Oliva @ 2023-11-20 2:33 UTC (permalink / raw) To: gcc-patches Cc: David Malcolm, Rainer Orth, Mike Stump, Jason Merrill, Nathan Sidwell 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH #2/4] c++: mark short-enums as packed 2023-11-20 2:33 ` [PATCH #2/4] c++: mark short-enums as packed Alexandre Oliva @ 2023-11-20 19:55 ` Jason Merrill 2023-11-22 8:17 ` Alexandre Oliva 0 siblings, 1 reply; 14+ messages in thread From: Jason Merrill @ 2023-11-20 19:55 UTC (permalink / raw) To: Alexandre Oliva, gcc-patches Cc: David Malcolm, Rainer Orth, Mike Stump, Nathan Sidwell, H.J. Lu 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH #2/4] c++: mark short-enums as packed 2023-11-20 19:55 ` Jason Merrill @ 2023-11-22 8:17 ` Alexandre Oliva 2023-11-22 18:12 ` Jason Merrill 0 siblings, 1 reply; 14+ messages in thread From: Alexandre Oliva @ 2023-11-22 8:17 UTC (permalink / raw) To: Jason Merrill Cc: gcc-patches, David Malcolm, Rainer Orth, Mike Stump, Nathan Sidwell, H.J. Lu 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH #2/4] c++: mark short-enums as packed 2023-11-22 8:17 ` Alexandre Oliva @ 2023-11-22 18:12 ` Jason Merrill 2023-11-22 18:26 ` Jason Merrill 0 siblings, 1 reply; 14+ messages in thread From: Jason Merrill @ 2023-11-22 18:12 UTC (permalink / raw) To: Alexandre Oliva Cc: gcc-patches, David Malcolm, Rainer Orth, Mike Stump, Nathan Sidwell, H.J. Lu 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH #2/4] c++: mark short-enums as packed 2023-11-22 18:12 ` Jason Merrill @ 2023-11-22 18:26 ` Jason Merrill 2023-11-29 9:39 ` Alexandre Oliva 0 siblings, 1 reply; 14+ messages in thread From: Jason Merrill @ 2023-11-22 18:26 UTC (permalink / raw) To: Alexandre Oliva Cc: gcc-patches, David Malcolm, Rainer Orth, Mike Stump, Nathan Sidwell, H.J. Lu 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH #2/4] c++: mark short-enums as packed 2023-11-22 18:26 ` Jason Merrill @ 2023-11-29 9:39 ` Alexandre Oliva 2023-11-29 19:03 ` Jason Merrill 0 siblings, 1 reply; 14+ messages in thread From: Alexandre Oliva @ 2023-11-29 9:39 UTC (permalink / raw) To: Jason Merrill Cc: gcc-patches, David Malcolm, Rainer Orth, Mike Stump, Nathan Sidwell, H.J. Lu 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH #2/4] c++: mark short-enums as packed 2023-11-29 9:39 ` Alexandre Oliva @ 2023-11-29 19:03 ` Jason Merrill 2023-11-30 7:21 ` Alexandre Oliva 0 siblings, 1 reply; 14+ messages in thread From: Jason Merrill @ 2023-11-29 19:03 UTC (permalink / raw) To: Alexandre Oliva Cc: gcc-patches, David Malcolm, Rainer Orth, Mike Stump, Nathan Sidwell, H.J. Lu 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH #2/4] c++: mark short-enums as packed 2023-11-29 19:03 ` Jason Merrill @ 2023-11-30 7:21 ` Alexandre Oliva 0 siblings, 0 replies; 14+ messages in thread From: Alexandre Oliva @ 2023-11-30 7:21 UTC (permalink / raw) To: Jason Merrill Cc: gcc-patches, David Malcolm, Rainer Orth, Mike Stump, Nathan Sidwell, H.J. Lu 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH #3/4] warn on cast of pointer to packed plus constant 2023-11-19 7:36 [PATCH] testsuite: analyzer: expect alignment warning with -fshort-enums Alexandre Oliva 2023-11-19 15:13 ` Jeff Law 2023-11-20 2:33 ` [PATCH #2/4] c++: mark short-enums as packed Alexandre Oliva @ 2023-11-20 2:34 ` Alexandre Oliva 2023-11-20 2:34 ` [PATCH #4/4] testsuite: discard c++ exclusion on underaligned pointer warning Alexandre Oliva 3 siblings, 0 replies; 14+ messages in thread From: Alexandre Oliva @ 2023-11-20 2:34 UTC (permalink / raw) To: gcc-patches Cc: David Malcolm, Rainer Orth, Mike Stump, Jason Merrill, Nathan Sidwell 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH #4/4] testsuite: discard c++ exclusion on underaligned pointer warning 2023-11-19 7:36 [PATCH] testsuite: analyzer: expect alignment warning with -fshort-enums Alexandre Oliva ` (2 preceding siblings ...) 2023-11-20 2:34 ` [PATCH #3/4] warn on cast of pointer to packed plus constant Alexandre Oliva @ 2023-11-20 2:34 ` Alexandre Oliva 2023-11-23 20:27 ` Mike Stump 3 siblings, 1 reply; 14+ messages in thread From: Alexandre Oliva @ 2023-11-20 2:34 UTC (permalink / raw) To: gcc-patches Cc: David Malcolm, Rainer Orth, Mike Stump, Jason Merrill, Nathan Sidwell 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH #4/4] testsuite: discard c++ exclusion on underaligned pointer warning 2023-11-20 2:34 ` [PATCH #4/4] testsuite: discard c++ exclusion on underaligned pointer warning Alexandre Oliva @ 2023-11-23 20:27 ` Mike Stump 0 siblings, 0 replies; 14+ messages in thread From: Mike Stump @ 2023-11-23 20:27 UTC (permalink / raw) To: Alexandre Oliva Cc: gcc-patches, David Malcolm, Rainer Orth, Jason Merrill, Nathan Sidwell 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2023-11-30 7:21 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-11-19 7:36 [PATCH] testsuite: analyzer: expect alignment warning with -fshort-enums Alexandre Oliva 2023-11-19 15:13 ` Jeff Law 2023-11-20 22:19 ` David Malcolm 2023-11-20 2:33 ` [PATCH #2/4] c++: mark short-enums as packed Alexandre Oliva 2023-11-20 19:55 ` Jason Merrill 2023-11-22 8:17 ` Alexandre Oliva 2023-11-22 18:12 ` Jason Merrill 2023-11-22 18:26 ` Jason Merrill 2023-11-29 9:39 ` Alexandre Oliva 2023-11-29 19:03 ` Jason Merrill 2023-11-30 7:21 ` Alexandre Oliva 2023-11-20 2:34 ` [PATCH #3/4] warn on cast of pointer to packed plus constant Alexandre Oliva 2023-11-20 2:34 ` [PATCH #4/4] testsuite: discard c++ exclusion on underaligned pointer warning Alexandre Oliva 2023-11-23 20:27 ` Mike Stump
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).