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

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

[Qing Zhao]

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

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>");
       if (tree ti = TYPE_TEMPLATE_INFO_MAYBE_ALIAS (t))
 	if (PRIMARY_TEMPLATE_P (TI_TEMPLATE (ti)))
 	  {
diff --git a/gcc/testsuite/g++.dg/pr101515.C b/gcc/testsuite/g++.dg/pr101515.C
new file mode 100644
index 000000000000..898c7e003c22
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr101515.C
@@ -0,0 +1,25 @@
+/* PR101515 -  ICE in pp_cxx_unqualified_id, at cp/cxx-pretty-print.c:128 
+   { dg-do compile }
+   { dg-options "-Wuninitialized -O1" } */
+
+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 *))))  /* { dg-warning "is used uninitialized" } */
+    return 1;
+}
-- 
2.27.0

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

[Jason Merrill]

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
> 
> 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.

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.

>         if (tree ti = TYPE_TEMPLATE_INFO_MAYBE_ALIAS (t))
>   	if (PRIMARY_TEMPLATE_P (TI_TEMPLATE (ti)))
>   	  {
> diff --git a/gcc/testsuite/g++.dg/pr101515.C b/gcc/testsuite/g++.dg/pr101515.C
> new file mode 100644
> index 000000000000..898c7e003c22
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/pr101515.C
> @@ -0,0 +1,25 @@
> +/* PR101515 -  ICE in pp_cxx_unqualified_id, at cp/cxx-pretty-print.c:128
> +   { dg-do compile }
> +   { dg-options "-Wuninitialized -O1" } */
> +
> +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 *))))  /* { dg-warning "is used uninitialized" } */
> +    return 1;
> +}

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

[Qing Zhao]

> On Feb 8, 2022, at 4:20 PM, Jason Merrill <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
>> 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. 

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).

thanks.

Qing


> 
>>        if (tree ti = TYPE_TEMPLATE_INFO_MAYBE_ALIAS (t))
>>  	if (PRIMARY_TEMPLATE_P (TI_TEMPLATE (ti)))
>>  	  {
>> diff --git a/gcc/testsuite/g++.dg/pr101515.C b/gcc/testsuite/g++.dg/pr101515.C
>> new file mode 100644
>> index 000000000000..898c7e003c22
>> --- /dev/null
>> +++ b/gcc/testsuite/g++.dg/pr101515.C
>> @@ -0,0 +1,25 @@
>> +/* PR101515 -  ICE in pp_cxx_unqualified_id, at cp/cxx-pretty-print.c:128
>> +   { dg-do compile }
>> +   { dg-options "-Wuninitialized -O1" } */
>> +
>> +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 *))))  /* { dg-warning "is used uninitialized" } */
>> +    return 1;
>> +}
> 

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

[Jason Merrill]

On 2/9/22 10:51, Qing Zhao wrote:
> 
> 
>> On Feb 8, 2022, at 4:20 PM, Jason Merrill <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
>>> 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.

> 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.

Jason

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

[Qing Zhao]

> On Feb 9, 2022, at 12:23 PM, Jason Merrill <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> 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
>>>> 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.

Qing
> 
> Jason

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

[Jason Merrill]

On 2/9/22 16:01, Qing Zhao wrote:
> 
> 
>> On Feb 9, 2022, at 12:23 PM, Jason Merrill <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> 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
>>>>> 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?

Maybe call pp->type_id in that case.

>>> 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.
> 
> Qing
>>
>> Jason
> 

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

[Qing Zhao]

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
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. And then finally print out this COMPONENT_REF as a BASE + offsetof (field), in which the “field” is the compiler generated __pmf member field of the class, and its TYPE_NAME is NULL. Since the current code of “cp/cxx-pretty-print.cc<http://cxx-pretty-print.cc>” does NOT handle the case when TYPE_NAME==NULL, therefore the ICE.

So, I still think that the major issue with this bug is cp/cxx-pretty-print.cc<http://cxx-pretty-print.cc> does not handle the case when TYPE_NAME==NULL. This clearly is a bug that need to be fixed.

Let me know if you have more comments and suggestions.

thanks.

Qing

Qing

Jason

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

