> 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” (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
