On Tue, Jun 23, 2020 at 12:22 AM Martin Sebor via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> On 6/22/20 12:55 PM, Jason Merrill wrote:
> > On 6/22/20 1:25 PM, Martin Sebor wrote:
> >> The attached fix parallels the one for the equivalent C bug 95580
> >> where the pretty printers don't correctly handle MEM_REF arguments
> >> with type void* or other pointers to an incomplete type.
> >>
> >> The incorrect handling was exposed by the recent change to
> >> -Wuninitialized which includes such expressions in diagnostics.
> >
> >> + if (tree size = TYPE_SIZE_UNIT (TREE_TYPE (argtype)))
> >> + if (!integer_onep (size))
> >> + {
> >> + pp_cxx_left_paren (pp);
> >> + dump_type (pp, ptr_type_node, flags);
> >> + pp_cxx_right_paren (pp);
> >> + }
> >
> > Don't we want to print the cast if the pointer target type is incomplete?
>
> I suppose, yes, although after some more testing I think what should
> be output is the type of the access. The target pointer type isn't
> meaningful (at least not in this case).
>
> Here's what the warning looks like in C for the test case in
> gcc.dg/pr95580.c:
>
> warning: ‘*((void *)(p)+1)’ may be used uninitialized
>
> and like this in C++:
>
> warning: ‘*(p +1)’ may be used uninitialized
>
> The +1 is a byte offset, which is correct given that incrementing
> a void* in GCC is the same as adding 1 to the byte address, but
> dereferencing a void* doesn't correspond to what's going on in
> the source.
>
> Even for a complete type (with size greater than 1), printing
> the type of the argument plus a byte offset is wrong. It ends
> up with this for the C++ test case from 95768:
>
> warning: ‘*((int*)<unknown> +4)’ is used uninitialized
>
> when the access is actually ‘*((int*)<unknown> +1)’
>
> So it seems to me for MEM_REF, to make the output meaningful,
> it's the type of the access (i.e., the MEM_REF type) that should
> be printed here, and the offset should either be in elements of
> the accessed type, i.e.,
>
> warning: ‘*((int*)<unknown> +1)’ is used uninitialized
>
> or, if the access is misaligned, the argument should first be
> cast to char*, the offset added, and the result then cast to
> the access type, like this:
>
> warning: ‘*(T*)((char*)<unknown> +1)’ is used uninitialized
>
> The attached revised and less than fully tested patch implements
> this for C++ only for now. If we agree on this approach I'll see
> about making the corresponding change in C.
Note that there is no C/C++ way of fully expressing MEM_REF
semantics. __MEM <int> ((T *)p + 1) is not actually
*(int *)((char *)p + 1) because that does not reflect that the
effective type of the lvalue when TBAA is concerned is 'T'
rather than 'int'. Note for MEM_REF the offset is always
a constant byte offset but it indeed does not have to be a
multiple of the MEM_REF type size.
I wonder whether printing the MEM_REF in full provides
any real diagnostic value in the more "obfuscated" cases.
I'd also not print <unknown> but <register>.
Richard.
> Martin