[Jason Merrill]

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

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

[Qing Zhao]

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
> 

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

[Jason Merrill]

On 2/11/22 13:11, Qing Zhao wrote:
> 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?

But there is no such cast in the source; the cast is (ptrmemfunc*)&x, 
which appears in the fixed message.

Though *(ptrmemfunc*)&x.ptrmemfunc::ptr is wrong syntax, it should be 
((ptrmemfunc*)&x)->ptr

Jakub, this is your code from r11-6729; from the comment on that commit 
it sounds like you were deliberately ignoring type incompatibility here, 
and my suggested fix changes two lines in uninit-40.c.  What do you 
think should happen for this testcase?

Jason

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

[Qing Zhao]

> On Feb 11, 2022, at 1:39 PM, Jason Merrill <jason@redhat.com> wrote:
> 
> On 2/11/22 13:11, Qing Zhao wrote:
>> 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?
> 
> But there is no such cast in the source; the cast is (ptrmemfunc*)&x, which appears in the fixed message.

still a little confused here:  the original type for “x” is “sp” (is “sp” equal to “S::__PTRMEMFUNC”?), then it was casted to “ptrmemfunc *”.
So, should this type conversion from “S::__PTRMEMFUNC” to “ptrmemfunc *” be exposed to the user in the message? 

Qing
> 
> Though *(ptrmemfunc*)&x.ptrmemfunc::ptr is wrong syntax, it should be ((ptrmemfunc*)&x)->ptr
> 
> Jakub, this is your code from r11-6729; from the comment on that commit it sounds like you were deliberately ignoring type incompatibility here, and my suggested fix changes two lines in uninit-40.c.  What do you think should happen for this testcase?
> 
> Jason

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

[Jason Merrill]

On 2/11/22 15:29, Qing Zhao wrote:
> 
> 
>> On Feb 11, 2022, at 1:39 PM, Jason Merrill <jason@redhat.com> wrote:
>>
>> On 2/11/22 13:11, Qing Zhao wrote:
>>> 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?
>>
>> But there is no such cast in the source; the cast is (ptrmemfunc*)&x, which appears in the fixed message.
> 
> still a little confused here:  the original type for “x” is “sp”

Yes.

> (is “sp” equal to “S::__PTRMEMFUNC”?)

No.

> then it was casted to “ptrmemfunc *”.

Yes.

> So, should this type conversion from “S::__PTRMEMFUNC” to “ptrmemfunc *” be exposed to the user in the message?

The conversion from sp* to ptrmemfunc* is exposed as (ptrmemfunc*), 
which is normal C++ syntax; a cast only names the target type, not the 
source type.

>> Though *(ptrmemfunc*)&x.ptrmemfunc::ptr is wrong syntax, it should be ((ptrmemfunc*)&x)->ptr
>>
>> Jakub, this is your code from r11-6729; from the comment on that commit it sounds like you were deliberately ignoring type incompatibility here, and my suggested fix changes two lines in uninit-40.c.  What do you think should happen for this testcase?

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

[Qing Zhao]

> On Feb 11, 2022, at 3:54 PM, Jason Merrill <jason@redhat.com> wrote:
> 
> On 2/11/22 15:29, Qing Zhao wrote:
>>> On Feb 11, 2022, at 1:39 PM, Jason Merrill <jason@redhat.com> wrote:
>>> 
>>> On 2/11/22 13:11, Qing Zhao wrote:
>>>> 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?
>>> 
>>> But there is no such cast in the source; the cast is (ptrmemfunc*)&x, which appears in the fixed message.
>> still a little confused here:  the original type for “x” is “sp”
> 
> Yes.
> 
>> (is “sp” equal to “S::__PTRMEMFUNC”?)
> 
> No.
> 
>> then it was casted to “ptrmemfunc *”.
> 
> Yes.
> 
>> So, should this type conversion from “S::__PTRMEMFUNC” to “ptrmemfunc *” be exposed to the user in the message?
> 
> The conversion from sp* to ptrmemfunc* is exposed as (ptrmemfunc*), which is normal C++ syntax; a cast only names the target type, not the source type.
Okay, I see now.

thanks.

