On Wed, 13 Jan 2021, Jakub Jelinek wrote:

> Hi!
> 
> The following patch doesn't actually fix the print_mem_ref bugs, I've kept
> it for now as broken as it was, but at least improves the cases where
> we can unambiguously map back MEM[&something + off] into some particular
> reference (e.g. something.foo[1].bar etc.).
> In the distant past I think we were folding such MEM_REFs back to
> COMPONENT_REFs and ARRAY_REFs, but we've stopped doing that.

Yeah, because it has different semantics - *(((int *)t + 3)
accesses an int object while t.u.b accesses a 't' object from the TBAA
perspective.

>  But for
> diagnostics that is what the user actually want to see IMHO.
> So on the attached testcase, instead of printing what is in left column
> it prints what is in right column:
> ((int*)t) + 3         t.u.b
> ((int*)t) + 6         t.u.e.i
> ((int*)t) + 8         t.v
> s + 1                 s[1]

so while that's "nice" in general, for TBAA diagnostics it might actually
be misleading.

I wonder whether we absolutely need to print a C expression here.
We could print, instead of *((int *)t + 3), "access to a memory
object of type 'int' at offset 12 bytes from 't'", thus explain
in plain english.

That said, *((int *)t + 3) is exactly what the access is,
semantically.  There's the case of a mismatch of the access type
and the TBAA type which we cannot write down in C terms but maybe
we want to have a builtin for this?  __builtin_access (ptr, lvalue-type,
tbaa-type)?

> Of course, print_mem_ref needs to be also fixed to avoid printing the
> nonsense it is printing right now, t is a structure type, so it can't be
> cast to int* in C and in C++ only using some user operator, and
> the result of what it printed is a pointer, while the uninitialized reads
> are int.
> 
> I was hoping Martin would fix that, but given his comment in the PR I think
> I'll fix it myself tomorrow.
> 
> Anyway, this patch is useful on its own.  Bootstrapped/regtested on
> x86_64-linux and i686-linux, ok for trunk?

In the light of Martins patch this is probably reasonable but still
the general direction is wrong (which is why I didn't approve Martins
original patch).  I'm also somewhat disappointed we're breaking this
so late in the cycle.

c_fold_indirect_ref_for_warn doesn't look like it is especially
careful about error recovery issues (error_mark_node in random
places of the trees).  Maybe that never happens.

Thanks,
Richard.

> 2021-01-13  Jakub Jelinek  <ja...@redhat.com>
> 
>       PR tree-optimization/98597
>       * c-pretty-print.c (c_fold_indirect_ref_for_warn): New function.
>       (print_mem_ref): Call it.
> 
>       * gcc.dg/uninit-40.c: New test.
> 
> --- gcc/c-family/c-pretty-print.c.jj  2021-01-13 08:02:09.425498954 +0100
> +++ gcc/c-family/c-pretty-print.c     2021-01-13 15:02:57.860329998 +0100
> @@ -1809,6 +1809,113 @@ pp_c_call_argument_list (c_pretty_printe
>    pp_c_right_paren (pp);
>  }
>  
> +/* Try to fold *(type *)&op into op.fld.fld2[1] if possible.
> +   Only used for printing expressions.  Should punt if ambiguous
> +   (e.g. in unions).  */
> +
> +static tree
> +c_fold_indirect_ref_for_warn (location_t loc, tree type, tree op,
> +                           offset_int &off)
> +{
> +  tree optype = TREE_TYPE (op);
> +  if (off == 0)
> +    {
> +      if (lang_hooks.types_compatible_p (optype, type))
> +     return op;
> +      /* *(foo *)&complexfoo => __real__ complexfoo */
> +      else if (TREE_CODE (optype) == COMPLEX_TYPE
> +            && lang_hooks.types_compatible_p (type, TREE_TYPE (optype)))
> +     return build1_loc (loc, REALPART_EXPR, type, op);
> +    }
> +  /* ((foo*)&complexfoo)[1] => __imag__ complexfoo */
> +  else if (TREE_CODE (optype) == COMPLEX_TYPE
> +        && lang_hooks.types_compatible_p (type, TREE_TYPE (optype))
> +        && tree_to_uhwi (TYPE_SIZE_UNIT (type)) == off)
> +    {
> +      off = 0;
> +      return build1_loc (loc, IMAGPART_EXPR, type, op);
> +    }
> +  /* ((foo *)&fooarray)[x] => fooarray[x] */
> +  if (TREE_CODE (optype) == ARRAY_TYPE
> +      && TYPE_SIZE_UNIT (TREE_TYPE (optype))
> +      && TREE_CODE (TYPE_SIZE_UNIT (TREE_TYPE (optype))) == INTEGER_CST
> +      && !integer_zerop (TYPE_SIZE_UNIT (TREE_TYPE (optype))))
> +    {
> +      tree type_domain = TYPE_DOMAIN (optype);
> +      tree min_val = size_zero_node;
> +      if (type_domain && TYPE_MIN_VALUE (type_domain))
> +     min_val = TYPE_MIN_VALUE (type_domain);
> +      offset_int el_sz = wi::to_offset (TYPE_SIZE_UNIT (TREE_TYPE (optype)));
> +      offset_int idx = off / el_sz;
> +      offset_int rem = off % el_sz;
> +      if (TREE_CODE (min_val) == INTEGER_CST)
> +     {
> +       tree index
> +         = wide_int_to_tree (sizetype, idx + wi::to_offset (min_val));
> +       op = build4_loc (loc, ARRAY_REF, TREE_TYPE (optype), op, index,
> +                        NULL_TREE, NULL_TREE);
> +       off = rem;
> +       if (tree ret = c_fold_indirect_ref_for_warn (loc, type, op, off))
> +         return ret;
> +       return op;
> +     }
> +    }
> +  /* ((foo *)&struct_with_foo_field)[x] => COMPONENT_REF */
> +  else if (TREE_CODE (optype) == RECORD_TYPE)
> +    {
> +      for (tree field = TYPE_FIELDS (optype);
> +        field; field = DECL_CHAIN (field))
> +     if (TREE_CODE (field) == FIELD_DECL
> +         && TREE_TYPE (field) != error_mark_node
> +         && TYPE_SIZE_UNIT (TREE_TYPE (field))
> +         && TREE_CODE (TYPE_SIZE_UNIT (TREE_TYPE (field))) == INTEGER_CST)
> +       {
> +         tree pos = byte_position (field);
> +         if (TREE_CODE (pos) != INTEGER_CST)
> +           continue;
> +         offset_int upos = wi::to_offset (pos);
> +         offset_int el_sz
> +           = wi::to_offset (TYPE_SIZE_UNIT (TREE_TYPE (field)));
> +         if (upos <= off && off < upos + el_sz)
> +           {
> +             tree cop = build3_loc (loc, COMPONENT_REF, TREE_TYPE (field),
> +                                    op, field, NULL_TREE);
> +             off = off - upos;
> +             if (tree ret = c_fold_indirect_ref_for_warn (loc, type, cop,
> +                                                          off))
> +               return ret;
> +             return cop;
> +           }
> +       }
> +    }
> +  /* Similarly for unions, but in this case try to be very conservative,
> +     only match if some field has type compatible with type and it is the
> +     only such field.  */
> +  else if (TREE_CODE (optype) == UNION_TYPE)
> +    {
> +      tree fld = NULL_TREE;
> +      for (tree field = TYPE_FIELDS (optype);
> +        field; field = DECL_CHAIN (field))
> +     if (TREE_CODE (field) == FIELD_DECL
> +         && TREE_TYPE (field) != error_mark_node
> +         && lang_hooks.types_compatible_p (TREE_TYPE (field), type))
> +       {
> +         if (fld)
> +           return NULL_TREE;
> +         else
> +           fld = field;
> +       }
> +      if (fld)
> +     {
> +       off = 0;
> +       return build3_loc (loc, COMPONENT_REF, TREE_TYPE (fld), op, fld,
> +                          NULL_TREE);
> +     }
> +    }
> +
> +  return NULL_TREE;
> +}
> +
>  /* Print the MEM_REF expression REF, including its type and offset.
>     Apply casts as necessary if the type of the access is different
>     from the type of the accessed object.  Produce compact output
> @@ -1836,6 +1943,17 @@ print_mem_ref (c_pretty_printer *pp, tre
>    const bool addr = TREE_CODE (arg) == ADDR_EXPR;
>    if (addr)
>      {
> +      tree op
> +     = c_fold_indirect_ref_for_warn (EXPR_LOCATION (e), TREE_TYPE (e),
> +                                     TREE_OPERAND (arg, 0), byte_off);
> +      if (op
> +       && byte_off == 0
> +       && lang_hooks.types_compatible_p (TREE_TYPE (e), TREE_TYPE (op)))
> +     {
> +       pp->expression (op);
> +       return;
> +     }
> +      byte_off = wi::to_offset (TREE_OPERAND (e, 1));
>        arg = TREE_OPERAND (arg, 0);
>        if (byte_off == 0)
>       {
> --- gcc/testsuite/gcc.dg/uninit-40.c.jj       2021-01-13 15:23:47.366134905 
> +0100
> +++ gcc/testsuite/gcc.dg/uninit-40.c  2021-01-13 15:23:15.208500279 +0100
> @@ -0,0 +1,31 @@
> +/* PR tree-optimization/98597 */
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -Wuninitialized" } */
> +
> +union U { double d; int i; float f; };
> +struct S { char a; int b; char c; unsigned d; union U e; };
> +struct T { char t; struct S u; int v; };
> +typedef short U[2][2];
> +void baz (U *);
> +
> +static inline int
> +bar (char *p)
> +{
> +  return *(int *) p;
> +}
> +
> +void
> +foo (int *q)
> +{
> +  struct T t;
> +  t.t = 1;
> +  t.u.c = 2;
> +  char *pt = (char *) &t;
> +  q[0] = bar (pt + __builtin_offsetof (struct T, u.b));      /* { dg-warning 
> "'t\\.u\\.b' is used uninitialized" } */
> +  q[1] = bar (pt + __builtin_offsetof (struct T, u.e));      /* { dg-warning 
> "'t\\.u\\.e\\.i' is used uninitialized" } */
> +  q[2] = bar (pt + __builtin_offsetof (struct T, v));        /* { dg-warning 
> "'t\\.v' is used uninitialized" } */
> +  int s[3];
> +  s[0] = 1;
> +  char *ps = (char *) s;
> +  q[3] = bar (ps + sizeof (int));                    /* { dg-warning 
> "'s\\\[1\\\]' is used uninitialized" } */
> +}
> 
>       Jakub
> 
> 

-- 
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)

Reply via email to