Re: [PATCH] analyzer: Fix PR analyzer/101980

Re: [PATCH] analyzer: Fix PR analyzer/101980

[David Malcolm]

On Fri, 2021-08-20 at 21:55 +0530, Ankur Saini wrote:
> The patch fixes the test failures introduced by :
> 
> aef703cf982072427e74034f4c460a11c5e04b8e
> 1b34248527472496ca3fe2a07183beac8cf69041
> 
> Thanks 
> - Ankur

Thanks for fixing this.

The patch looks OK, apart from some minor whitespace issues - I think
it's using spaces rather than tabs, as columns aren't lining up as
expected in some places.  (does your code editor support visualizing
whitespace and support GNU indentation styles?).

Ideally these nits should be fixed - but assuming this passes bootstrap
it's OK to push to trunk (and don't try to fix indentation in places
where it's already broken; best to focus on fixing the test suite).

Dave

Re: [PATCH] analyzer: Fix PR analyzer/101980

[Martin Liška]

On 8/20/21 18:47, David Malcolm via Gcc-patches wrote:
> On Fri, 2021-08-20 at 21:55 +0530, Ankur Saini wrote:
>> The patch fixes the test failures introduced by :
>>
>> aef703cf982072427e74034f4c460a11c5e04b8e
>> 1b34248527472496ca3fe2a07183beac8cf69041
>>
>> Thanks
>> - Ankur
> 
> Thanks for fixing this.
> 
> The patch looks OK, apart from some minor whitespace issues - I think
> it's using spaces rather than tabs, as columns aren't lining up as
> expected in some places.  (does your code editor support visualizing
> whitespace and support GNU indentation styles?).
> 
> Ideally these nits should be fixed - but assuming this passes bootstrap
> it's OK to push to trunk (and don't try to fix indentation in places
> where it's already broken; best to focus on fixing the test suite).
> 
> Dave
> 
> 

Hello.

I noticed the patch leads to the following Clang warning:

build/gcc/analyzer/diagnostic-manager.cc:2108:21: warning: variable 'caller_var' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]

Is it something we should handle? Or a false positive?
Thanks,
Martin

Re: [PATCH] analyzer: Fix PR analyzer/101980

[Martin Liška]

On 8/23/21 10:10, Martin Liška wrote:
> On 8/20/21 18:47, David Malcolm via Gcc-patches wrote:
>> On Fri, 2021-08-20 at 21:55 +0530, Ankur Saini wrote:
>>> The patch fixes the test failures introduced by :
>>>
>>> aef703cf982072427e74034f4c460a11c5e04b8e
>>> 1b34248527472496ca3fe2a07183beac8cf69041
>>>
>>> Thanks
>>> - Ankur
>>
>> Thanks for fixing this.
>>
>> The patch looks OK, apart from some minor whitespace issues - I think
>> it's using spaces rather than tabs, as columns aren't lining up as
>> expected in some places.  (does your code editor support visualizing
>> whitespace and support GNU indentation styles?).
>>
>> Ideally these nits should be fixed - but assuming this passes bootstrap
>> it's OK to push to trunk (and don't try to fix indentation in places
>> where it's already broken; best to focus on fixing the test suite).
>>
>> Dave
>>
>>
> 
> Hello.
> 
> I noticed the patch leads to the following Clang warning:
> 
> build/gcc/analyzer/diagnostic-manager.cc:2108:21: warning: variable 'caller_var' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
> 
> Is it something we should handle? Or a false positive?
> Thanks,
> Martin

And it likely caused https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102020.

Martin

Re: [PATCH] analyzer: Fix PR analyzer/101980

[Ankur Saini]

> On 23-Aug-2021, at 2:20 PM, Martin Liška <mliska@suse.cz> wrote:
> 
> On 8/23/21 10:10, Martin Liška wrote:
>> On 8/20/21 18:47, David Malcolm via Gcc-patches wrote:
>>> On Fri, 2021-08-20 at 21:55 +0530, Ankur Saini wrote:
>>>> The patch fixes the test failures introduced by :
>>>> 
>>>> aef703cf982072427e74034f4c460a11c5e04b8e
>>>> 1b34248527472496ca3fe2a07183beac8cf69041
>>>> 
>>>> Thanks
>>>> - Ankur
>>> 
>>> Thanks for fixing this.
>>> 
>>> The patch looks OK, apart from some minor whitespace issues - I think
>>> it's using spaces rather than tabs, as columns aren't lining up as
>>> expected in some places.  (does your code editor support visualizing
>>> whitespace and support GNU indentation styles?).
>>> 
>>> Ideally these nits should be fixed - but assuming this passes bootstrap
>>> it's OK to push to trunk (and don't try to fix indentation in places
>>> where it's already broken; best to focus on fixing the test suite).
>>> 
>>> Dave
>>> 
>>> 
>> Hello.
>> I noticed the patch leads to the following Clang warning:
>> build/gcc/analyzer/diagnostic-manager.cc:2108:21: warning: variable 'caller_var' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
>> Is it something we should handle? Or a false positive?
>> Thanks,
>> Martin
> 
> And it likely caused https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102020.