Qing
> 
>>> Though *(ptrmemfunc*)&x.ptrmemfunc::ptr is wrong syntax, it should be ((ptrmemfunc*)&x)->ptr
>>> 
>>> Jakub, this is your code from r11-6729; from the comment on that commit it sounds like you were deliberately ignoring type incompatibility here, and my suggested fix changes two lines in uninit-40.c.  What do you think should happen for this testcase?

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

[Jakub Jelinek]

On Fri, Feb 11, 2022 at 12:27:49PM -0500, Jason Merrill wrote:
> 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

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.

One question is if we could return something better for the TYPE_PTRMEMFUNC_FLAG
RECORD_TYPE members here (something that would print it more naturally/readably
in a C++ way), though the fact that the routine is in c-family makes it
harder.

Another one is whether we shouldn't punt for FIELD_DECLs that don't have
nicely printable name of its containing scope, something like:
		if (tree scope = get_containing_scope (field))
		  if (TYPE_P (scope) && TYPE_NAME (scope) == NULL_TREE)
		    break;
		return cop;
or so.
Note the returned cop is a COMPONENT_REF where the first argument has a
nicely printable type name (x with type sp), but sp's TYPE_MAIN_VARIANT
is the unnamed TYPE_PTRMEMFUNC_FLAG.  So another possibility would be if
we see such a problem for the FIELD_DECL's scope, check if TYPE_MAIN_VARIANT
of the first COMPONENT_REF's argument is equal to that scope and in that
case use TREE_TYPE of the first COMPONENT_REF's argument as the scope
instead.

	Jakub

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

[Jason Merrill]

On 3/15/22 08:32, Jakub Jelinek wrote:
> On Fri, Feb 11, 2022 at 12:27:49PM -0500, Jason Merrill wrote:
>> 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
> 
> 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.

> One question is if we could return something better for the TYPE_PTRMEMFUNC_FLAG
> RECORD_TYPE members here (something that would print it more naturally/readably
> in a C++ way), though the fact that the routine is in c-family makes it
> harder.
> 
> Another one is whether we shouldn't punt for FIELD_DECLs that don't have
> nicely printable name of its containing scope, something like:
> 		if (tree scope = get_containing_scope (field))
> 		  if (TYPE_P (scope) && TYPE_NAME (scope) == NULL_TREE)
> 		    break;
> 		return cop;
> or so.
> Note the returned cop is a COMPONENT_REF where the first argument has a
> nicely printable type name (x with type sp), but sp's TYPE_MAIN_VARIANT
> is the unnamed TYPE_PTRMEMFUNC_FLAG.  So another possibility would be if
> we see such a problem for the FIELD_DECL's scope, check if TYPE_MAIN_VARIANT
> of the first COMPONENT_REF's argument is equal to that scope and in that
> case use TREE_TYPE of the first COMPONENT_REF's argument as the scope
> instead.
> 
> 	Jakub
> 

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

[Jakub Jelinek]

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.
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).

	Jakub

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

[Jason Merrill]

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.

Jason

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

[Jakub Jelinek]

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

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

[Jason Merrill]

On 3/18/22 14:20, Jakub Jelinek wrote:
> 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.

One important point about the OP's testcase is that we're dealing with 
offset 0, which corresponds to both the PMF as a whole and its first 
element.  Since we're looking for a RECORD_TYPE, choosing to interpret 
the desired object as the (RECORD_TYPE) PMF as a whole seems like the 
better choice; being more flexible probably does make sense at non-0 
offsets.

Jason

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

[Jakub Jelinek]

On Fri, Mar 18, 2022 at 02:27:30PM -0400, Jason Merrill wrote:
> > 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.
> 
> One important point about the OP's testcase is that we're dealing with
> offset 0, which corresponds to both the PMF as a whole and its first
> element.  Since we're looking for a RECORD_TYPE, choosing to interpret the
> desired object as the (RECORD_TYPE) PMF as a whole seems like the better
> choice; being more flexible probably does make sense at non-0 offsets.

Even if we punt for 0 offsets in RECORD_TYPEs, we'd still run into
the ICE if we tried to access something overlapping the __delta member.
For 0 offsets and type puning, the question is what will help user more,
it is true we don't need to print those + offsetof(...) parts because it
is the start of it, on the other side the user might not know what is the
first member of the struct and printing that might sometimes help.

	Jakub

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

