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

[Qing Zhao <qing.zhao@oracle.com>]

Hi, Jason,

> On Feb 11, 2022, at 11:27 AM, Jason Merrill <jason@redhat.com> wrote:
>>>> 
>>>> Sure, we might as well make this code more robust.  But we can do better than <unnamed type> if we check TYPE_PTRMEMFUNC_P.
>>> Okay, so what should we print to the user if it's “TYPE_PTRMEMFUNC_P”? Print nothing or some special string?
>>>> 
>>>>> 2. The second level issue is what you suggested in the above, shall we print the “compiler generated internal type”  to the user? And I agree with you that it might not be a good idea to print such compiler internal names to the user.  Are we do this right now in general? (i.e, check whether the current TYPE is a source level TYPE or a compiler internal TYPE, and then only print out the name of TYPE for the source level TYPE?) and is there a bit in the TYPE to distinguish whether a TYPE is user -level type or a compiler generated internal type?
>>>> 
>>>>>> I think the real problem comes sooner, when c_fold_indirect_ref_for_warn turns a MEM_REF with RECORD_TYPE into a COMPONENT_REF with POINTER_TYPE.
>>>> 
>>>>> What’s the major issue for this transformation? (I will study this in more details).
>>>> 
>>>> We told c_fold_indirect_ref that we want a RECORD_TYPE (the PMF as a whole) and it gave us back a POINTER_TYPE instead (the __pmf member). Folding shouldn't change the type of an expression like that.
>>> 
>>> Yes, this is not correct transformation, will study in more detail and try to fix it.
>> After a deeper study of commit  r11-6729-gadb520606ce3e1e1 (which triggered the ICE and introduced the new routine “c_fold_indirect_ref_for_warn”), from my understanding,  the above transformation from a RECORD_TYPE (the PMF as a whole) to POINTER_TYPE (the __pmf member) is what the function intended to do as following:
>> 1823 static tree
>> 1824 c_fold_indirect_ref_for_warn (location_t loc, tree type, tree op,
>> 1825                               offset_int &off)
>> 1826 {
>> …
>> 1870 */* ((foo *)&struct_with_foo_field)[x] => COMPONENT_REF */*
>> 1871   else if (TREE_CODE (optype) == RECORD_TYPE)
>> 1872     {
>> 1873       for (tree field = TYPE_FIELDS (optype);
>> 1874            field; field = DECL_CHAIN (field))
>> 1875         if (TREE_CODE (field) == FIELD_DECL
>> …
>> 1886 if(upos <= off && off < upos + el_sz)
>> 1887               {
>> 1888                 tree cop = build3_loc (loc, COMPONENT_REF, TREE_TYPE (field),
>> 1889                                       op, field, NULL_TREE);
>> 1890                 off = off - upos;
>> The above code was used to transform a MEM_REF to a RECORD_TYPE to a COMPONENT_REF to the corresponding FIELD.
> 
> Yes, that's what the above code would correctly do if TYPE were the pointer-to-method type.  It's wrong for this case because TYPE is unrelated to TREE_TYPE (field).
> 
> I think the problem is just this line:
> 
>>                if (tree ret = c_fold_indirect_ref_for_warn (loc, type, cop,
>>                                                             off))
>>                  return ret;
>>                return cop;
>                  ^^^^^^^^^^
> 
> The recursive call does the proper type checking, but then the "return cop" line returns the COMPONENT_REF even though the type check failed. The parallel code in cxx_fold_indirect_ref_1 doesn't have this line,

Just compared the routine “cxx_fold_indirect_ref_1” and “c_fold_indirect_ref_for_warn”, looks like there are more places that have such difference, for example, 
In “cxx_fold_indirect_ref_1”:

  /* ((foo *)&fooarray)[x] => fooarray[x] */
  else if (TREE_CODE (optype) == ARRAY_TYPE
           && tree_fits_uhwi_p (TYPE_SIZE_UNIT (TREE_TYPE (optype)))
           && !integer_zerop (TYPE_SIZE_UNIT (TREE_TYPE (optype))))
…
      if (tree_fits_uhwi_p (min_val))
        {
          tree index = size_int (idx + tree_to_uhwi (min_val));
          op = build4_loc (loc, ARRAY_REF, TREE_TYPE (optype), op, index,
                           NULL_TREE, NULL_TREE);         
	  return cxx_fold_indirect_ref_1 (ctx, loc, type, op, rem,
                                          empty_base);
	}
However, in “c_fold_indirect_ref_for_warn”, the corresponding part is:

  /* ((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))))
…
      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;
        }

The exactly same difference as for “RECORD_TYPE”. So, I suspect that it’s a typo for “RECORD_TYPE” in “c_fold_indirect_ref_for_warn”. 

> and removing it fixes the testcase, so I see
> 
> warning: ‘*(ptrmemfunc*)&x.ptrmemfunc::ptr’ is used uninitialized


The question is:

For the following IR:

  struct sp x;
  void (*<T389>) (void) _1;
 ...
  <bb 2> [local count: 1073741824]:
  _1 = MEM[(struct ptrmemfunc_U *)&x].ptr;
  _7 = _1 != 8B;

Which message is better:

1. warning: ‘*(ptrmemfunc*)&x.ptrmemfunc::ptr’ is used uninitialized
Or
2. warning: ‘*(ptrmemfunc*)((char*)&x + offsetof(void (S::*)(),__PTRMEMFUNC)).ptrmemfunc::ptr’ is used uninitialized

From the source code:

====
struct S
{
  int j;
};
struct T : public S
{
  virtual void h () {}
};
struct ptrmemfunc
{
  void (*ptr) ();
};
typedef void (S::*sp)();
int main ()
{
  T t;
  sp x;
  ptrmemfunc *xp = (ptrmemfunc *) &x;
  if (xp->ptr != ((void (*)())(sizeof(void *))))
    return 1;
}
====

The reference “xp->ptr” went through from “x” to “xp”, and there is a clear type casting from S::__PTRMEMFUNC to ptrmemfunc::ptr. 
Shall we emit such type casting to the user?

Qing

> Jason
>