Yes, you are right, there is a typo in in gcc/analyzer/diagnostic-manager.cc:2113 which should be changed to something like this :

- - -
diff --git a/gcc/analyzer/diagnostic-manager.cc b/gcc/analyzer/diagnostic-manager.cc
index 89b5d1e3c3c..77dda4d2768 100644
--- a/gcc/analyzer/diagnostic-manager.cc
+++ b/gcc/analyzer/diagnostic-manager.cc
@@ -2110,7 +2110,7 @@ diagnostic_manager::prune_for_sm_diagnostic (checker_path *path,
                    = cg_superedge.map_expr_from_callee_to_caller (callee_var,
                                                                    &expr);
                 else
-                  callee_var = callee_model->get_representative_tree (sval);
+                 caller_var = caller_model->get_representative_tree (sval);
               }
             else
              caller_var = caller_model->get_representative_tree (sval);
- - -

But maybe the fail is not due to this typo, as ideally the analyzer should not enter that else statement in this case.

I see DejaGnu reporting a failing test ( with excess errors ) at line 72 but no test for failing or passing "test for warnings” there, even though there is a {dg-warning "double-'free’”} on line 72.

Thank you
- Ankur

Re: [PATCH] analyzer: Fix PR analyzer/101980

[Ankur Saini]

update 

looks like the problem was with test itself, apparently Deja is quite strict when it comes to spaces and didn’t read the “dg-“ directive on line 72 as there wasn’t an extra space between the statement and the closing brace. 

Adding it seems to fix the issue 

- - -

diff --git a/gcc/testsuite/gcc.dg/analyzer/malloc-callbacks.c b/gcc/testsuite/gcc.dg/analyzer/malloc-callbacks.c
index 53c75fddf84..8820dddf923 100644
--- a/gcc/testsuite/gcc.dg/analyzer/malloc-callbacks.c
+++ b/gcc/testsuite/gcc.dg/analyzer/malloc-callbacks.c
@@ -69,7 +69,7 @@ int *test_5 (void)
 static void __attribute__((noinline))
 called_by_test_6a (void *ptr)
 {
-  free (ptr); /* { dg-warning "double-'free'"} */
+  free (ptr); /* { dg-warning "double-'free'" } */
 }
 
 static deallocator_t __attribute__((noinline))

- - -

Thanks 
- Ankur

Re: [PATCH] analyzer: Fix PR analyzer/101980

[Martin Liška]

On 8/23/21 13:04, Ankur Saini wrote:
> 
> 
>> On 23-Aug-2021, at 2:20 PM, Martin Liška <mliska@suse.cz> wrote:
>>
>> On 8/23/21 10:10, Martin Liška wrote:
>>> On 8/20/21 18:47, David Malcolm via Gcc-patches wrote:
>>>> On Fri, 2021-08-20 at 21:55 +0530, Ankur Saini wrote:
>>>>> The patch fixes the test failures introduced by :
>>>>>
>>>>> aef703cf982072427e74034f4c460a11c5e04b8e
>>>>> 1b34248527472496ca3fe2a07183beac8cf69041
>>>>>
>>>>> Thanks
>>>>> - Ankur
>>>>
>>>> Thanks for fixing this.
>>>>
>>>> The patch looks OK, apart from some minor whitespace issues - I think
>>>> it's using spaces rather than tabs, as columns aren't lining up as
>>>> expected in some places.  (does your code editor support visualizing
>>>> whitespace and support GNU indentation styles?).
>>>>
>>>> Ideally these nits should be fixed - but assuming this passes bootstrap
>>>> it's OK to push to trunk (and don't try to fix indentation in places
>>>> where it's already broken; best to focus on fixing the test suite).
>>>>
>>>> Dave
>>>>
>>>>
>>> Hello.
>>> I noticed the patch leads to the following Clang warning:
>>> build/gcc/analyzer/diagnostic-manager.cc:2108:21: warning: variable 'caller_var' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
>>> Is it something we should handle? Or a false positive?
>>> Thanks,
>>> Martin
>>
>> And it likely caused https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102020.
> 
> Yes, you are right, there is a typo in in gcc/analyzer/diagnostic-manager.cc:2113 which should be changed to something like this :
> 
> - - -
> diff --git a/gcc/analyzer/diagnostic-manager.cc b/gcc/analyzer/diagnostic-manager.cc
> index 89b5d1e3c3c..77dda4d2768 100644
> --- a/gcc/analyzer/diagnostic-manager.cc
> +++ b/gcc/analyzer/diagnostic-manager.cc
> @@ -2110,7 +2110,7 @@ diagnostic_manager::prune_for_sm_diagnostic (checker_path *path,
>                      = cg_superedge.map_expr_from_callee_to_caller (callee_var,
>                                                                      &expr);
>                   else
> -                  callee_var = callee_model->get_representative_tree (sval);
> +                 caller_var = caller_model->get_representative_tree (sval);
>                 }
>               else
>                caller_var = caller_model->get_representative_tree (sval);

