> On Jan 14, 2022, at 12:58 PM, Martin Sebor <mse...@gmail.com> wrote:
>
> On 1/14/22 11:29, Qing Zhao wrote:
>>> On Jan 14, 2022, at 12:11 PM, Martin Sebor <mse...@gmail.com> wrote:
>>>
>>> On 1/14/22 09:30, Qing Zhao wrote:
>>>>> On Jan 14, 2022, at 6:45 AM, Richard Biener <richard.guent...@gmail.com>
>>>>> wrote:
>>>>>
>>>>> On Thu, Jan 13, 2022 at 2:45 AM Qing Zhao <qing.z...@oracle.com> wrote:
>>>>>>
>>>>>> Hi, Richard,
>>>>>>
>>>>>> This is the updated version for the second patch, which is mainly the
>>>>>> change for "Enable -Wuninitialized + -ftrivial-auto-var-init for
>>>>>> address taken variables”.
>>>>>>
>>>>>> In this update, I mainly made the following change:
>>>>>>
>>>>>> 1. Delete “repl_var”, use the var_def_stmt, i.e, the call to
>>>>>> .DEFERRED_INIT to record the warning suppressed info.
>>>>>> 2. Add and change the comments in multiple places to make the change
>>>>>> more readable.
>>>>>>
>>>>>> Now, for the deleted variable, we will get the necessary info to report
>>>>>> warning from the call to .DEFERRED_INIT.
>>>>>>
>>>>>> A. the name string of DECL from the 3rd parameter of the call;
>>>>>> B. the location of the DECL from the location of the call;
>>>>>> C. the call can also be used to hold the information on whether the
>>>>>> warning
>>>>>> has been issued or not to suppress warning messages when needed;
>>>>>>
>>>>>> Please see the detailed description below for the problem and solution
>>>>>> of this patch.
>>>>>>
>>>>>> This patch has been bootstrapped and regressing tested on both X86 and
>>>>>> aarch64.
>>>>>>
>>>>>> Okay for GCC12?
>>>>>
>>>>> I think the change to split "%qD is used uninitialized" is bad for i8n
>>>>> though it seems
>>>>> like none of the strings passed to warn_uninit are currently marked
>>>>> for localization.
>>>>> I suppose there's lazy matching with the exact same strings passed to
>>>>> warning_at around like 641 but after your change those will no longer
>>>>> match up,
>>>> Silly question: What does the above “641” mean?
>>>
>>> At around line 641 :)
>> I thought of this, but I didn’t find a meaningful statement around line 641
>> in tree-ssa-uninit.c…
>> 636 found_alloc = true;
>> 637 break;
>> 638 }
>> 639
>> 640 if (!is_gimple_assign (def_stmt))
>> 641 break;
>> 642
>> 643 tree_code code = gimple_assign_rhs_code (def_stmt);
>> 644 if (code != ADDR_EXPR && code != POINTER_PLUS_EXPR)
>> 645 break;
>>
>>>
>>>>> at least for the "%qs ..." case constructed. I think a better way
>>>>> (for i8n) would be
>>>>> to pass down a flag whether it is "may" or "is" and have the full
>>>>> translatable
>>>>> strings literally passed to warning_at at the expense of some code
>>>>> duplication.
>>>>> Actually the extra flag is unnecessary, the OPT_W... we pass is already
>>>>> fully
>>>>> specifying that.
>>>> The only issue with the above change is: among the 4 calls to
>>>> “warn_uninit” as following:
>>>> if (use_stmt)
>>>> warn_uninit (OPT_Wuninitialized, def, SSA_NAME_VAR (def),
>>>> - "%qD is used uninitialized", use_stmt);
>>>> + "is used uninitialized", use_stmt);
>>>> }
>>>> }
>>>> @@ -932,10 +1015,10 @@ warn_uninitialized_vars (bool wmaybe_uninit)
>>>> tree use = USE_FROM_PTR (use_p);
>>>> if (wlims.always_executed)
>>>> warn_uninit (OPT_Wuninitialized, use, SSA_NAME_VAR (use),
>>>> - "%qD is used uninitialized", stmt);
>>>> + "is used uninitialized", stmt);
>>>> else if (wmaybe_uninit)
>>>> warn_uninit (OPT_Wmaybe_uninitialized, use, SSA_NAME_VAR
>>>> (use),
>>>> - "%qD may be used uninitialized", stmt);
>>>> + "may be used uninitialized", stmt);
>>>> }
>>>> /* For limiting the alias walk below we count all
>>>> @@ -1182,7 +1265,7 @@ warn_uninitialized_phi (gphi *phi, vec<gphi *>
>>>> *worklist,
>>>> warn_uninit (OPT_Wmaybe_uninitialized, uninit_op,
>>>> SSA_NAME_VAR (uninit_op),
>>>> - "%qD may be used uninitialized in this function",
>>>> + "may be used uninitialized in this function",
>>>> uninit_use_stmt, loc);
>>>> All the strings passed map well with the OPT_W… except the last one, since
>>>> the last one has an additional string “in this function” at the end.
>>>> However, since the last call has the last argument “loc” been NULL, maybe
>>>> we can use this to distinguish.
>>>
>>> Now that diagnostics are prefixed by something like "In function 'foo':
>>> the "in this function" part is superfluous and could be removed from
>>> all warning messages.
>> Okay, so, you mean that it’s safe to just remove “in this function” from the
>> last callsite?
>
> Yes, I would remove it. It might require some cleanup in the test
> suite but I wouldn't expect it to be too bad.
Okay, I see.
I didn’t see any test case regression due to this change.
>
>>>
>>> When there's no location (i.e., loc is UNKNOWN_LOCATION) the called
>>> code tries to derive a location from the context. When it can't, it
>>> won't point anywhere useful, so if that case ever comes up here it
>>> should probably be handled by using the location of the curly brace
>>> closing the function definition.
>> You mean the following in the routine warn_uninit:
>> /* Use either the location of the read statement or that of the PHI
>> argument, or that of the uninitialized variable, in that order,
>> whichever is valid. */
>> location_t location = UNKNOWN_LOCATION;
>> if (gimple_has_location (context))
>> location = gimple_location (context);
>> else if (phi_arg_loc != UNKNOWN_LOCATION)
>> location = phi_arg_loc;
>> else if (var)
>> location = DECL_SOURCE_LOCATION (var);
>> else if (var_name_str)
>> location = gimple_location (var_def_stmt);
>> When all the above failed, we could add the “location of the curly brace
>> closing the function definition”?
>
> Yes, that's what I meant. But to be clear, I didn't mean to suggest
> you make the change as part of this change It was just a side note.
> Sorry for not being clearer.
Okay.
Thanks a lot.
Qing
>
> Martin
>
>
>> Qing
>>>
>>> Martin
>>>
>>>>>
>>>>> Instead of doing
>>>>>
>>>>> + if (gimple_call_internal_p (var_def_stmt, IFN_DEFERRED_INIT))
>>>>> + {
>>>>> + /* Ignore the call to .DEFERRED_INIT that define the original
>>>>> + var itself as the following case:
>>>>> + temp = .DEFERRED_INIT (4, 2, â~@~\alt_reloc");
>>>>> + alt_reloc = temp;
>>>>> + In order to avoid generating warning for the fake usage
>>>>> + at alt_reloc = temp.
>>>>> ...
>>>>>
>>>>> I thought of many options, none really very appealing so I guess we have
>>>>> to
>>>>> go with this for now.
>>>> I agree with you, this is really ugly, I am not very comfortable myself
>>>> either. But looks like no better way to resolve it….
>>>>>
>>>>> So OK with the i8n thing sorted out - please post one hopefully last
>>>>> update
>>>>> for the patch.
>>>> Will do it.
>>>>>
>>>>> Thanks for your patience,
>>>> Thank you!
>>>> Qing
>>>>> Richard.
>>>>>
>>>>>> thanks.
>>>>>>
>>>>>> Qing.
>>>>>>
>>>>>> ========================
>>>>>> Enable -Wuninitialized + -ftrivial-auto-var-init for address
>>>>>> taken variables.
>>>>>>
>>>>>> With -ftrivial-auto-var-init, the address taken auto variable is
>>>>>> replaced with
>>>>>> a temporary variable during gimplification, and the original auto
>>>>>> variable might
>>>>>> be eliminated by compiler optimization completely. As a result, the
>>>>>> current
>>>>>> uninitialized warning analysis cannot get enough information from the IR,
>>>>>> therefore the uninitialized warnings for address taken variable cannot be
>>>>>> issued based on the current implemenation of -ftrival-auto-var-init.
>>>>>>
>>>>>> For more info please refer to:
>>>>>> https://gcc.gnu.org/pipermail/gcc-patches/2021-August/577431.html
>>>>>>
>>>>>> In order to improve this situation, we can improve uninitialized analysis
>>>>>> for address taken auto variables with -ftrivial-auto-var-init as
>>>>>> following:
>>>>>>
>>>>>> for the following stmt:
>>>>>>
>>>>>> _1 = .DEFERRED_INIT (4, 2, &"alt_reloc"[0]);
>>>>>> if (_1 != 0)
>>>>>>
>>>>>> The original variable DECL has been eliminated from the IR, all the
>>>>>> necessary
>>>>>> information that is needed for reporting the warnings for DECL can be
>>>>>> acquired
>>>>>> from the call to .DEFERRED_INIT.
>>>>>>
>>>>>> A. the name string of DECL from the 3rd parameter of the call;
>>>>>> B. the location of the DECL from the location of the call;
>>>>>> C. the call can also be used to hold the information on whether the
>>>>>> warning
>>>>>> has been issued or not to suppress warning messages when needed;
>>>>>>
>>>>>> The current testing cases for uninitialized warnings +
>>>>>> -ftrivial-auto-var-init
>>>>>> are adjusted to reflect the fact that we can issue warnings for address
>>>>>> taken
>>>>>> variables.
>>>>>>
>>>>>> gcc/ChangeLog:
>>>>>>
>>>>>> 2022-01-12 qing zhao <qing.z...@oracle.com>
>>>>>>
>>>>>> * tree-ssa-uninit.c (warn_uninit): Handle .DEFERRED_INIT call
>>>>>> with an
>>>>>> anonymous SSA_NAME specially.
>>>>>> (check_defs): Likewise.
>>>>>> (warn_uninit_phi_uses): Adjust the message format for warn_uninit.
>>>>>> (warn_uninitialized_vars): Likewise.
>>>>>> (warn_uninitialized_phi): Likewise
>>>>>>
>>>>>> gcc/testsuite/ChangeLog:
>>>>>>
>>>>>> 2022-01-12 qing zhao <qing.z...@oracle.com>
>>>>>>
>>>>>> * gcc.dg/auto-init-uninit-16.c (testfunc): Delete xfail to reflect
>>>>>> the fact that address taken variable can be warned.
>>>>>> * gcc.dg/auto-init-uninit-34.c (warn_scalar_1): Likewise.
>>>>>> (warn_scalar_2): Likewise.
>>>>>> * gcc.dg/auto-init-uninit-37.c (T1): Likewise.
>>>>>> (T2): Likewise.
>>>>>> * gcc.dg/auto-init-uninit-B.c (baz): Likewise.
>>>>>>
>>>>>> The complete patch is attached:
