Hi,
I met another issue for “address taken” auto variable, see below for details:
**** the testing case: (gcc/testsuite/gcc.dg/uninit-16.c)
int foo, bar;
static
void decode_reloc(int reloc, int *is_alt)
{
if (reloc >= 20)
*is_alt = 1;
else if (reloc >= 10)
*is_alt = 0;
}
void testfunc()
{
int alt_reloc;
decode_reloc(foo, &alt_reloc);
if (alt_reloc) /* { dg-warning "may be used uninitialized" } */
bar = 42;
}
****When compiled with -ftrivial-auto-var-init=zero -O2 -Wuninitialized
-fdump-tree-all:
.*************gimple dump:
void testfunc ()
{
int alt_reloc;
try
{
_1 = .DEFERRED_INIT (4, 2, 0);
alt_reloc = _1;
foo.0_2 = foo;
decode_reloc (foo.0_2, &alt_reloc);
alt_reloc.1_3 = alt_reloc;
if (alt_reloc.1_3 != 0) goto <D.1952>; else goto <D.1953>;
<D.1952>:
bar = 42;
<D.1953>:
}
finally
{
alt_reloc = {CLOBBER};
}
}
**************fre1 dump:
void testfunc ()
{
int alt_reloc;
int _1;
int foo.0_2;
<bb 2> :
_1 = .DEFERRED_INIT (4, 2, 0);
foo.0_2 = foo;
if (foo.0_2 > 19)
goto <bb 3>; [50.00%]
else
goto <bb 4>; [50.00%]
<bb 3> :
goto <bb 7>; [100.00%]
<bb 4> :
if (foo.0_2 > 9)
goto <bb 5>; [50.00%]
else
goto <bb 6>; [50.00%]
<bb 5> :
goto <bb 8>; [100.00%]
<bb 6> :
if (_1 != 0)
goto <bb 7>; [INV]
else
goto <bb 8>; [INV]
<bb 7> :
bar = 42;
<bb 8> :
return;
}
From the above IR file after “FRE”, we can see that the major issue with this
IR is:
The address taken auto variable “alt_reloc” has been completely replaced by the
temporary variable “_1” in all
the uses of the original “alt_reloc”.
The major problem with such IR is, during uninitialized analysis phase, the
original use of “alt_reloc” disappeared completely.
So, the warning cannot be reported.
My questions:
1. Is it possible to get the original “alt_reloc” through the temporary
variable “_1” with some available information recorded in the IR?
2. If not, then we have to record the relationship between “alt_reloc” and “_1”
when the original “alt_reloc” is replaced by “_1” and get such relationship
during
Uninitialized analysis phase. Is this doable?
3. Looks like that for “address taken” auto variable, if we have to introduce a
new temporary variable and split the call to .DEFERRED_INIT into two:
temp = .DEFERRED_INIT (4, 2, 0);
alt_reloc = temp;
More issues might possible.
Any comments and suggestions on this issue?
Qing
j
> On Aug 11, 2021, at 11:55 AM, Richard Biener <[email protected]> wrote:
>
> On August 11, 2021 6:22:00 PM GMT+02:00, Qing Zhao <[email protected]>
> wrote:
>>
>>
>>> On Aug 11, 2021, at 10:53 AM, Richard Biener <[email protected]> wrote:
>>>
>>> On August 11, 2021 5:30:40 PM GMT+02:00, Qing Zhao <[email protected]>
>>> wrote:
>>>> I modified the routine “gimple_add_init_for_auto_var” as the following:
>>>> ====
>>>> /* Generate initialization to automatic variable DECL based on INIT_TYPE.
>>>> Build a call to internal const function DEFERRED_INIT:
>>>> 1st argument: SIZE of the DECL;
>>>> 2nd argument: INIT_TYPE;
>>>> 3rd argument: IS_VLA, 0 NO, 1 YES;
>>>>
>>>> as DEFERRED_INIT (SIZE of the DECL, INIT_TYPE, IS_VLA). */
>>>> static void
>>>> gimple_add_init_for_auto_var (tree decl,
>>>> enum auto_init_type init_type,
>>>> bool is_vla,
>>>> gimple_seq *seq_p)
>>>> {
>>>> gcc_assert (VAR_P (decl) && !DECL_EXTERNAL (decl) && !TREE_STATIC (decl));
>>>> gcc_assert (init_type > AUTO_INIT_UNINITIALIZED);
>>>> tree decl_size = TYPE_SIZE_UNIT (TREE_TYPE (decl));
>>>>
>>>> tree init_type_node
>>>> = build_int_cst (integer_type_node, (int) init_type);
>>>> tree is_vla_node
>>>> = build_int_cst (integer_type_node, (int) is_vla);
>>>>
>>>> tree call = build_call_expr_internal_loc (UNKNOWN_LOCATION,
>>>> IFN_DEFERRED_INIT,
>>>> TREE_TYPE (decl), 3,
>>>> decl_size, init_type_node,
>>>> is_vla_node);
>>>>
>>>> /* If this DECL is a VLA, a temporary address variable for it has been
>>>> created, the replacement for DECL is recorded in DECL_VALUE_EXPR (decl),
>>>> we should use it as the LHS of the call. */
>>>>
>>>> tree lhs_call
>>>> = is_vla ? DECL_VALUE_EXPR (decl) : decl;
>>>> gimplify_assign (lhs_call, call, seq_p);
>>>> }
>>>>
>>>> With this change, the current issue is resolved, the gimple dump now is:
>>>>
>>>> (*arr.1) = .DEFERRED_INIT (D.1952, 2, 1);
>>>>
>>>> However, there is another new issue:
>>>>
>>>> For the following testing case:
>>>>
>>>> ======
>>>> [opc@qinzhao-ol8u3-x86 gcc]$ cat t.c
>>>> int bar;
>>>>
>>>> extern void decode_reloc(int *);
>>>>
>>>> void testfunc()
>>>> {
>>>> int alt_reloc;
>>>>
>>>> decode_reloc(&alt_reloc);
>>>>
>>>> if (alt_reloc) /* { dg-warning "may be used uninitialized" } */
>>>> bar = 42;
>>>> }
>>>> =====
>>>>
>>>> In the above, the auto var “alt_reloc” is address taken, then the gimple
>>>> dump for it when compiled with -ftrivial-auto-var-init=zero is:
>>>>
>>>> void testfunc ()
>>>> {
>>>> int alt_reloc;
>>>>
>>>> try
>>>> {
>>>> _1 = .DEFERRED_INIT (4, 2, 0);
>>>> alt_reloc = _1;
>>>> decode_reloc (&alt_reloc);
>>>> alt_reloc.0_2 = alt_reloc;
>>>> if (alt_reloc.0_2 != 0) goto <D.1949>; else goto <D.1950>;
>>>> <D.1949>:
>>>> bar = 42;
>>>> <D.1950>:
>>>> }
>>>> finally
>>>> {
>>>> alt_reloc = {CLOBBER};
>>>> }
>>>> }
>>>>
>>>> I.e, instead of the expected IR:
>>>>
>>>> alt_reloc = .DEFERRED_INIT (4, 2, 0);
>>>>
>>>> We got the following:
>>>>
>>>> _1 = .DEFERRED_INIT (4, 2, 0);
>>>> alt_reloc = _1;
>>>>
>>>> I guess the temp “_1” is created because “alt_reloc” is address taken.
>>>
>>> Yes and no. The reason is that alt_reloc is memory (because it is address
>>> taken) and that GIMPLE says that register typed stores need to use a
>>> is_gimple_val RHS which the call is not.
>>
>> Okay.
>>>
>>>> My questions:
>>>>
>>>> Shall we accept such IR for .DEFERRED_INIT purpose when the auto var is
>>>> address taken?
>>>
>>> I think so. Note it doesn't necessarily need address taking but any other
>>> reason that prevents SSA rewriting the variable suffices.
>>
>> You mean, in addition to “address taken”, there are other situations that
>> will introduce such IR:
>>
>> temp = .DEFERRED_INIT();
>> auto_var = temp;
>>
>> So, such IR is unavoidable and we have to handle it?
>
> Yes.
>
>> If we have to handle it, what’ the best way to do it?
>>
>> The solution in my mind is:
>> 1. During uninitialized analysis phase, following the data flow to connect
>> .DEFERRED_INIT to “auto_var”, and then decide that “auto_var” is
>> uninitialized.
>
> Yes. Basically if there's an artificial variable auto initialized you have to
> look at its uses.
>
>> 2. During RTL expansion, following the data flow to connect .DEFERRED_INIT
>> to “auto_var”, and then delete “temp”, and then expand .DEFERRED_INIT to
>> auto_var.
>
> That shouldn't be necessary. You'd initialize a temporary register which is
> then copied to the real variable. That's good enough and should be optimized
> by the RTL pipeline.
>
>> Let me know your comments and suggestions on this.
>>
>>
>>>
>>> The only other option is to force. DEFERED_INIT making the LHS address
>>> taken which I think could be achieved by passing it the address as argument
>>> instead of having a LHS. But let's not go down this route - it will have
>>> quite bad behavior on alias analysis and optimization.
>>
>> Okay.
>>
>> Qing
>>>
>>>> If so, “uninitialized analysis” phase need to be further adjusted to
>>>> specially handle such IR.
>>>>
>>>> If not, what should we do when the auto var is address taken?
>>>>
>>>> Thanks a lot.
>>>>
>>>> Qing
>>>>
>>>>
>>>>> On Aug 11, 2021, at 8:58 AM, Richard Biener <[email protected]> wrote:
>>>>>
>>>>> On Wed, 11 Aug 2021, Qing Zhao wrote:
>>>>>
>>>>>>
>>>>>>
>>>>>>> On Aug 11, 2021, at 8:37 AM, Richard Biener <[email protected]> wrote:
>>>>>>>
>>>>>>> On Wed, 11 Aug 2021, Qing Zhao wrote:
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>> On Aug 11, 2021, at 2:02 AM, Richard Biener <[email protected]> wrote:
>>>>>>>>>
>>>>>>>>> On Tue, 10 Aug 2021, Qing Zhao wrote:
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> On Aug 10, 2021, at 3:16 PM, Qing Zhao via Gcc-patches
>>>>>>>>>>> <[email protected]> wrote:
>>>>>>>>>>>
>>>>>>>>>>> Hi, Richard,
>>>>>>>>>>>
>>>>>>>>>>>> On Aug 10, 2021, at 10:22 AM, Richard Biener <[email protected]>
>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Especially in the VLA case but likely also in general (though
>>>>>>>>>>>>>> unlikely
>>>>>>>>>>>>>> since usually the receiver of initializations are simple
>>>>>>>>>>>>>> enough). I'd
>>>>>>>>>>>>>> expect the VLA case end up as
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> *ptr_to_decl = .DEFERRED_INIT (...);
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> where *ptr_to_decl is the DECL_VALUE_EXPR of the decl.
>>>>>>>>>>>>>
>>>>>>>>>>>>> So, for the following small testing case:
>>>>>>>>>>>>>
>>>>>>>>>>>>> ====
>>>>>>>>>>>>> extern void bar (int);
>>>>>>>>>>>>>
>>>>>>>>>>>>> void foo(int n)
>>>>>>>>>>>>> {
>>>>>>>>>>>>> int arr[n];
>>>>>>>>>>>>> bar (arr[2]);
>>>>>>>>>>>>> return;
>>>>>>>>>>>>> }
>>>>>>>>>>>>> =====
>>>>>>>>>>>>>
>>>>>>>>>>>>> If I compile it with -ftrivial-auto-var-init=zero
>>>>>>>>>>>>> -fdump-tree-gimple -S -o auto-init-11.s -fdump-rtl-expand, the
>>>>>>>>>>>>> *.gimple dump is:
>>>>>>>>>>>>>
>>>>>>>>>>>>> =====
>>>>>>>>>>>>> void foo (int n)
>>>>>>>>>>>>> {
>>>>>>>>>>>>> int n.0;
>>>>>>>>>>>>> sizetype D.1950;
>>>>>>>>>>>>> bitsizetype D.1951;
>>>>>>>>>>>>> sizetype D.1952;
>>>>>>>>>>>>> bitsizetype D.1953;
>>>>>>>>>>>>> sizetype D.1954;
>>>>>>>>>>>>> int[0:D.1950] * arr.1;
>>>>>>>>>>>>> void * saved_stack.2;
>>>>>>>>>>>>> int arr[0:D.1950] [value-expr: *arr.1];
>>>>>>>>>>>>>
>>>>>>>>>>>>> saved_stack.2 = __builtin_stack_save ();
>>>>>>>>>>>>> try
>>>>>>>>>>>>> {
>>>>>>>>>>>>> n.0 = n;
>>>>>>>>>>>>> _1 = (long int) n.0;
>>>>>>>>>>>>> _2 = _1 + -1;
>>>>>>>>>>>>> _3 = (sizetype) _2;
>>>>>>>>>>>>> D.1950 = _3;
>>>>>>>>>>>>> _4 = (sizetype) n.0;
>>>>>>>>>>>>> _5 = (bitsizetype) _4;
>>>>>>>>>>>>> _6 = _5 * 32;
>>>>>>>>>>>>> D.1951 = _6;
>>>>>>>>>>>>> _7 = (sizetype) n.0;
>>>>>>>>>>>>> _8 = _7 * 4;
>>>>>>>>>>>>> D.1952 = _8;
>>>>>>>>>>>>> _9 = (sizetype) n.0;
>>>>>>>>>>>>> _10 = (bitsizetype) _9;
>>>>>>>>>>>>> _11 = _10 * 32;
>>>>>>>>>>>>> D.1953 = _11;
>>>>>>>>>>>>> _12 = (sizetype) n.0;
>>>>>>>>>>>>> _13 = _12 * 4;
>>>>>>>>>>>>> D.1954 = _13;
>>>>>>>>>>>>> arr.1 = __builtin_alloca_with_align (D.1954, 32);
>>>>>>>>>>>>> arr = .DEFERRED_INIT (D.1952, 2, 1);
>>>>>>>>>>>>> _14 = (*arr.1)[2];
>>>>>>>>>>>>> bar (_14);
>>>>>>>>>>>>> return;
>>>>>>>>>>>>> }
>>>>>>>>>>>>> finally
>>>>>>>>>>>>> {
>>>>>>>>>>>>> __builtin_stack_restore (saved_stack.2);
>>>>>>>>>>>>> }
>>>>>>>>>>>>> }
>>>>>>>>>>>>>
>>>>>>>>>>>>> ====
>>>>>>>>>>>>>
>>>>>>>>>>>>> You think that the above .DEFEERED_INIT is not correct?
>>>>>>>>>>>>> It should be:
>>>>>>>>>>>>>
>>>>>>>>>>>>> *arr.1 = .DEFERRED_INIT (D.1952. 2, 1);
>>>>>>>>>>>>>
>>>>>>>>>>>>> ?
>>>>>>>>>>>>
>>>>>>>>>>>> Yes.
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> I updated gimplify.c for VLA and now it emits the call to
>>>>>>>>>>> .DEFERRED_INIT as:
>>>>>>>>>>>
>>>>>>>>>>> arr.1 = __builtin_alloca_with_align (D.1954, 32);
>>>>>>>>>>> *arr.1 = .DEFERRED_INIT (D.1952, 2, 1);
>>>>>>>>>>>
>>>>>>>>>>> However, this call triggered the assertion failure in
>>>>>>>>>>> verify_gimple_call of tree-cfg.c because the LHS is not a valid
>>>>>>>>>>> LHS.
>>>>>>>>>>> Then I modify tree-cfg.c as:
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
>>>>>>>>>>> index 330eb7dd89bf..180d4f1f9e32 100644
>>>>>>>>>>> --- a/gcc/tree-cfg.c
>>>>>>>>>>> +++ b/gcc/tree-cfg.c
>>>>>>>>>>> @@ -3375,7 +3375,11 @@ verify_gimple_call (gcall *stmt)
>>>>>>>>>>> }
>>>>>>>>>>>
>>>>>>>>>>> tree lhs = gimple_call_lhs (stmt);
>>>>>>>>>>> + /* For .DEFERRED_INIT call, the LHS might be an indirection of
>>>>>>>>>>> + a pointer for the VLA variable, which is not a valid LHS of
>>>>>>>>>>> + a gimple call, we ignore the asssertion on this. */
>>>>>>>>>>> if (lhs
>>>>>>>>>>> + && (!gimple_call_internal_p (stmt, IFN_DEFERRED_INIT))
>>>>>>>>>>> && (!is_gimple_reg (lhs)
>>>>>>>>>>> && (!is_gimple_lvalue (lhs)
>>>>>>>>>>> || verify_types_in_gimple_reference
>>>>>>>>>>>
>>>>>>>>>>> The assertion failure in tree-cfg.c got resolved, but I got another
>>>>>>>>>>> assertion failure in operands_scanner::get_expr_operands (tree
>>>>>>>>>>> *expr_p, int flags), line 945:
>>>>>>>>>>>
>>>>>>>>>>> 939 /* If we get here, something has gone wrong. */
>>>>>>>>>>> 940 if (flag_checking)
>>>>>>>>>>> 941 {
>>>>>>>>>>> 942 fprintf (stderr, "unhandled expression in
>>>>>>>>>>> get_expr_operands():\n");
>>>>>>>>>>> 943 debug_tree (expr);
>>>>>>>>>>> 944 fputs ("\n", stderr);
>>>>>>>>>>> 945 gcc_unreachable ();
>>>>>>>>>>> 946 }
>>>>>>>>>>>
>>>>>>>>>>> Looks like that the gimple statement:
>>>>>>>>>>> *arr.1 = .DEFERRED_INIT (D.1952, 2, 1);
>>>>>>>>>>>
>>>>>>>>>>> Is not valid. i.e, the LHS should not be an indirection to a
>>>>>>>>>>> pointer.
>>>>>>>>>>>
>>>>>>>>>>> How to resolve this issue?
>>>>>>>>>
>>>>>>>>> It sounds like the LHS is an INDIRECT_REF maybe? That means it's
>>>>>>>>> still not properly gimplified because it should end up as a MEM_REF
>>>>>>>>> instead.
>>>>>>>>>
>>>>>>>>> But I'm just guessing here ... if you are in a debugger then you can
>>>>>>>>> invoke debug_tree (lhs) in the inferior to see what it exactly is
>>>>>>>>> at the point of the failure.
>>>>>>>>
>>>>>>>> Yes, it’s an INDIRECT_REF at the point of the failure even though I
>>>>>>>> added a
>>>>>>>>
>>>>>>>> gimplify_var_or_parm_decl (lhs)
>>>>>>>
>>>>>>> I think the easiest is to build the .DEFERRED_INIT as GENERIC
>>>>>>> and use gimplify_assign () to gimplify and add the result
>>>>>>> to the sequence. Thus, build a GENERIC CALL_EXPR and then
>>>>>>> gimplify_assign (lhs, call_expr, seq);
>>>>>>
>>>>>> Which utility routine is used to build an Internal generic call?
>>>>>> Currently, I used “gimple_build_call_internal” to build this internal
>>>>>> gimple call.
>>>>>>
>>>>>> For the generic call, shall I use “build_call_expr_loc” ?
>>>>>
>>>>> For example look at build_asan_poison_call_expr which does such thing
>>>>> for ASAN poison internal function call insertion at gimplification time.
>>>>>
>>>>> Richard.
>>>>>
>>>>>> Qing
>>>>>>
>>>>>>>
>>>>>>> Richard.
>>>>>>>
>>>>>>>> Qing
>>>>>>>>
>>>>>>>>>
>>>>>>>>>> I came up with the following solution:
>>>>>>>>>>
>>>>>>>>>> Define the IFN_DEFERRED_INIT function as:
>>>>>>>>>>
>>>>>>>>>> LHS = DEFERRED_INIT (SIZE of the DECL, INIT_TYPE, IS_VLA);
>>>>>>>>>>
>>>>>>>>>> if IS_VLA is false, the LHS is the DECL itself,
>>>>>>>>>> if IS_VLA is true, the LHS is the pointer to this DECL that created
>>>>>>>>>> by
>>>>>>>>>> gimplify_vla_decl.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> The benefit of this solution are:
>>>>>>>>>>
>>>>>>>>>> 1. Resolved the invalid IR issue;
>>>>>>>>>> 2. The call stmt carries the address of the VLA natually;
>>>>>>>>>>
>>>>>>>>>> The issue with this solution is:
>>>>>>>>>>
>>>>>>>>>> For VLA and non-VLA, the LHS will be different,
>>>>>>>>>>
>>>>>>>>>> Do you see any other potential issues with this solution?
>>>>>>>>>>
>>>>>>>>>> thanks.
>>>>>>>>>>
>>>>>>>>>> Qing
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> --
>>>>>>>>> Richard Biener <[email protected]>
>>>>>>>>> SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409
>>>>>>>>> Nuernberg,
>>>>>>>>> Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> Richard Biener <[email protected]>
>>>>>>> SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
>>>>>>> Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
>>>>>>
>>>>>>
>>>>>
>>>>> --
>>>>> Richard Biener <[email protected]>
>>>>> SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
>>>>> Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
