On Mon, Dec 17, 2018 at 1:39 AM Richard Biener
<richard.guent...@gmail.com> wrote:
>
> On Fri, Dec 14, 2018 at 11:48 PM H.J. Lu <hjl.to...@gmail.com> wrote:
> >
> > On Fri, Dec 14, 2018 at 2:10 PM Jason Merrill <ja...@redhat.com> wrote:
> > >
> > > On 12/13/18 6:56 PM, H.J. Lu wrote:
> > > > On Thu, Dec 13, 2018 at 12:50 PM Jason Merrill <ja...@redhat.com> wrote:
> > > >>
> > > >> On 9/25/18 11:46 AM, H.J. Lu wrote:
> > > >>> On Fri, Aug 31, 2018 at 2:04 PM, Jason Merrill <ja...@redhat.com> 
> > > >>> wrote:
> > > >>>> On 07/23/2018 05:24 PM, H.J. Lu wrote:
> > > >>>>>
> > > >>>>> On Mon, Jun 18, 2018 at 12:26 PM, Joseph Myers 
> > > >>>>> <jos...@codesourcery.com>
> > > >>>>> wrote:
> > > >>>>>>
> > > >>>>>> On Mon, 18 Jun 2018, Jason Merrill wrote:
> > > >>>>>>
> > > >>>>>>> On Mon, Jun 18, 2018 at 11:59 AM, Joseph Myers 
> > > >>>>>>> <jos...@codesourcery.com>
> > > >>>>>>> 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.

Reply via email to