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

[Jason Merrill <jason@redhat.com>]

On 2/11/22 11:07, Qing Zhao wrote:
> Hi, Jason,
> 
>> On Feb 9, 2022, at 3:01 PM, Qing Zhao via Gcc-patches 
>> <gcc-patches@gcc.gnu.org <mailto:gcc-patches@gcc.gnu.org>> wrote:
>>
>>
>>
>>> On Feb 9, 2022, at 12:23 PM, Jason Merrill <jason@redhat.com 
>>> <mailto:jason@redhat.com>> wrote:
>>>
>>> On 2/9/22 10:51, Qing Zhao wrote:
>>>>> On Feb 8, 2022, at 4:20 PM, Jason Merrill <jason@redhat.com 
>>>>> <mailto:jason@redhat.com>> wrote:
>>>>>
>>>>> On 2/8/22 15:11, Qing Zhao wrote:
>>>>>> Hi,
>>>>>> This is the patch to fix PR101515 (ICE in pp_cxx_unqualified_id, 
>>>>>> at  cp/cxx-pretty-print.c:128)
>>>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101515 
>>>>>> <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101515>
>>>>>> It's possible that the TYPE_NAME of a record_type is NULL, 
>>>>>> therefore when
>>>>>> printing the TYPE_NAME, we should check and handle this special case.
>>>>>> Please see the comment of pr101515 for more details.
>>>>>> The fix is very simple, just check and special handle cases when 
>>>>>> TYPE_NAME is NULL.
>>>>>> Bootstrapped and regression tested on both x86 and aarch64, no issues.
>>>>>> Okay for commit?
>>>>>> Thanks.
>>>>>> Qing
>>>>>> =====================================
>>>>>> From f37ee8d21b80cb77d8108cb97a487c84c530545b Mon Sep 17 00:00:00 2001
>>>>>> From: Qing Zhao <qing.zhao@oracle.com>
>>>>>> Date: Tue, 8 Feb 2022 16:10:37 +0000
>>>>>> Subject: [PATCH] Fix PR 101515 ICE in pp_cxx_unqualified_id, at
>>>>>> cp/cxx-pretty-print.c:128.
>>>>>> It's possible that the TYPE_NAME of a record_type is NULL, 
>>>>>> therefore when
>>>>>> printing the TYPE_NAME, we should check and handle this special case.
>>>>>> gcc/cp/ChangeLog:
>>>>>> * cxx-pretty-print.cc (pp_cxx_unqualified_id): Check and handle
>>>>>> the case when TYPE_NAME is NULL.
>>>>>> gcc/testsuite/ChangeLog:
>>>>>> * g++.dg/pr101515.C: New test.
>>>>>> ---
>>>>>> gcc/cp/cxx-pretty-print.cc      |  5 ++++-
>>>>>> gcc/testsuite/g++.dg/pr101515.C | 25 +++++++++++++++++++++++++
>>>>>> 2 files changed, 29 insertions(+), 1 deletion(-)
>>>>>> create mode 100644 gcc/testsuite/g++.dg/pr101515.C
>>>>>> diff --git a/gcc/cp/cxx-pretty-print.cc b/gcc/cp/cxx-pretty-print.cc
>>>>>> index 4f9a090e520d..744ed0add5ba 100644
>>>>>> --- a/gcc/cp/cxx-pretty-print.cc
>>>>>> +++ b/gcc/cp/cxx-pretty-print.cc
>>>>>> @@ -171,7 +171,10 @@ pp_cxx_unqualified_id (cxx_pretty_printer 
>>>>>> *pp, tree t)
>>>>>>     case ENUMERAL_TYPE:
>>>>>>     case TYPENAME_TYPE:
>>>>>>     case UNBOUND_CLASS_TEMPLATE:
>>>>>> -      pp_cxx_unqualified_id (pp, TYPE_NAME (t));
>>>>>> +      if (TYPE_NAME (t))
>>>>>> +pp_cxx_unqualified_id (pp, TYPE_NAME (t));
>>>>>> +      else
>>>>>> +pp_string (pp, "<unnamed type>");
>>>>>
>>>>> Hmm, but it's not an unnamed class, it's a pointer to member 
>>>>> function type, and it would be better to avoid dumping compiler 
>>>>> internal representations like the __pfn field name.
>>>> Yes, It’s not an unnamed class, but the ICE happened when try to 
>>>> print the compiler generated member function type 
>>>> “__ptrmemfunc_type”, whose TYPE_NAME is NULLed during building this 
>>>> type in c++ FE and the c++ FE does not handle the case when 
>>>> TYPE_NAME is NULL correctly.
>>>> So, there are two levels of issues:
>>>> 1. The first level issue is that the current C++ FE does not handle 
>>>> the case TYPE_NAME being NULL correctly, this is indeed a bug in the 
>>>> current code and should be fixed as in the current patch.
>>>
>>> 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, and 
removing it fixes the testcase, so I see

warning: ‘*(ptrmemfunc*)&x.ptrmemfunc::ptr’ is used uninitialized

Jason