[RFC] Display inlining context for uninitialized warnings

Hi, 

This patch (partially) adds displaying inlining context for
-W{maybe,}uninitialized warnings.  This is not as trivial to enable as
simply supplying the "%G" format specifier, so I have some questions
below.

I need this hunk 

 void
 percent_K_format (text_info *text, location_t loc, tree block)
 {
-  text->set_location (0, loc, SHOW_RANGE_WITH_CARET);
+  if (loc != UNKNOWN_LOCATION)
+    text->set_location (0, loc, SHOW_RANGE_WITH_CARET);
  
for two reasons:

- The gimple return stmt has UNKNOWN_LOCATION due to this code in
  lower_gimple_return ():

  if (gimple_return_retval (stmt) == gimple_return_retval (tmp_rs.stmt))
    {
      /* Remove the line number from the representative return statement.
         It now fills in for many such returns.  Failure to remove this
         will result in incorrect results for coverage analysis.  */
      gimple_set_location (tmp_rs.stmt, UNKNOWN_LOCATION);

  (In case you are wondering, the code and the comment were
  added in 2004.)

- When percent_K_format is called, TEXT might already hold precise
  location information in case its (indirect) caller is
  warning_at/error_at.  So it seems to me, this function lacks
  distinguishing the two cases: being called from plain warning/error
  functions vs. their *_at counterparts (the location shouldn't be
  updated in the latter case).

  Martin, you did the necessary work for percent_G_format to accept
  arbitrary gimple statements rather than just calls for PR86650, but
  left the PR open.  Do you remember running into that sort of problem,
  or was it something else?
  
Sometimes inlining context is still lost (with the current patch), as
can be seen e.g. with a version of the test with 's/unsigned yo/int yo/'
substitution applied.  (The chain of block = BLOCK_SUPERCONTEXT (block)
is broken - it ends with 0 rather than a FUNCTION_DECL.)  Is this known
and expected (e.g. because pass_late_warn_uninitialized runs very late),
or something to be investigated and fixed?

The patch was bootstrapped and regtested on x86_64-pc-linux-gnu.  Shall
it be applied?

On 6/19/19 5:11 AM, Vladislav Ivanishin wrote:
> Hi,
> 
> This patch (partially) adds displaying inlining context for
> -W{maybe,}uninitialized warnings.  This is not as trivial to enable as
> simply supplying the "%G" format specifier, so I have some questions
> below.
> 
> I need this hunk
> 
>   void
>   percent_K_format (text_info *text, location_t loc, tree block)
>   {
> -  text->set_location (0, loc, SHOW_RANGE_WITH_CARET);
> +  if (loc != UNKNOWN_LOCATION)
> +    text->set_location (0, loc, SHOW_RANGE_WITH_CARET);
>    
> for two reasons:
> 
> - The gimple return stmt has UNKNOWN_LOCATION due to this code in
>    lower_gimple_return ():
> 
>    if (gimple_return_retval (stmt) == gimple_return_retval (tmp_rs.stmt))
>      {
>        /* Remove the line number from the representative return statement.
>           It now fills in for many such returns.  Failure to remove this
>           will result in incorrect results for coverage analysis.  */
>        gimple_set_location (tmp_rs.stmt, UNKNOWN_LOCATION);

This code is also causing quite a few non-trivial instances of
-Wreturn-local-addr to have no location after two (or more) such
statements have been merged (I raised PR 90735 for it), independent
of inlining.  I wonder if using the location of the closing curly
brace of the function definition (if it's available somewhere)
would work without messing up the coverage analysis.

> 
>    (In case you are wondering, the code and the comment were
>    added in 2004.)
> 
> - When percent_K_format is called, TEXT might already hold precise
>    location information in case its (indirect) caller is
>    warning_at/error_at.  So it seems to me, this function lacks
>    distinguishing the two cases: being called from plain warning/error
>    functions vs. their *_at counterparts (the location shouldn't be
>    updated in the latter case).
> 
>    Martin, you did the necessary work for percent_G_format to accept
>    arbitrary gimple statements rather than just calls for PR86650, but
>    left the PR open.  Do you remember running into that sort of problem,
>    or was it something else?

I'm not sure it was the same problem.  I described what I was seeing
when I posted the patch:
https://gcc.gnu.org/ml/gcc-patches/2018-07/msg01286.html
I think it's worth revisiting this with your patch in place.  It would
be nice to get the inlining context in place for -Warray-bounds too.
David Malcolm (CC'd) is the expert on locations and the diagnostic
subsystem so he will have a much better insight than me into all of
this than me.

Martin

>    
> Sometimes inlining context is still lost (with the current patch), as
> can be seen e.g. with a version of the test with 's/unsigned yo/int yo/'
> substitution applied.  (The chain of block = BLOCK_SUPERCONTEXT (block)
> is broken - it ends with 0 rather than a FUNCTION_DECL.)  Is this known
> and expected (e.g. because pass_late_warn_uninitialized runs very late),
> or something to be investigated and fixed?
> 
> The patch was bootstrapped and regtested on x86_64-pc-linux-gnu.  Shall
> it be applied?
> 
> 
>

On 6/19/19 8:57 AM, Martin Sebor wrote:
> On 6/19/19 5:11 AM, Vladislav Ivanishin wrote:
>> Hi,
>>
>> This patch (partially) adds displaying inlining context for
>> -W{maybe,}uninitialized warnings.  This is not as trivial to enable as
>> simply supplying the "%G" format specifier, so I have some questions
>> below.
>>
>> I need this hunk
>>
>>   void
>>   percent_K_format (text_info *text, location_t loc, tree block)
>>   {
>> -  text->set_location (0, loc, SHOW_RANGE_WITH_CARET);
>> +  if (loc != UNKNOWN_LOCATION)
>> +    text->set_location (0, loc, SHOW_RANGE_WITH_CARET);
>>    for two reasons:
>>
>> - The gimple return stmt has UNKNOWN_LOCATION due to this code in
>>    lower_gimple_return ():
>>
>>    if (gimple_return_retval (stmt) == gimple_return_retval (tmp_rs.stmt))
>>      {
>>        /* Remove the line number from the representative return
>> statement.
>>           It now fills in for many such returns.  Failure to remove this
>>           will result in incorrect results for coverage analysis.  */
>>        gimple_set_location (tmp_rs.stmt, UNKNOWN_LOCATION);
> 
> This code is also causing quite a few non-trivial instances of
> -Wreturn-local-addr to have no location after two (or more) such
> statements have been merged (I raised PR 90735 for it), independent
> of inlining.  I wonder if using the location of the closing curly
> brace of the function definition (if it's available somewhere)
> would work without messing up the coverage analysis.
> 
>>
>>    (In case you are wondering, the code and the comment were
>>    added in 2004.)
>>
>> - When percent_K_format is called, TEXT might already hold precise
>>    location information in case its (indirect) caller is
>>    warning_at/error_at.  So it seems to me, this function lacks
>>    distinguishing the two cases: being called from plain warning/error
>>    functions vs. their *_at counterparts (the location shouldn't be
>>    updated in the latter case).
>>
>>    Martin, you did the necessary work for percent_G_format to accept
>>    arbitrary gimple statements rather than just calls for PR86650, but
>>    left the PR open.  Do you remember running into that sort of problem,
>>    or was it something else?
> 
> I'm not sure it was the same problem.  I described what I was seeing
> when I posted the patch:
> https://gcc.gnu.org/ml/gcc-patches/2018-07/msg01286.html
> I think it's worth revisiting this with your patch in place.  It would
> be nice to get the inlining context in place for -Warray-bounds too.
> David Malcolm (CC'd) is the expert on locations and the diagnostic
> subsystem so he will have a much better insight than me into all of
> this than me.
You might also want to bring in Andreas K who added the original bits to
help with debug info generation and Eric B who extended that code in
2014 due to impacts on profiling.

https://gcc.gnu.org/ml/gcc-patches/2014-11/msg00769.html
https://gcc.gnu.org/ml/gcc-patches/2011-02/msg00421.html

Jeff

Hi,

I'm pinging <https://gcc.gnu.org/ml/gcc-patches/2019-06/msg01124.html>.

I think, there are two subtopics to it that can be discussed separately.
I would like to focus on the patch itself here.  I am going to also
start a subthread dedicated to dealing with representative returns.

I still have the same questions I had when I was sending the patch.
I am going to reiterate them, but in a more structured/focused fashion.

> When percent_K_format is called, TEXT might already hold precise
> location information in case its (indirect) caller is
> warning_at/error_at.  So it seems to me, this function lacks
> distinguishing the two cases: being called from plain warning/error
> functions vs. their *_at counterparts (the location shouldn't be
> updated in the latter case).

David, do you agree?  Should a discriminator of some sort be introduced?

> Sometimes inlining context is still lost (with the current patch), as
> can be seen e.g. with a version of the test with 's/unsigned yo/int yo/'
> substitution applied.  (The chain of block = BLOCK_SUPERCONTEXT (block)
> is broken - it ends with 0 rather than a FUNCTION_DECL.)  Is this known
> and expected (e.g. because pass_late_warn_uninitialized runs very late),
> or something to be investigated and fixed?

I don't know whom to address this question to.  I guess I'll just go
ahead and investigate if no expert shows up.

> The patch was bootstrapped and regtested on x86_64-pc-linux-gnu.
> Shall it be applied?

Addressing the two points above would make the solution more complete.
However, I think, the patch is still beneficial as-is, and it is not
hacky.  Re-tested, no regressions.

Jeff Law <law@redhat.com> writes:
> On 6/19/19 8:57 AM, Martin Sebor wrote:
>> On 6/19/19 5:11 AM, Vladislav Ivanishin wrote:
>>> Hi,
>>>
>>> This patch (partially) adds displaying inlining context for
>>> -W{maybe,}uninitialized warnings.  This is not as trivial to enable as
>>> simply supplying the "%G" format specifier, so I have some questions
>>> below.
>>>
>>> I need this hunk
>>>
>>>   void
>>>   percent_K_format (text_info *text, location_t loc, tree block)
>>>   {
>>> -  text->set_location (0, loc, SHOW_RANGE_WITH_CARET);
>>> +  if (loc != UNKNOWN_LOCATION)
>>> +    text->set_location (0, loc, SHOW_RANGE_WITH_CARET);
>>>    for two reasons:
>>>
>>> - The gimple return stmt has UNKNOWN_LOCATION due to this code in
>>>    lower_gimple_return ():
>>>
>>>    if (gimple_return_retval (stmt) == gimple_return_retval (tmp_rs.stmt))
>>>      {
>>>        /* Remove the line number from the representative return
>>> statement.
>>>           It now fills in for many such returns.  Failure to remove this
>>>           will result in incorrect results for coverage analysis.  */
>>>        gimple_set_location (tmp_rs.stmt, UNKNOWN_LOCATION);
>> 
>> This code is also causing quite a few non-trivial instances of
>> -Wreturn-local-addr to have no location after two (or more) such
>> statements have been merged (I raised PR 90735 for it), independent
>> of inlining.  I wonder if using the location of the closing curly
>> brace of the function definition (if it's available somewhere)
>> would work without messing up the coverage analysis.
>> 
>>>
>>>    (In case you are wondering, the code and the comment were
>>>    added in 2004.)
[... snip ...]
> You might also want to bring in Andreas K who added the original bits to
> help with debug info generation and Eric B who extended that code in
> 2014 due to impacts on profiling.
>
> https://gcc.gnu.org/ml/gcc-patches/2014-11/msg00769.html
> https://gcc.gnu.org/ml/gcc-patches/2011-02/msg00421.html

(Let's focus on location info and representative returns in this
subthread.)

AFAIU, a return statement may be chosen as a representative return for a
function.  The representative return's location is set to
UNKNOWN_LOCATION, because otherwise the results of coverage analysis are
skewed.  What is the difficulty preventing us from having both the
location info for the return and faithful coverage?

Andreas, Eric, would you have any comments?

> (Let's focus on location info and representative returns in this
> subthread.)
> 
> AFAIU, a return statement may be chosen as a representative return for a
> function.  The representative return's location is set to
> UNKNOWN_LOCATION, because otherwise the results of coverage analysis are
> skewed.  What is the difficulty preventing us from having both the
> location info for the return and faithful coverage?

That's simply impossible since you merge two instructions with different 
locations into a single one.  But the return location should be on the goto.