[Jason Merrill]

On 3/18/22 14:47, Jakub Jelinek wrote:
> On Fri, Mar 18, 2022 at 02:27:30PM -0400, Jason Merrill wrote:
>>> 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.
>>
>> One important point about the OP's testcase is that we're dealing with
>> offset 0, which corresponds to both the PMF as a whole and its first
>> element.  Since we're looking for a RECORD_TYPE, choosing to interpret the
>> desired object as the (RECORD_TYPE) PMF as a whole seems like the better
>> choice; being more flexible probably does make sense at non-0 offsets.
> 
> Even if we punt for 0 offsets in RECORD_TYPEs, we'd still run into
> the ICE if we tried to access something overlapping the __delta member.

Good point.  Your patch is OK, then.

> For 0 offsets and type puning, the question is what will help user more,
> it is true we don't need to print those + offsetof(...) parts because it
> is the start of it, on the other side the user might not know what is the
> first member of the struct and printing that might sometimes help.

Or it might not, as in this case.  Going with the enclosing class if 
none of the types match seems like a better default to me, but I guess 
let's not worry about it now.

Jason

[PATCH] c-family: Fix ICE in pp_cxx_unqualified_id, at cp/cxx-pretty-print.c:128 [PR101515]

[Jakub Jelinek]

On Tue, Mar 15, 2022 at 01:32:46PM +0100, Jakub Jelinek via Gcc-patches wrote:
> Another one is whether we shouldn't punt for FIELD_DECLs that don't have
> nicely printable name of its containing scope, something like:
> 		if (tree scope = get_containing_scope (field))
> 		  if (TYPE_P (scope) && TYPE_NAME (scope) == NULL_TREE)
> 		    break;
> 		return cop;
> or so.

This seems to work fine, ok if it passes bootstrap/regtest?
No need to do it for C because it doesn't print the contexts (has no syntax
for that even).

2022-03-16  Jakub Jelinek  <jakub@redhat.com>

	PR c++/101515
	* c-pretty-print.cc (c_fold_indirect_ref_for_warn): For C++ don't
	return COMPONENT_REFs with FIELD_DECLs whose containing scope can't
	be printed.

	* g++.dg/warn/pr101515.C: New test.

--- gcc/c-family/c-pretty-print.cc.jj	2022-02-14 13:14:39.037128630 +0100
+++ gcc/c-family/c-pretty-print.cc	2022-03-16 11:16:54.957658064 +0100
@@ -1889,6 +1889,12 @@ c_fold_indirect_ref_for_warn (location_t
 	      = wi::to_offset (TYPE_SIZE_UNIT (TREE_TYPE (field)));
 	    if (upos <= off && off < upos + el_sz)
 	      {
+		/* The C++ pretty printers print scope of the FIELD_DECLs,
+		   so punt if it is something that can't be printed.  */
+		if (c_dialect_cxx ())
+		  if (tree scope = get_containing_scope (field))
+		    if (TYPE_P (scope) && TYPE_NAME (scope) == NULL_TREE)
+		      break;
 		tree cop = build3_loc (loc, COMPONENT_REF, TREE_TYPE (field),
 				       op, field, NULL_TREE);
 		off = off - upos;
--- gcc/testsuite/g++.dg/warn/pr101515.C.jj	2022-03-16 11:19:09.004806778 +0100
+++ gcc/testsuite/g++.dg/warn/pr101515.C	2022-03-16 11:24:35.136302673 +0100
@@ -0,0 +1,18 @@
+// PR c++/101515
+// { dg-do compile }
+// { dg-options "-O1 -Wuninitialized" }
+
+struct S { int j; };
+struct T : public S { virtual void h () {} };
+struct U { void (*ptr) (); };
+typedef void (S::*sp) ();
+
+int
+main ()
+{
+  T t;
+  sp x;
+  U *xp = (U *) &x;
+  if (xp->ptr != ((void (*) ()) (sizeof (void *))))	// { dg-warning "is used uninitialized" }
+    return 1;
+}


	Jakub