Re: V6 [PATCH] C/C++: Add -Waddress-of-packed-member
On 12/17/18 7:42 AM, H.J. Lu wrote: On Mon, Dec 17, 2018 at 1:39 AM Richard Biener wrote: On Fri, Dec 14, 2018 at 11:48 PM H.J. Lu wrote: On Fri, Dec 14, 2018 at 2:10 PM Jason Merrill wrote: On 12/13/18 6:56 PM, H.J. Lu wrote: On Thu, Dec 13, 2018 at 12:50 PM Jason Merrill wrote: On 9/25/18 11:46 AM, H.J. Lu wrote: On Fri, Aug 31, 2018 at 2:04 PM, Jason Merrill wrote: On 07/23/2018 05:24 PM, H.J. Lu wrote: On Mon, Jun 18, 2018 at 12:26 PM, Joseph Myers wrote: On Mon, 18 Jun 2018, Jason Merrill wrote: On Mon, Jun 18, 2018 at 11:59 AM, Joseph Myers wrote: On Mon, 18 Jun 2018, Jason Merrill wrote: + if (TREE_CODE (rhs) == COND_EXPR) +{ + /* Check the THEN path first. */ + tree op1 = TREE_OPERAND (rhs, 1); + context = check_address_of_packed_member (type, op1); This should handle the GNU extension of re-using operand 0 if operand 1 is omitted. Doesn't that just use a SAVE_EXPR? Hmm, I suppose it does, but many places in the compiler seem to expect that it produces a COND_EXPR with TREE_OPERAND 1 as NULL_TREE. Maybe that's used somewhere inside the C++ front end. For C a SAVE_EXPR is produced directly. Here is the updated patch. Changes from the last one: 1. Handle COMPOUND_EXPR. 2. Fixed typos in comments. 3. Combined warn_for_pointer_of_packed_member and warn_for_address_of_packed_member into warn_for_address_or_pointer_of_packed_member. c.i:4:33: warning: converting a packed ‘struct C *’ pointer increases the alignment of ‘long int *’ pointer from 1 to 8 [-Waddress-of-packed-member] I think this would read better as c.i:4:33: warning: converting a packed ‘struct C *’ pointer (alignment 1) to ‘long int *’ (alignment 8) may result in an unaligned pointer value [-Waddress-of-packed-member] Fixed. + while (TREE_CODE (base) == ARRAY_REF) + base = TREE_OPERAND (base, 0); + if (TREE_CODE (base) != COMPONENT_REF) + return NULL_TREE; Are you deliberately not handling the other handled_component_p cases? If so, there should be a comment. I changed it to while (handled_component_p (base)) { enum tree_code code = TREE_CODE (base); if (code == COMPONENT_REF) break; switch (code) { case ARRAY_REF: base = TREE_OPERAND (base, 0); break; default: /* FIXME: Can it ever happen? */ gcc_unreachable (); break; } } Is there a testcase to trigger this ICE? I couldn't find one. You can take the address of an element of complex: __complex int i; int *p = &__real(i); You may get VIEW_CONVERT_EXPR with location wrappers. Fixed. I replaced gcc_unreachable with return NULL_TREE; Then we're back to my earlier question: are you deliberately not handling the other cases? Why not look through them as well? What if e.g. the operand of __real is a packed field? Here is the updated patch with diff --git a/gcc/c-family/c-warn.c b/gcc/c-family/c-warn.c index 615134cfdac..f105742598e 100644 --- a/gcc/c-family/c-warn.c +++ b/gcc/c-family/c-warn.c @@ -2669,6 +2669,9 @@ check_address_of_packed_member (tree type, tree rhs) switch (code) { case ARRAY_REF: + case REALPART_EXPR: + case IMAGPART_EXPR: + case VIEW_CONVERT_EXPR: base = TREE_OPERAND (base, 0); break; default: don't we have handled_component_p () for this? (you're still missing BIT_FIELD_REF which might be used for vector element accesses) Do you have a testcase? Is there a reason you only want to handle some component references and not others? If not, checking handled_component_p is simpler and more future proof than enumerating specific codes. Jason
Re: V6 [PATCH] C/C++: Add -Waddress-of-packed-member
On Mon, Dec 17, 2018 at 1:43 PM H.J. Lu wrote: > > On Mon, Dec 17, 2018 at 1:39 AM Richard Biener > wrote: > > > > On Fri, Dec 14, 2018 at 11:48 PM H.J. Lu wrote: > > > > > > On Fri, Dec 14, 2018 at 2:10 PM Jason Merrill wrote: > > > > > > > > On 12/13/18 6:56 PM, H.J. Lu wrote: > > > > > On Thu, Dec 13, 2018 at 12:50 PM Jason Merrill > > > > > wrote: > > > > >> > > > > >> On 9/25/18 11:46 AM, H.J. Lu wrote: > > > > >>> On Fri, Aug 31, 2018 at 2:04 PM, Jason Merrill > > > > >>> wrote: > > > > On 07/23/2018 05:24 PM, H.J. Lu wrote: > > > > > > > > > > On Mon, Jun 18, 2018 at 12:26 PM, Joseph Myers > > > > > > > > > > wrote: > > > > >> > > > > >> On Mon, 18 Jun 2018, Jason Merrill wrote: > > > > >> > > > > >>> On Mon, Jun 18, 2018 at 11:59 AM, Joseph Myers > > > > >>> > > > > >>> wrote: > > > > > > > > On Mon, 18 Jun 2018, Jason Merrill wrote: > > > > > > > > >> + if (TREE_CODE (rhs) == COND_EXPR) > > > > >> +{ > > > > >> + /* Check the THEN path first. */ > > > > >> + tree op1 = TREE_OPERAND (rhs, 1); > > > > >> + context = check_address_of_packed_member (type, op1); > > > > > > > > > > > > > > > This should handle the GNU extension of re-using operand 0 if > > > > > operand > > > > > 1 is omitted. > > > > > > > > > > > > Doesn't that just use a SAVE_EXPR? > > > > >>> > > > > >>> > > > > >>> Hmm, I suppose it does, but many places in the compiler seem to > > > > >>> expect > > > > >>> that it produces a COND_EXPR with TREE_OPERAND 1 as NULL_TREE. > > > > >> > > > > >> > > > > >> Maybe that's used somewhere inside the C++ front end. For C a > > > > >> SAVE_EXPR > > > > >> is produced directly. > > > > > > > > > > > > > > > Here is the updated patch. Changes from the last one: > > > > > > > > > > 1. Handle COMPOUND_EXPR. > > > > > 2. Fixed typos in comments. > > > > > 3. Combined warn_for_pointer_of_packed_member and > > > > > warn_for_address_of_packed_member into > > > > > warn_for_address_or_pointer_of_packed_member. > > > > > > > > > > > > > c.i:4:33: warning: converting a packed ‘struct C *’ pointer > > > > > increases the > > > > > alignment of ‘long int *’ pointer from 1 to 8 > > > > > [-Waddress-of-packed-member] > > > > > > > > > > > > I think this would read better as > > > > > > > > c.i:4:33: warning: converting a packed ‘struct C *’ pointer > > > > (alignment 1) to > > > > ‘long int *’ (alignment 8) may result in an unaligned pointer value > > > > [-Waddress-of-packed-member] > > > > >>> > > > > >>> Fixed. > > > > >>> > > > > > + while (TREE_CODE (base) == ARRAY_REF) > > > > > + base = TREE_OPERAND (base, 0); > > > > > + if (TREE_CODE (base) != COMPONENT_REF) > > > > > + return NULL_TREE; > > > > > > > > > > > > Are you deliberately not handling the other handled_component_p > > > > cases? If > > > > so, there should be a comment. > > > > >>> > > > > >>> I changed it to > > > > >>> > > > > >>>while (handled_component_p (base)) > > > > >>> { > > > > >>> enum tree_code code = TREE_CODE (base); > > > > >>> if (code == COMPONENT_REF) > > > > >>> break; > > > > >>> switch (code) > > > > >>> { > > > > >>> case ARRAY_REF: > > > > >>> base = TREE_OPERAND (base, 0); > > > > >>> break; > > > > >>> default: > > > > >>> /* FIXME: Can it ever happen? */ > > > > >>> gcc_unreachable (); > > > > >>> break; > > > > >>> } > > > > >>> } > > > > >>> > > > > >>> Is there a testcase to trigger this ICE? I couldn't find one. > > > > >> > > > > >> You can take the address of an element of complex: > > > > >> > > > > >> __complex int i; > > > > >> int *p = &__real(i); > > > > >> > > > > >> You may get VIEW_CONVERT_EXPR with location wrappers. > > > > > > > > > > Fixed. I replaced gcc_unreachable with return NULL_TREE; > > > > > > > > Then we're back to my earlier question: are you deliberately not > > > > handling the other cases? Why not look through them as well? What if > > > > e.g. the operand of __real is a packed field? > > > > > > > > > > Here is the updated patch with > > > > > > diff --git a/gcc/c-family/c-warn.c b/gcc/c-family/c-warn.c > > > index 615134cfdac..f105742598e 100644 > > > --- a/gcc/c-family/c-warn.c > > > +++ b/gcc/c-family/c-warn.c > > > @@ -2669,6 +2669,9 @@ check_address_of_packed_member (tree type, tree rhs) > > > switch (code) > > > { > > > case ARRAY_REF: > > > + case REALPART_EXPR: > > > + case IMAGPART_EXPR: > > > + case
Re: V6 [PATCH] C/C++: Add -Waddress-of-packed-member
On Mon, Dec 17, 2018 at 1:39 AM Richard Biener wrote: > > On Fri, Dec 14, 2018 at 11:48 PM H.J. Lu wrote: > > > > On Fri, Dec 14, 2018 at 2:10 PM Jason Merrill wrote: > > > > > > On 12/13/18 6:56 PM, H.J. Lu wrote: > > > > On Thu, Dec 13, 2018 at 12:50 PM Jason Merrill wrote: > > > >> > > > >> On 9/25/18 11:46 AM, H.J. Lu wrote: > > > >>> On Fri, Aug 31, 2018 at 2:04 PM, Jason Merrill > > > >>> wrote: > > > On 07/23/2018 05:24 PM, H.J. Lu wrote: > > > > > > > > On Mon, Jun 18, 2018 at 12:26 PM, Joseph Myers > > > > > > > > wrote: > > > >> > > > >> On Mon, 18 Jun 2018, Jason Merrill wrote: > > > >> > > > >>> On Mon, Jun 18, 2018 at 11:59 AM, Joseph Myers > > > >>> > > > >>> wrote: > > > > > > On Mon, 18 Jun 2018, Jason Merrill wrote: > > > > > > >> + if (TREE_CODE (rhs) == COND_EXPR) > > > >> +{ > > > >> + /* Check the THEN path first. */ > > > >> + tree op1 = TREE_OPERAND (rhs, 1); > > > >> + context = check_address_of_packed_member (type, op1); > > > > > > > > > > > > This should handle the GNU extension of re-using operand 0 if > > > > operand > > > > 1 is omitted. > > > > > > > > > Doesn't that just use a SAVE_EXPR? > > > >>> > > > >>> > > > >>> Hmm, I suppose it does, but many places in the compiler seem to > > > >>> expect > > > >>> that it produces a COND_EXPR with TREE_OPERAND 1 as NULL_TREE. > > > >> > > > >> > > > >> Maybe that's used somewhere inside the C++ front end. For C a > > > >> SAVE_EXPR > > > >> is produced directly. > > > > > > > > > > > > Here is the updated patch. Changes from the last one: > > > > > > > > 1. Handle COMPOUND_EXPR. > > > > 2. Fixed typos in comments. > > > > 3. Combined warn_for_pointer_of_packed_member and > > > > warn_for_address_of_packed_member into > > > > warn_for_address_or_pointer_of_packed_member. > > > > > > > > > > c.i:4:33: warning: converting a packed ‘struct C *’ pointer > > > > increases the > > > > alignment of ‘long int *’ pointer from 1 to 8 > > > > [-Waddress-of-packed-member] > > > > > > > > > I think this would read better as > > > > > > c.i:4:33: warning: converting a packed ‘struct C *’ pointer > > > (alignment 1) to > > > ‘long int *’ (alignment 8) may result in an unaligned pointer value > > > [-Waddress-of-packed-member] > > > >>> > > > >>> Fixed. > > > >>> > > > > + while (TREE_CODE (base) == ARRAY_REF) > > > > + base = TREE_OPERAND (base, 0); > > > > + if (TREE_CODE (base) != COMPONENT_REF) > > > > + return NULL_TREE; > > > > > > > > > Are you deliberately not handling the other handled_component_p > > > cases? If > > > so, there should be a comment. > > > >>> > > > >>> I changed it to > > > >>> > > > >>>while (handled_component_p (base)) > > > >>> { > > > >>> enum tree_code code = TREE_CODE (base); > > > >>> if (code == COMPONENT_REF) > > > >>> break; > > > >>> switch (code) > > > >>> { > > > >>> case ARRAY_REF: > > > >>> base = TREE_OPERAND (base, 0); > > > >>> break; > > > >>> default: > > > >>> /* FIXME: Can it ever happen? */ > > > >>> gcc_unreachable (); > > > >>> break; > > > >>> } > > > >>> } > > > >>> > > > >>> Is there a testcase to trigger this ICE? I couldn't find one. > > > >> > > > >> You can take the address of an element of complex: > > > >> > > > >> __complex int i; > > > >> int *p = &__real(i); > > > >> > > > >> You may get VIEW_CONVERT_EXPR with location wrappers. > > > > > > > > Fixed. I replaced gcc_unreachable with return NULL_TREE; > > > > > > Then we're back to my earlier question: are you deliberately not > > > handling the other cases? Why not look through them as well? What if > > > e.g. the operand of __real is a packed field? > > > > > > > Here is the updated patch with > > > > diff --git a/gcc/c-family/c-warn.c b/gcc/c-family/c-warn.c > > index 615134cfdac..f105742598e 100644 > > --- a/gcc/c-family/c-warn.c > > +++ b/gcc/c-family/c-warn.c > > @@ -2669,6 +2669,9 @@ check_address_of_packed_member (tree type, tree rhs) > > switch (code) > > { > > case ARRAY_REF: > > + case REALPART_EXPR: > > + case IMAGPART_EXPR: > > + case VIEW_CONVERT_EXPR: > > base = TREE_OPERAND (base, 0); > > break; > > default: > > don't we have handled_component_p () for this? (you're still > missing BIT_FIELD_REF which might be used for vector > element accesses) > Do you have a testcase? -- H.J.
Re: V6 [PATCH] C/C++: Add -Waddress-of-packed-member
On Fri, Dec 14, 2018 at 11:48 PM H.J. Lu wrote: > > On Fri, Dec 14, 2018 at 2:10 PM Jason Merrill wrote: > > > > On 12/13/18 6:56 PM, H.J. Lu wrote: > > > On Thu, Dec 13, 2018 at 12:50 PM Jason Merrill wrote: > > >> > > >> On 9/25/18 11:46 AM, H.J. Lu wrote: > > >>> On Fri, Aug 31, 2018 at 2:04 PM, Jason Merrill wrote: > > On 07/23/2018 05:24 PM, H.J. Lu wrote: > > > > > > On Mon, Jun 18, 2018 at 12:26 PM, Joseph Myers > > > > > > wrote: > > >> > > >> On Mon, 18 Jun 2018, Jason Merrill wrote: > > >> > > >>> On Mon, Jun 18, 2018 at 11:59 AM, Joseph Myers > > >>> > > >>> wrote: > > > > On Mon, 18 Jun 2018, Jason Merrill wrote: > > > > >> + if (TREE_CODE (rhs) == COND_EXPR) > > >> +{ > > >> + /* Check the THEN path first. */ > > >> + tree op1 = TREE_OPERAND (rhs, 1); > > >> + context = check_address_of_packed_member (type, op1); > > > > > > > > > This should handle the GNU extension of re-using operand 0 if > > > operand > > > 1 is omitted. > > > > > > Doesn't that just use a SAVE_EXPR? > > >>> > > >>> > > >>> Hmm, I suppose it does, but many places in the compiler seem to > > >>> expect > > >>> that it produces a COND_EXPR with TREE_OPERAND 1 as NULL_TREE. > > >> > > >> > > >> Maybe that's used somewhere inside the C++ front end. For C a > > >> SAVE_EXPR > > >> is produced directly. > > > > > > > > > Here is the updated patch. Changes from the last one: > > > > > > 1. Handle COMPOUND_EXPR. > > > 2. Fixed typos in comments. > > > 3. Combined warn_for_pointer_of_packed_member and > > > warn_for_address_of_packed_member into > > > warn_for_address_or_pointer_of_packed_member. > > > > > > > c.i:4:33: warning: converting a packed ‘struct C *’ pointer increases > > > the > > > alignment of ‘long int *’ pointer from 1 to 8 > > > [-Waddress-of-packed-member] > > > > > > I think this would read better as > > > > c.i:4:33: warning: converting a packed ‘struct C *’ pointer (alignment > > 1) to > > ‘long int *’ (alignment 8) may result in an unaligned pointer value > > [-Waddress-of-packed-member] > > >>> > > >>> Fixed. > > >>> > > > + while (TREE_CODE (base) == ARRAY_REF) > > > + base = TREE_OPERAND (base, 0); > > > + if (TREE_CODE (base) != COMPONENT_REF) > > > + return NULL_TREE; > > > > > > Are you deliberately not handling the other handled_component_p cases? > > If > > so, there should be a comment. > > >>> > > >>> I changed it to > > >>> > > >>>while (handled_component_p (base)) > > >>> { > > >>> enum tree_code code = TREE_CODE (base); > > >>> if (code == COMPONENT_REF) > > >>> break; > > >>> switch (code) > > >>> { > > >>> case ARRAY_REF: > > >>> base = TREE_OPERAND (base, 0); > > >>> break; > > >>> default: > > >>> /* FIXME: Can it ever happen? */ > > >>> gcc_unreachable (); > > >>> break; > > >>> } > > >>> } > > >>> > > >>> Is there a testcase to trigger this ICE? I couldn't find one. > > >> > > >> You can take the address of an element of complex: > > >> > > >> __complex int i; > > >> int *p = &__real(i); > > >> > > >> You may get VIEW_CONVERT_EXPR with location wrappers. > > > > > > Fixed. I replaced gcc_unreachable with return NULL_TREE; > > > > Then we're back to my earlier question: are you deliberately not > > handling the other cases? Why not look through them as well? What if > > e.g. the operand of __real is a packed field? > > > > Here is the updated patch with > > diff --git a/gcc/c-family/c-warn.c b/gcc/c-family/c-warn.c > index 615134cfdac..f105742598e 100644 > --- a/gcc/c-family/c-warn.c > +++ b/gcc/c-family/c-warn.c > @@ -2669,6 +2669,9 @@ check_address_of_packed_member (tree type, tree rhs) > switch (code) > { > case ARRAY_REF: > + case REALPART_EXPR: > + case IMAGPART_EXPR: > + case VIEW_CONVERT_EXPR: > base = TREE_OPERAND (base, 0); > break; > default: don't we have handled_component_p () for this? (you're still missing BIT_FIELD_REF which might be used for vector element accesses) > > Now I got > > [hjl@gnu-cfl-1 pr51628-6]$ cat foo.i > struct A { __complex int i; }; > struct B { struct A a; }; > struct C { struct B b __attribute__ ((packed)); }; > > extern struct C *p; > > int* > foo1 (void) > { > return &__real(p->b.a.i); > } > int* > foo2 (void) > { > return &__imag(p->b.a.i); > } > [hjl@gnu-cfl-1 pr51628-6]$ make foo.s > /export/build/gnu/tools-build/gcc-debug/build-x86_64-linux/gcc/xgcc >
V6 [PATCH] C/C++: Add -Waddress-of-packed-member
On Fri, Dec 14, 2018 at 2:10 PM Jason Merrill wrote: > > On 12/13/18 6:56 PM, H.J. Lu wrote: > > On Thu, Dec 13, 2018 at 12:50 PM Jason Merrill wrote: > >> > >> On 9/25/18 11:46 AM, H.J. Lu wrote: > >>> On Fri, Aug 31, 2018 at 2:04 PM, Jason Merrill wrote: > On 07/23/2018 05:24 PM, H.J. Lu wrote: > > > > On Mon, Jun 18, 2018 at 12:26 PM, Joseph Myers > > wrote: > >> > >> On Mon, 18 Jun 2018, Jason Merrill wrote: > >> > >>> On Mon, Jun 18, 2018 at 11:59 AM, Joseph Myers > >>> > >>> wrote: > > On Mon, 18 Jun 2018, Jason Merrill wrote: > > >> + if (TREE_CODE (rhs) == COND_EXPR) > >> +{ > >> + /* Check the THEN path first. */ > >> + tree op1 = TREE_OPERAND (rhs, 1); > >> + context = check_address_of_packed_member (type, op1); > > > > > > This should handle the GNU extension of re-using operand 0 if > > operand > > 1 is omitted. > > > Doesn't that just use a SAVE_EXPR? > >>> > >>> > >>> Hmm, I suppose it does, but many places in the compiler seem to expect > >>> that it produces a COND_EXPR with TREE_OPERAND 1 as NULL_TREE. > >> > >> > >> Maybe that's used somewhere inside the C++ front end. For C a > >> SAVE_EXPR > >> is produced directly. > > > > > > Here is the updated patch. Changes from the last one: > > > > 1. Handle COMPOUND_EXPR. > > 2. Fixed typos in comments. > > 3. Combined warn_for_pointer_of_packed_member and > > warn_for_address_of_packed_member into > > warn_for_address_or_pointer_of_packed_member. > > > > c.i:4:33: warning: converting a packed ‘struct C *’ pointer increases > > the > > alignment of ‘long int *’ pointer from 1 to 8 > > [-Waddress-of-packed-member] > > > I think this would read better as > > c.i:4:33: warning: converting a packed ‘struct C *’ pointer (alignment > 1) to > ‘long int *’ (alignment 8) may result in an unaligned pointer value > [-Waddress-of-packed-member] > >>> > >>> Fixed. > >>> > > + while (TREE_CODE (base) == ARRAY_REF) > > + base = TREE_OPERAND (base, 0); > > + if (TREE_CODE (base) != COMPONENT_REF) > > + return NULL_TREE; > > > Are you deliberately not handling the other handled_component_p cases? If > so, there should be a comment. > >>> > >>> I changed it to > >>> > >>>while (handled_component_p (base)) > >>> { > >>> enum tree_code code = TREE_CODE (base); > >>> if (code == COMPONENT_REF) > >>> break; > >>> switch (code) > >>> { > >>> case ARRAY_REF: > >>> base = TREE_OPERAND (base, 0); > >>> break; > >>> default: > >>> /* FIXME: Can it ever happen? */ > >>> gcc_unreachable (); > >>> break; > >>> } > >>> } > >>> > >>> Is there a testcase to trigger this ICE? I couldn't find one. > >> > >> You can take the address of an element of complex: > >> > >> __complex int i; > >> int *p = &__real(i); > >> > >> You may get VIEW_CONVERT_EXPR with location wrappers. > > > > Fixed. I replaced gcc_unreachable with return NULL_TREE; > > Then we're back to my earlier question: are you deliberately not > handling the other cases? Why not look through them as well? What if > e.g. the operand of __real is a packed field? > Here is the updated patch with diff --git a/gcc/c-family/c-warn.c b/gcc/c-family/c-warn.c index 615134cfdac..f105742598e 100644 --- a/gcc/c-family/c-warn.c +++ b/gcc/c-family/c-warn.c @@ -2669,6 +2669,9 @@ check_address_of_packed_member (tree type, tree rhs) switch (code) { case ARRAY_REF: + case REALPART_EXPR: + case IMAGPART_EXPR: + case VIEW_CONVERT_EXPR: base = TREE_OPERAND (base, 0); break; default: Now I got [hjl@gnu-cfl-1 pr51628-6]$ cat foo.i struct A { __complex int i; }; struct B { struct A a; }; struct C { struct B b __attribute__ ((packed)); }; extern struct C *p; int* foo1 (void) { return &__real(p->b.a.i); } int* foo2 (void) { return &__imag(p->b.a.i); } [hjl@gnu-cfl-1 pr51628-6]$ make foo.s /export/build/gnu/tools-build/gcc-debug/build-x86_64-linux/gcc/xgcc -B/export/build/gnu/tools-build/gcc-debug/build-x86_64-linux/gcc/ -O2 -S foo.i foo.i: In function ‘foo1’: foo.i:10:10: warning: taking address of packed member of ‘struct C’ may result in an unaligned pointer value [-Waddress-of-packed-member] 10 | return &__real(p->b.a.i); | ^ foo.i: In function ‘foo2’: foo.i:15:10: warning: taking address of packed member of ‘struct C’ may result in an unaligned pointer value [-Waddress-of-packed-member] 15 | return &__imag(p->b.a.i);