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