Ok, please push it as obvious. Do you have a git account?

Cheers,
Martin

> - - -
> 
> But maybe the fail is not due to this typo, as ideally the analyzer should not enter that else statement in this case.
> 
> I see DejaGnu reporting a failing test ( with excess errors ) at line 72 but no test for failing or passing "test for warnings” there, even though there is a {dg-warning "double-'free’”} on line 72.

Yeah, there's missing space in between quote and the closing brace:

diff --git a/gcc/testsuite/gcc.dg/analyzer/malloc-callbacks.c b/gcc/testsuite/gcc.dg/analyzer/malloc-callbacks.c
index 53c75fddf84..8820dddf923 100644
--- a/gcc/testsuite/gcc.dg/analyzer/malloc-callbacks.c
+++ b/gcc/testsuite/gcc.dg/analyzer/malloc-callbacks.c
@@ -69,7 +69,7 @@ int *test_5 (void)
  static void __attribute__((noinline))
  called_by_test_6a (void *ptr)
  {
-  free (ptr); /* { dg-warning "double-'free'"} */
+  free (ptr); /* { dg-warning "double-'free'" } */
  }
  
  static deallocator_t __attribute__((noinline))

Using the patch, the test works.
Martin

> 
> Thank you
> - Ankur
> 

Re: [PATCH] analyzer: Fix PR analyzer/101980

[Ankur Saini]

> On 23-Aug-2021, at 5:00 PM, Martin Liška <mliska@suse.cz> wrote:
> 
> On 8/23/21 13:04, Ankur Saini wrote:
>>> On 23-Aug-2021, at 2:20 PM, Martin Liška <mliska@suse.cz> wrote:
>>> 
>>> On 8/23/21 10:10, Martin Liška wrote:
>>>> On 8/20/21 18:47, David Malcolm via Gcc-patches wrote:
>>>>> On Fri, 2021-08-20 at 21:55 +0530, Ankur Saini wrote:
>>>>>> The patch fixes the test failures introduced by :
>>>>>> 
>>>>>> aef703cf982072427e74034f4c460a11c5e04b8e
>>>>>> 1b34248527472496ca3fe2a07183beac8cf69041
>>>>>> 
>>>>>> Thanks
>>>>>> - Ankur
>>>>> 
>>>>> Thanks for fixing this.
>>>>> 
>>>>> The patch looks OK, apart from some minor whitespace issues - I think
>>>>> it's using spaces rather than tabs, as columns aren't lining up as
>>>>> expected in some places.  (does your code editor support visualizing
>>>>> whitespace and support GNU indentation styles?).
>>>>> 
>>>>> Ideally these nits should be fixed - but assuming this passes bootstrap
>>>>> it's OK to push to trunk (and don't try to fix indentation in places
>>>>> where it's already broken; best to focus on fixing the test suite).
>>>>> 
>>>>> Dave
>>>>> 
>>>>> 
>>>> Hello.
>>>> I noticed the patch leads to the following Clang warning:
>>>> build/gcc/analyzer/diagnostic-manager.cc:2108:21: warning: variable 'caller_var' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
>>>> Is it something we should handle? Or a false positive?
>>>> Thanks,
>>>> Martin
>>> 
>>> And it likely caused https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102020.
>> Yes, you are right, there is a typo in in gcc/analyzer/diagnostic-manager.cc:2113 which should be changed to something like this :
>> - - -
>> diff --git a/gcc/analyzer/diagnostic-manager.cc b/gcc/analyzer/diagnostic-manager.cc
>> index 89b5d1e3c3c..77dda4d2768 100644
>> --- a/gcc/analyzer/diagnostic-manager.cc
>> +++ b/gcc/analyzer/diagnostic-manager.cc
>> @@ -2110,7 +2110,7 @@ diagnostic_manager::prune_for_sm_diagnostic (checker_path *path,
>>                     = cg_superedge.map_expr_from_callee_to_caller (callee_var,
>>                                                                     &expr);
>>                  else
>> -                  callee_var = callee_model->get_representative_tree (sval);
>> +                 caller_var = caller_model->get_representative_tree (sval);
>>                }
>>              else
>>               caller_var = caller_model->get_representative_tree (sval);
> 
> Ok, please push it as obvious. Do you have a git account?

Thank you, pushed the changes as e7721590e08e6d87adc879f8d549557cbe2bb7bb

- Ankur