Bug 104434 - Analyzer doesn't know about "pure" and "const" functions
Summary: Analyzer doesn't know about "pure" and "const" functions
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: analyzer (show other bugs)
Version: 12.0
: P3 normal
Target Milestone: ---
Assignee: David Malcolm
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-02-07 22:43 UTC by David Malcolm
Modified: 2022-02-28 14:44 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description David Malcolm 2022-02-07 22:43:13 UTC
Consider:

extern int pure_p (int) __attribute__ ((pure));
extern void do_stuff (void);

void test (int a)
{
  void *p;
  if (pure_p (a))
    {
      p = __builtin_malloc (1024);
      if (!p)
	return;
    }
  do_stuff ();
  if (pure_p (a))
    __builtin_free (p);
}

Currently trunk -fanalyzer emits these warnings:

/tmp/foo.c: In function ‘test’:
/tmp/foo.c:14:6: warning: leak of ‘p’ [CWE-401] [-Wanalyzer-malloc-leak]
   14 |   if (pure_p (a))
      |      ^
  ‘test’: events 1-7
    |
    |    7 |   if (pure_p (a))
    |      |      ^
    |      |      |
    |      |      (1) following ‘true’ branch...
    |    8 |     {
    |    9 |       p = __builtin_malloc (1024);
    |      |           ~~~~~~~~~~~~~~~~~~~~~~~
    |      |           |
    |      |           (2) ...to here
    |      |           (3) allocated here
    |   10 |       if (!p)
    |      |          ~
    |      |          |
    |      |          (4) assuming ‘p’ is non-NULL
    |      |          (5) following ‘false’ branch (when ‘p’ is non-NULL)...
    |......
    |   13 |   do_stuff ();
    |      |   ~~~~~~~~~~~
    |      |   |
    |      |   (6) ...to here
    |   14 |   if (pure_p (a))
    |      |      ~
    |      |      |
    |      |      (7) following ‘false’ branch...
    |
  ‘test’: event 8
    |
    |cc1:
    | (8): ...to here
    |
  ‘test’: event 9
    |
    |   14 |   if (pure_p (a))
    |      |      ^
    |      |      |
    |      |      (9) ‘p’ leaks here; was allocated at (3)
    |
/tmp/foo.c:15:5: warning: use of uninitialized value ‘p’ [CWE-457] [-Wanalyzer-use-of-uninitialized-value]
   15 |     __builtin_free (p);
      |     ^~~~~~~~~~~~~~~~~~
  ‘test’: events 1-6
    |
    |    6 |   void *p;
    |      |         ^
    |      |         |
    |      |         (1) region created on stack here
    |    7 |   if (pure_p (a))
    |      |      ~   
    |      |      |
    |      |      (2) following ‘false’ branch...
    |......
    |   13 |   do_stuff ();
    |      |   ~~~~~~~~~~~
    |      |   |
    |      |   (3) ...to here
    |   14 |   if (pure_p (a))
    |      |      ~   
    |      |      |
    |      |      (4) following ‘true’ branch...
    |   15 |     __builtin_free (p);
    |      |     ~~~~~~~~~~~~~~~~~~
    |      |     |
    |      |     (5) ...to here
    |      |     (6) use of uninitialized value ‘p’ here
    |

by considering the execution paths where
  pure_p (a)
is true then false (the false leak diagnostic), and false then true (the false uninit diagnostic)

Presumably the analyzer should consider that the result of a pure/const function doesn't change, and thus only considers the true/true and false/false paths.
Comment 1 David Malcolm 2022-02-07 22:45:03 UTC
Seen on
https://github.com/xianyi/OpenBLAS/blob/c5f280a7f0e875d83833d895b2b8b0e341efabf4/lapack-netlib/LAPACKE/src/lapacke_cgbbrd_work.c
where the code has:

   if( LAPACKE_lsame( vect, 'b' ) || LAPACKE_lsame( vect, 'p' ) ) {
            pt_t = (lapack_complex_float*)
                LAPACKE_malloc( sizeof(lapack_complex_float) *
                                ldpt_t * MAX(1,n) );
      ...snip...
   }

   [...snip lots of code...]

   if( LAPACKE_lsame( vect, 'b' ) || LAPACKE_lsame( vect, 'p' ) ) {
            LAPACKE_free( pt_t );
   }

where the analyzer considers the execution path where the conditions
guarding the malloc and the free are first true, and then false.

LAPACKE_lsame is a case-insensitive comparison, implemented in its own
source file.  I think if it were marked as "pure", the analyzer could
fix this without needing LTO.
Comment 2 David Malcolm 2022-02-23 14:06:28 UTC
On rereading
  https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html
I think that "pure" isn't strong enough for the above example: the result of a pure function is allowed to change between invocations with the same inputs.  I think the function needs to be "const".
Comment 3 David Malcolm 2022-02-23 21:06:01 UTC
OpenBLAS issue filed as https://github.com/xianyi/OpenBLAS/issues/3543 suggesting the use of __attribute__((const)) on LAPACKE_lsame.
Comment 4 GCC Commits 2022-02-23 23:52:45 UTC
The master branch has been updated by David Malcolm <dmalcolm@gcc.gnu.org>:

https://gcc.gnu.org/g:aee1adf2cdc1cf4e116e5c05b6e7c92b0fbb264b

commit r12-7364-gaee1adf2cdc1cf4e116e5c05b6e7c92b0fbb264b
Author: David Malcolm <dmalcolm@redhat.com>
Date:   Wed Feb 23 09:14:58 2022 -0500

    analyzer: handle __attribute__((const)) [PR104434]
    
    When testing -fanalyzer on openblas-0.3, I noticed slightly over 2000
    false positives from -Wanalyzer-malloc-leak on code like this:
    
            if( LAPACKE_lsame( vect, 'b' ) || LAPACKE_lsame( vect, 'p' ) ) {
                pt_t = (lapack_complex_float*)
                    LAPACKE_malloc( sizeof(lapack_complex_float) *
                                    ldpt_t * MAX(1,n) );
                [...snip...]
            }
    
            [...snip lots of code...]
    
            if( LAPACKE_lsame( vect, 'b' ) || LAPACKE_lsame( vect, 'q' ) ) {
                LAPACKE_free( pt_t );
            }
    
    where LAPACKE_lsame is a char-comparison function implemented in a
    different TU.
    The analyzer naively considers the execution path where:
      LAPACKE_lsame( vect, 'b' ) || LAPACKE_lsame( vect, 'p' )
    is true at the malloc guard, but then false at the free guard, which
    is thus a memory leak.
    
    This patch makes -fanalyer respect __attribute__((const)), so that the
    analyzer treats such functions as returning the same value when given
    the same inputs.
    
    I've filed https://github.com/xianyi/OpenBLAS/issues/3543 suggesting that
    LAPACKE_lsame be annotated with __attribute__((const)); with that, and
    with this patch, the false positives seem to be fixed.
    
    gcc/analyzer/ChangeLog:
            PR analyzer/104434
            * analyzer.h (class const_fn_result_svalue): New decl.
            * region-model-impl-calls.cc (call_details::get_manager): New.
            * region-model-manager.cc
            (region_model_manager::get_or_create_const_fn_result_svalue): New.
            (region_model_manager::log_stats): Log
            m_const_fn_result_values_map.
            * region-model.cc (const_fn_p): New.
            (maybe_get_const_fn_result): New.
            (region_model::on_call_pre): Handle fndecls with
            __attribute__((const)) by calling the above rather than making
            a conjured_svalue.
            * region-model.h (visitor::visit_const_fn_result_svalue): New.
            (region_model_manager::get_or_create_const_fn_result_svalue): New
            decl.
            (region_model_manager::const_fn_result_values_map_t): New typedef.
            (region_model_manager::m_const_fn_result_values_map): New field.
            (call_details::get_manager): New decl.
            * svalue.cc (svalue::cmp_ptr): Handle SK_CONST_FN_RESULT.
            (const_fn_result_svalue::dump_to_pp): New.
            (const_fn_result_svalue::dump_input): New.
            (const_fn_result_svalue::accept): New.
            * svalue.h (enum svalue_kind): Add SK_CONST_FN_RESULT.
            (svalue::dyn_cast_const_fn_result_svalue): New.
            (class const_fn_result_svalue): New.
            (is_a_helper <const const_fn_result_svalue *>::test): New.
            (template <> struct default_hash_traits<const_fn_result_svalue::key_t>):
            New.
    
    gcc/testsuite/ChangeLog:
            PR analyzer/104434
            * gcc.dg/analyzer/attr-const-1.c: New test.
            * gcc.dg/analyzer/attr-const-2.c: New test.
            * gcc.dg/analyzer/attr-const-3.c: New test.
            * gcc.dg/analyzer/pr104434-const.c: New test.
            * gcc.dg/analyzer/pr104434-nonconst.c: New test.
            * gcc.dg/analyzer/pr104434.h: New test.
    
    Signed-off-by: David Malcolm <dmalcolm@redhat.com>
Comment 5 David Malcolm 2022-02-23 23:59:22 UTC
Should be fixed by the above commit for GCC 12.
Comment 6 David Malcolm 2022-02-28 14:44:59 UTC
OpenBLAS commit adding __attribute__((const)) to the decl:
https://github.com/xianyi/OpenBLAS/commit/1c1ffb0591186e50311670369dee2cb450980d9a