> On Feb 11, 2022, at 3:54 PM, Jason Merrill <ja...@redhat.com> wrote:
>
> On 2/11/22 15:29, Qing Zhao wrote:
>>> On Feb 11, 2022, at 1:39 PM, Jason Merrill <ja...@redhat.com> wrote:
>>>
>>> On 2/11/22 13:11, Qing Zhao wrote:
>>>> Hi, Jason,
>>>>> On Feb 11, 2022, at 11:27 AM, Jason Merrill <ja...@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?
