From: Jason Merrill <jason@redhat.com>
To: Qing Zhao <qing.zhao@oracle.com>, Jakub Jelinek <jakub@redhat.com>
Cc: gcc-patches Paul A Clarke via <gcc-patches@gcc.gnu.org>
Subject: Re: [PATCH] Fix PR 101515 (ICE in pp_cxx_unqualified_id, at cp/cxx-pretty-print.c:128)
Date: Fri, 11 Feb 2022 16:54:01 -0500 [thread overview]
Message-ID: <1af663c5-6451-06ec-8c98-7a22e6a83c42@redhat.com> (raw)
In-Reply-To: <58075E24-AD19-4B12-9550-DB0464F469DB@oracle.com>
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?
next prev parent reply other threads:[~2022-02-11 21:54 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-02-08 20:11 Qing Zhao
2022-02-08 22:20 ` Jason Merrill
2022-02-09 15:51 ` Qing Zhao
2022-02-09 18:23 ` Jason Merrill
2022-02-09 21:01 ` Qing Zhao
2022-02-10 2:49 ` Jason Merrill
2022-02-11 16:07 ` Qing Zhao
2022-02-11 17:27 ` Jason Merrill
2022-02-11 18:11 ` Qing Zhao
2022-02-11 19:39 ` Jason Merrill
2022-02-11 20:29 ` Qing Zhao
2022-02-11 21:54 ` Jason Merrill [this message]
2022-02-11 22:19 ` Qing Zhao
2022-03-15 12:32 ` Jakub Jelinek
2022-03-15 15:57 ` Jason Merrill
2022-03-15 16:06 ` Jakub Jelinek
2022-03-18 17:35 ` Jason Merrill
2022-03-18 18:20 ` Jakub Jelinek
2022-03-18 18:27 ` Jason Merrill
2022-03-18 18:47 ` Jakub Jelinek
2022-03-19 5:32 ` Jason Merrill
2022-03-16 10:29 ` [PATCH] c-family: Fix ICE in pp_cxx_unqualified_id, at cp/cxx-pretty-print.c:128 [PR101515] Jakub Jelinek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1af663c5-6451-06ec-8c98-7a22e6a83c42@redhat.com \
--to=jason@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jakub@redhat.com \
--cc=qing.zhao@oracle.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).