[Bug analyzer/104434] New: Analyzer doesn't know about "pure" and "const" functions

[Bug analyzer/104434] New: Analyzer doesn't know about "pure" and "const" functions

[dmalcolm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104434

            Bug ID: 104434
           Summary: Analyzer doesn't know about "pure" and "const"
                    functions
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: analyzer
          Assignee: dmalcolm at gcc dot gnu.org
          Reporter: dmalcolm at gcc dot gnu.org
  Target Milestone: ---

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.

[Bug analyzer/104434] Analyzer doesn't know about "pure" and "const" functions

[dmalcolm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104434

--- Comment #1 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
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.

[Bug analyzer/104434] Analyzer doesn't know about "pure" and "const" functions

[dmalcolm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104434

--- Comment #2 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
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".

[Bug analyzer/104434] Analyzer doesn't know about "pure" and "const" functions

[dmalcolm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104434

--- Comment #3 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
OpenBLAS issue filed as https://github.com/xianyi/OpenBLAS/issues/3543
suggesting the use of __attribute__((const)) on LAPACKE_lsame.

[Bug analyzer/104434] Analyzer doesn't know about "pure" and "const" functions

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104434

--- Comment #4 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
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>

[Bug analyzer/104434] Analyzer doesn't know about "pure" and "const" functions

[dmalcolm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104434

David Malcolm <dmalcolm at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|UNCONFIRMED                 |RESOLVED

--- Comment #5 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
Should be fixed by the above commit for GCC 12.

[Bug analyzer/104434] Analyzer doesn't know about "pure" and "const" functions

[dmalcolm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104434

--- Comment #6 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
OpenBLAS commit adding __attribute__((const)) to the decl:
https://github.com/xianyi/OpenBLAS/commit/1c1ffb0591186e50311670369dee2cb450980d9a