[Bug analyzer/105962] New: Unhelpful diagnostics paths from analyzer in the face of inlining

[Bug analyzer/105962] New: Unhelpful diagnostics paths from analyzer in the face of inlining

[dmalcolm at gcc dot gnu.org]

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

            Bug ID: 105962
           Summary: Unhelpful diagnostics paths from analyzer in the face
                    of inlining
           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 this double-free:


void foo (void *p)
{
  __builtin_free (p);
}

void bar (void *q)
{
  foo (q);
  foo (q);
}


Below -O2, -fanalyzer shows the calls and returns:

../../src/gcc/testsuite/gcc.dg/analyzer/inline-1.c: In function ‘foo’:
../../src/gcc/testsuite/gcc.dg/analyzer/inline-1.c:3:3: warning: double-‘free’
of ‘p’ [CWE-415] [-Wanalyzer-double-free]
    3 |   __builtin_free (p);
      |   ^~~~~~~~~~~~~~~~~~
  ‘bar’: events 1-2
    |
    |    6 | void bar (void *q)
    |      |      ^~~
    |      |      |
    |      |      (1) entry to ‘bar’
    |    7 | {
    |    8 |   foo (q);
    |      |   ~~~~~~~
    |      |   |
    |      |   (2) calling ‘foo’ from ‘bar’
    |
    +--> ‘foo’: events 3-4
           |
           |    1 | void foo (void *p)
           |      |      ^~~
           |      |      |
           |      |      (3) entry to ‘foo’
           |    2 | {
           |    3 |   __builtin_free (p);
           |      |   ~~~~~~~~~~~~~~~~~~
           |      |   |
           |      |   (4) first ‘free’ here
           |
    <------+
    |
  ‘bar’: events 5-6
    |
    |    8 |   foo (q);
    |      |   ^~~~~~~
    |      |   |
    |      |   (5) returning to ‘bar’ from ‘foo’
    |    9 |   foo (q);
    |      |   ~~~~~~~
    |      |   |
    |      |   (6) passing freed pointer ‘q’ in call to ‘foo’ from ‘bar’
    |
    +--> ‘foo’: events 7-8
           |
           |    1 | void foo (void *p)
           |      |      ^~~
           |      |      |
           |      |      (7) entry to ‘foo’
           |    2 | {
           |    3 |   __builtin_free (p);
           |      |   ~~~~~~~~~~~~~~~~~~
           |      |   |
           |      |   (8) second ‘free’ here; first ‘free’ was at (4)
           |


but at -O2 and above, we show the unhelpful:

In function ‘foo’,
    inlined from ‘bar’ at
../../src/gcc/testsuite/gcc.dg/analyzer/inline-1.c:9:3:
../../src/gcc/testsuite/gcc.dg/analyzer/inline-1.c:3:3: warning: double-‘free’
of ‘q’ [CWE-415] [-Wanalyzer-double-free]
    3 |   __builtin_free (p);
      |   ^~~~~~~~~~~~~~~~~~
  ‘bar’: events 1-2
    |
    |    3 |   __builtin_free (p);
    |      |   ^~~~~~~~~~~~~~~~~~
    |      |   |
    |      |   (1) first ‘free’ here
    |      |   (2) second ‘free’ here; first ‘free’ was at (1)

which is somewhat mystifying.

This is happening because -fanalyzer is running after inlining and sees this at
the gimple level:

void bar (void * q)
{
  <bb 2> [local count: 1073741824]:
  __builtin_free (q_2(D));
  __builtin_free (q_2(D));
  return;

}

We can probably improve the readability by fixing up the events in the
diagnostic path based on the LOCATION_BLOCK inlining information captured in
the stmt location values.

I tried a version of this years ago in:
 
https://dmalcolm.fedorapeople.org/gcc/2020-02-20/gcc-newgit-analyzer-gcc10-analysis-gcc10-v52-relative-to-bc0f8df124f6ee12c82c5a6c1335868a15bcaecb/0012-FIXME-WIP-on-inlining-and-paths.patch

[Bug analyzer/105962] Unhelpful diagnostics paths from analyzer in the face of inlining

[dmalcolm at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2022-06-15

--- Comment #1 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
I'm testing a patch for this

[Bug analyzer/105962] Unhelpful diagnostics paths from analyzer in the face of inlining

[cvs-commit at gcc dot gnu.org]

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

--- Comment #2 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:63c073199491b7ec2261d39af51c02147c2f0daf

commit r13-1119-g63c073199491b7ec2261d39af51c02147c2f0daf
Author: David Malcolm <dmalcolm@redhat.com>
Date:   Wed Jun 15 17:44:14 2022 -0400

    analyzer: fix up paths for inlining (PR analyzer/105962)

    -fanalyzer runs late compared to other code analysis tools, in that in
    runs on the partially-optimized gimple-ssa representation.  I chose this
    point to run in the hope of easy integration with LTO.

    As PR analyzer/105962 notes, this means that function inlining can occur
    before the -fanalyzer "sees" the user's code.  For example given:

    void foo (void *p)
    {
      __builtin_free (p);
    }

    void bar (void *q)
    {
      foo (q);
      foo (q);
    }

    Below -O2, -fanalyzer shows the calls and returns:

    inline-1.c: In function âfooâ:
    inline-1.c:3:3: warning: double-âfreeâ of âpâ [CWE-415]
[-Wanalyzer-double-free]
        3 |   __builtin_free (p);
          |   ^~~~~~~~~~~~~~~~~~
      âbarâ: events 1-2
        |
        |    6 | void bar (void *q)
        |      |      ^~~
        |      |      |
        |      |      (1) entry to âbarâ
        |    7 | {
        |    8 |   foo (q);
        |      |   ~~~~~~~
        |      |   |
        |      |   (2) calling âfooâ from âbarâ
        |
        +--> âfooâ: events 3-4
               |
               |    1 | void foo (void *p)
               |      |      ^~~
               |      |      |
               |      |      (3) entry to âfooâ
               |    2 | {
               |    3 |   __builtin_free (p);
               |      |   ~~~~~~~~~~~~~~~~~~
               |      |   |
               |      |   (4) first âfreeâ here
               |
        <------+
        |
      âbarâ: events 5-6
        |
        |    8 |   foo (q);
        |      |   ^~~~~~~
        |      |   |
        |      |   (5) returning to âbarâ from âfooâ
        |    9 |   foo (q);
        |      |   ~~~~~~~
        |      |   |
        |      |   (6) passing freed pointer âqâ in call to âfooâ from
âbarâ
        |
        +--> âfooâ: events 7-8
               |
               |    1 | void foo (void *p)
               |      |      ^~~
               |      |      |
               |      |      (7) entry to âfooâ
               |    2 | {
               |    3 |   __builtin_free (p);
               |      |   ~~~~~~~~~~~~~~~~~~
               |      |   |
               |      |   (8) second âfreeâ here; first âfreeâ was at
(4)
               |

    but at -O2, -fanalyzer "sees" this gimple:

    void bar (void * q)
    {
      <bb 2> [local count: 1073741824]:
      __builtin_free (q_2(D));
      __builtin_free (q_2(D));
      return;
    }

    where "foo" has been inlined away, leading to this unhelpful output:

    In function âfooâ,
        inlined from âbarâ at inline-1.c:9:3:
    inline-1.c:3:3: warning: double-âfreeâ of âqâ [CWE-415]
[-Wanalyzer-double-free]
        3 |   __builtin_free (p);
          |   ^~~~~~~~~~~~~~~~~~
      âbarâ: events 1-2
        |
        |    3 |   __builtin_free (p);
        |      |   ^~~~~~~~~~~~~~~~~~
        |      |   |
        |      |   (1) first âfreeâ here
        |      |   (2) second âfreeâ here; first âfreeâ was at (1)

    where the stack frame information in the execution path suggests that these
    events are happening in "bar", in the top stack frame.

    This is what the analyzer sees, but I find it hard to decipher such
    output.  Hence, as a workaround for the fact that -fanalyzer runs so
    late, this patch attempts to reconstruct the "true" stack frame
    information, and to inject events showing inline calls, based on the
    inlining chain information recorded in the location_t values for the
events.

    Doing so leads to this output at -O2 on the above example (with
    -fdiagnostics-show-path-depths):

    In function âfooâ,
        inlined from âbarâ at inline-1.c:9:3:
    inline-1.c:3:3: warning: double-âfreeâ of âqâ [CWE-415]
[-Wanalyzer-double-free]
        3 |   __builtin_free (p);
          |   ^~~~~~~~~~~~~~~~~~
      âbarâ: events 1-2 (depth 1)
        |
        |    6 | void bar (void *q)
        |      |      ^~~
        |      |      |
        |      |      (1) entry to âbarâ
        |    7 | {
        |    8 |   foo (q);
        |      |   ~
        |      |   |
        |      |   (2) inlined call to âfooâ from âbarâ
        |
        +--> âfooâ: event 3 (depth 2)
               |
               |    3 |   __builtin_free (p);
               |      |   ^~~~~~~~~~~~~~~~~~
               |      |   |
               |      |   (3) first âfreeâ here
               |
        <------+
        |
      âbarâ: event 4 (depth 1)
        |
        |    9 |   foo (q);
        |      |   ^
        |      |   |
        |      |   (4) inlined call to âfooâ from âbarâ
        |
        +--> âfooâ: event 5 (depth 2)
               |
               |    3 |   __builtin_free (p);
               |      |   ^~~~~~~~~~~~~~~~~~
               |      |   |
               |      |   (5) second âfreeâ here; first âfreeâ was at
(3)
               |

    reconstructing the calls and returns.

    The patch also adds a new option, -fno-analyzer-undo-inlining, which can
    be used to disable this reconstruction, restoring the output listed
    above (this time with -fdiagnostics-show-path-depths):

    In function âfooâ,
        inlined from âbarâ at inline-1.c:9:3:
    inline-1.c:3:3: warning: double-âfreeâ of âqâ [CWE-415]
[-Wanalyzer-double-free]
        3 |   __builtin_free (p);
          |   ^~~~~~~~~~~~~~~~~~
      âbarâ: events 1-2 (depth 1)
        |
        |    3 |   __builtin_free (p);
        |      |   ^~~~~~~~~~~~~~~~~~
        |      |   |
        |      |   (1) first âfreeâ here
        |      |   (2) second âfreeâ here; first âfreeâ was at (1)
        |

    gcc/analyzer/ChangeLog:
            PR analyzer/105962
            * analyzer.opt (fanalyzer-undo-inlining): New option.
            * checker-path.cc: Include "diagnostic-core.h" and
            "inlining-iterator.h".
            (event_kind_to_string): Handle EK_INLINED_CALL.
            (class inlining_info): New class.
            (checker_event::checker_event): Move here from checker-path.h.
            Store original fndecl and depth, and calculate effective fndecl
            and depth based on inlining information.
            (checker_event::dump): Emit original depth as well as effective
            depth when they differ; likewise for fndecl.
            (region_creation_event::get_desc): Use m_effective_fndecl.
            (inlined_call_event::get_desc): New.
            (inlined_call_event::get_meaning): New.
            (checker_path::inject_any_inlined_call_events): New.
            * checker-path.h (enum event_kind): Add EK_INLINED_CALL.
            (checker_event::checker_event): Make protected, and move
            definition to checker-path.cc.
            (checker_event::get_fndecl): Use effective fndecl.
            (checker_event::get_stack_depth): Use effective stack depth.
            (checker_event::get_logical_location): Use effective stack depth.
            (checker_event::get_original_stack_depth): New.
            (checker_event::m_fndecl): Rename to...
            (checker_event::m_original_fndecl): ...this.
            (checker_event::m_depth): Rename to...
            (checker_event::m_original_depth): ...this.
            (checker_event::m_effective_fndecl): New field.
            (checker_event::m_effective_depth): New field.
            (class inlined_call_event): New checker_event subclass.
            (checker_path::inject_any_inlined_call_events): New decl.
            * diagnostic-manager.cc: Include "inlining-iterator.h".
            (diagnostic_manager::emit_saved_diagnostic): Call
            checker_path::inject_any_inlined_call_events.
            (diagnostic_manager::prune_for_sm_diagnostic): Handle
            EK_INLINED_CALL.
            * engine.cc (tainted_args_function_custom_event::get_desc): Use
            effective fndecl.
            * inlining-iterator.h: New file.

    gcc/testsuite/ChangeLog:
            PR analyzer/105962
            * gcc.dg/analyzer/inlining-1-multiline.c: New test.
            * gcc.dg/analyzer/inlining-1-no-undo.c: New test.
            * gcc.dg/analyzer/inlining-1.c: New test.
            * gcc.dg/analyzer/inlining-2-multiline.c: New test.
            * gcc.dg/analyzer/inlining-2.c: New test.
            * gcc.dg/analyzer/inlining-3-multiline.c: New test.
            * gcc.dg/analyzer/inlining-3.c: New test.
            * gcc.dg/analyzer/inlining-4-multiline.c: New test.
            * gcc.dg/analyzer/inlining-4.c: New test.
            * gcc.dg/analyzer/inlining-5-multiline.c: New test.
            * gcc.dg/analyzer/inlining-5.c: New test.
            * gcc.dg/analyzer/inlining-6-multiline.c: New test.
            * gcc.dg/analyzer/inlining-6.c: New test.
            * gcc.dg/analyzer/inlining-7-multiline.c: New test.
            * gcc.dg/analyzer/inlining-7.c: New test.

    gcc/ChangeLog:
            PR analyzer/105962
            * doc/invoke.texi: Add -fno-analyzer-undo-inlining.
            * tree-diagnostic-path.cc (default_tree_diagnostic_path_printer):
            Extend -fdiagnostics-path-format=separate-events so that with
            -fdiagnostics-show-path-depths it prints fndecls as well as stack
            depths.

    Signed-off-by: David Malcolm <dmalcolm@redhat.com>

[Bug analyzer/105962] Unhelpful diagnostics paths from analyzer in the face of inlining

[dmalcolm at gcc dot gnu.org]

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

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

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

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