> 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?
>
> 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”?
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:
>
