Re: [PATCH] Fix PR 101515 (ICE in pp_cxx_unqualified_id, at cp/cxx-pretty-print.c:128)

[Jakub Jelinek <jakub@redhat.com>]

On Fri, Mar 18, 2022 at 01:35:53PM -0400, Jason Merrill wrote:
> On 3/15/22 12:06, Jakub Jelinek wrote:
> > On Tue, Mar 15, 2022 at 11:57:22AM -0400, Jason Merrill wrote:
> > > > The intent of r11-6729 is that it prints something that helps user to figure
> > > > out what exactly is being accessed.
> > > > When we find a unique non-static data member that is being accessed, even
> > > > when we can't fold it nicely, IMNSHO it is better to print
> > > >     ((sometype *)&var)->field
> > > > or
> > > >     (*(sometype *)&var).field
> > > > instead of
> > > >     *(fieldtype *)((char *)&var + 56)
> > > > because the user doesn't know what is at offset 56, we shouldn't ask user
> > > > to decipher structure layout etc.
> > > 
> > > The problem is that the reference is *not* to any non-static data member,
> > > it's to the PMF as a whole.  But c_fold_indirect_ref_for_warn wrongly turns
> > > it into a reference to the first non-static data member.
> > > 
> > > We asked c_fold_indirect_ref_warn to fold a MEM_REF with RECORD_TYPE, and it
> > > gave us back a COMPONENT_REF with POINTER_TYPE.  That seems clearly wrong.
> > 
> > That is not what I see on the testcase.
> > I see the outer c_fold_indirect_ref_for_warn call with type ptrmemfunc
> > which is a 64-bit RECORD_TYPE containing a single ptr member which has
> > pointer to function type, and op which is the x VAR_DECL with sp type which
> > is 128-bit RECORD_TYPE containing 64-bit __pfn member and 64-bit __delta
> > member.
> 
> Exactly: TYPE is RECORD_TYPE, TREE_TYPE (field) is POINTER_TYPE.
> 
> > As all the bits of the ptrmemfunc RECORD_TYPE fit within the __pfn member
> > (they are equal size), it wants to print (cast)(something.__pfn).
> 
> I disagree that this is what we want; we asked to fold an expression with
> class type, it seems unlikely that we want to get back an expression with
> pointer type.

That doesn't matter.  What c_fold_indirect_ref_warn returns is something
that can help the user understand what is actually being accessed.
Consider slightly modified testcase (which doesn't use the PMFs so that
we don't ICE on those too):
// PR c++/101515
// { dg-do compile }
// { dg-options "-O1 -Wuninitialized" }

struct S { int j; };
struct T : public S { virtual void h () {} };
struct U { char buf[32]; void (*ptr) (); };
struct sp { char a[14]; char b[50]; void (*pfn) (); long delta; };

int
main ()
{
  T t;
  sp x;
  U *xp = (U *) &x.b[18];
  if (xp->ptr != ((void (*) ()) (sizeof (void *))))
    return 1;
}
Trunk emits:
pr101515-2.C: In function ‘int main()’:
pr101515-2.C:16:11: warning: ‘*(U*)((char*)&x + offsetof(sp, sp::b[18])).U::ptr’ is used uninitialized [-Wuninitialized]
   16 |   if (xp->ptr != ((void (*) ()) (sizeof (void *))))
      |       ~~~~^~~
pr101515-2.C:14:6: note: ‘x’ declared here
   14 |   sp x;
      |      ^
here, which is indeed quite long expression, but valid C++ and helps the
user to narrow down what exactly is being accessed.
If I comment out the c_fold_indirect_ref_warn RECORD_TYPE handling so that
it punts on it, it prints:
pr101515-2.C: In function ‘int main()’:
pr101515-2.C:16:11: warning: ‘*(U*)((char*)&x + 32).U::ptr’ is used uninitialized [-Wuninitialized]
   16 |   if (xp->ptr != ((void (*) ()) (sizeof (void *))))
      |       ~~~~^~~
pr101515-2.C:14:6: note: ‘x’ declared here
   14 |   sp x;
      |      ^
That is also correct C++ expression, but user probably has no idea what is
present at offset 32 into the variable.
Of course if there is a type match and not any kind of type punning,
it will try to print much shorter and more readable expressions.

	Jakub