[Bug analyzer/95188] New: analyzer-unsafe-call-within-signal-handler shows wrong statement for signal registration event

[Bug analyzer/95188] New: analyzer-unsafe-call-within-signal-handler shows wrong statement for signal registration event

[mark at gcc dot gnu.org]

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

            Bug ID: 95188
           Summary: analyzer-unsafe-call-within-signal-handler shows wrong
                    statement for signal registration event
           Product: gcc
           Version: 10.1.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: analyzer
          Assignee: dmalcolm at gcc dot gnu.org
          Reporter: mark at gcc dot gnu.org
  Target Milestone: ---

Reproducer:

wget https://sourceware.org/ftp/bzip2/bzip2-1.0.8.tar.gz
tar zxf bzip2-1.0.8.tar.gz
cd bzip2-1.0.8/
gcc -g -O2 -fanalyzer -c bzip2.c

In function ‘showFileNames.part.0’:
bzip2.c:677:4: warning: call to ‘fprintf’ from within signal handler [CWE-479]
[-Wanalyzer-unsafe-call-within-signal-handler]
  677 |    fprintf (
      |    ^~~~~~~~~
  678 |       stderr,
      |       ~~~~~~~
  679 |       "\tInput file = %s, output file = %s\n",
      |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  680 |       inName, outName
      |       ~~~~~~~~~~~~~~~
  681 |    );
      |    ~
  ‘main’: events 1-2
    |
    | 1776 | IntNative main ( IntNative argc, Char *argv[] )
    |      |           ^~~~
    |      |           |
    |      |           (1) entry to ‘main’
    |......
    | 1792 |    smallMode               = False;
    |      |    ~~~~~~~~~
    |      |    |
    |      |    (2) registering ‘mySIGSEGVorSIGBUScatcher’ as signal handler
    |
  event 3
    |
    |cc1:
    | (3): later on, when the signal is delivered to the process
    |
    +--> ‘mySIGSEGVorSIGBUScatcher’: events 4-5
           |
           |  676 |    if (noisy)
           |      |       ~
           |      |       |
           |      |       (5) following ‘true’ branch...
           |......
           |  816 | void mySIGSEGVorSIGBUScatcher ( IntNative n )
           |      |      ^~~~~~~~~~~~~~~~~~~~~~~~
           |      |      |
           |      |      (4) entry to ‘mySIGSEGVorSIGBUScatcher’
           |
         ‘mySIGSEGVorSIGBUScatcher’: event 6
           |
           |cc1:
           | (6): ...to here
           |
         ‘mySIGSEGVorSIGBUScatcher’: event 7
           |
           |cc1:
           | (7): calling ‘showFileNames.part.0’ from
‘mySIGSEGVorSIGBUScatcher’
           |
           +--> ‘showFileNames.part.0’: events 8-9
                  |
                  |  674 | void showFileNames ( void )
                  |      |      ^~~~~~~~~~~~~
                  |      |      |
                  |      |      (8) entry to ‘showFileNames.part.0’
                  |......
                  |  677 |    fprintf (
                  |      |    ~~~~~~~~~
                  |      |    |
                  |      |    (9) call to ‘fprintf’ from within signal handler
                  |  678 |       stderr,
                  |      |       ~~~~~~~
                  |  679 |       "\tInput file = %s, output file = %s\n",
                  |      |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                  |  680 |       inName, outName
                  |      |       ~~~~~~~~~~~~~~~
                  |  681 |    );
                  |      |    ~  
                  |

Note that the signal handler registration points to the wrong instruction:

    | 1792 |    smallMode               = False;
    |      |    ~~~~~~~~~
    |      |    |
    |      |    (2) registering ‘mySIGSEGVorSIGBUScatcher’ as signal handler

A workaround is to add  -fanalyzer-fine-grained, then it does show to correct
signal registration event:

  | 1808 |    signal (SIGSEGV, mySIGSEGVorSIGBUScatcher);
  |      |    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  |      |    |
  |      |    (2) registering ‘mySIGSEGVorSIGBUScatcher’ as signal handler

[Bug analyzer/95188] analyzer-unsafe-call-within-signal-handler shows wrong statement for signal registration event

[cvs-commit at gcc dot gnu.org]

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

--- Comment #1 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:b28491dc2d79763ecbff4f0b9f1f3e48a443be1d

commit r11-3245-gb28491dc2d79763ecbff4f0b9f1f3e48a443be1d
Author: David Malcolm <dmalcolm@redhat.com>
Date:   Tue Aug 18 18:52:17 2020 -0400

    analyzer: bulk merger/processing of runs of nodes at CFG join points

    Prior to this patch the analyzer worklist considered only one node or
    two nodes at a time, processing and/or merging state individually or
    pairwise.

    This could lead to explosions of merger nodes at CFG join points,
    especially after switch statements, which could have large numbers
    of in-edges, and thus large numbers of merger exploded_nodes could
    be created, exceeding the per-point limit and thus stopping analysis
    with -Wanalyzer-too-complex.

    This patch special-cases the handling for runs of consecutive
    nodes in the worklist at a CFG join point, processing and merging
    them all together.

    The patch fixes a state explosion seen in bzip2.c seen when attempting
    to reproduce PR analyzer/95188, in a switch statement in a loop for
    argument parsing.  With this patch, the analyzer successfully
    consolidates the state after the argument parsing to a single exploded
    node.

    In gcc.dg/analyzer/pr96653.c there is a switch statement with over 300
    cases which leads to hitting the per-point limit.  With this patch
    the consolidation code doesn't manage to merge all of them due to other
    worklist-ordering bugs, and it still hits the per-point limits, but it
    does manage some very long consolidations:
      merged 2 in-enodes into 2 out-enode(s) at SN: 403
      merged 2 in-enodes into 2 out-enode(s) at SN: 403
      merged 2 in-enodes into 1 out-enode(s) at SN: 11
      merged 29 in-enodes into 1 out-enode(s) at SN: 35
      merged 6 in-enodes into 1 out-enode(s) at SN: 41
      merged 31 in-enodes into 1 out-enode(s) at SN: 35
    and with a followup patch to fix an SCC issue it manages:
      merged 358 in-enodes into 2 out-enode(s) at SN: 402

    The patch appears to fix the failure on non-x86_64 of:
      gcc.dg/analyzer/pr93032-mztools.c (test for excess errors)
    which is PR analyzer/96616.

    Unfortunately, the patch introduces a memory leak false positive in
    gcc.dg/analyzer/pr94851-1.c, but this appears to be a pre-existing bug
    that was hidden by state-merging failures.

    gcc/analyzer/ChangeLog:
            * engine.cc (exploded_node::dump_dot): Show STATUS_BULK_MERGED.
            (exploded_graph::process_worklist): Call
            maybe_process_run_of_before_supernode_enodes.
            (exploded_graph::maybe_process_run_of_before_supernode_enodes):
            New.
            (exploded_graph_annotator::print_enode): Show STATUS_BULK_MERGED.
            * exploded-graph.h (enum exploded_node::status): Add
            STATUS_BULK_MERGED.

    gcc/testsuite/ChangeLog:
            * gcc.dg/analyzer/bzip2-arg-parse-1.c: New test.
            * gcc.dg/analyzer/loop-n-down-to-1-by-1.c: Remove xfail.
            * gcc.dg/analyzer/pr94851-1.c: Add xfail.

[Bug analyzer/95188] analyzer-unsafe-call-within-signal-handler shows wrong statement for signal registration event

[dmalcolm at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2020-09-16
             Status|UNCONFIRMED                 |ASSIGNED

--- Comment #2 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
(In reply to CVS Commits from comment #1)
> The master branch has been updated by David Malcolm <dmalcolm@gcc.gnu.org>:
> 
> https://gcc.gnu.org/g:b28491dc2d79763ecbff4f0b9f1f3e48a443be1d
> 
> commit r11-3245-gb28491dc2d79763ecbff4f0b9f1f3e48a443be1d
> Author: David Malcolm <dmalcolm@redhat.com>
> Date:   Tue Aug 18 18:52:17 2020 -0400
> 
>     analyzer: bulk merger/processing of runs of nodes at CFG join points

[...]

>     The patch fixes a state explosion seen in bzip2.c seen when attempting
>     to reproduce PR analyzer/95188, in a switch statement in a loop for
>     argument parsing.  With this patch, the analyzer successfully
>     consolidates the state after the argument parsing to a single exploded
>     node.

[...]

As noted above, I'm currently not able to reproduce this bug.  My guess is that
there was a pre-existing failure to fully explore the program and we previously
were lucky to explore enough to trigger the bug, but at some point (probably
the reimplementation of state tracking of
r11-2694-g808f4dfeb3a95f50f15e71148e5c1067f90a126d) the bug is now in the
unexplored section.

The above commit from comment #1 will help, but I'm still not able to reproduce
the bug.  Marking as ASSIGNED since the state-explosion issue ought to be
fixed, and I can at least reproduce that.

[Bug analyzer/95188] analyzer-unsafe-call-within-signal-handler shows wrong statement for signal registration event

[dmalcolm at gcc dot gnu.org]

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

--- Comment #3 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
Also, I can probably cook up a more minimal reproducer - but it would be good
to track down the state-explosion issues: scaling up -fanalyzer to deal
effectively with real-world C code is a goal for me for GCC 11.

[Bug analyzer/95188] analyzer-unsafe-call-within-signal-handler shows wrong statement for signal registration event

[mark at gcc dot gnu.org]

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

--- Comment #4 from Mark Wielaard <mark at gcc dot gnu.org> ---
Note that I can replicate it with the instructions in the description and gcc
git: gcc (GCC) 11.0.0 20200916 (experimental)

$ /opt/local/install/gcc/bin/gcc -g -O2 -fanalyzer -c bzip2.c 2>&1 | head -25
bzip2.c: In function ‘showFileNames.part.0’:
bzip2.c:677:4: warning: call to ‘fprintf’ from within signal handler [CWE-479]
[-Wanalyzer-unsafe-call-within-signal-handler]
  677 |    fprintf (
      |    ^~~~~~~~~
  678 |       stderr,
      |       ~~~~~~~
  679 |       "\tInput file = %s, output file = %s\n",
      |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  680 |       inName, outName
      |       ~~~~~~~~~~~~~~~
  681 |    );
      |    ~
  ‘main’: events 1-2
    |
    | 1776 | IntNative main ( IntNative argc, Char *argv[] )
    |      |           ^~~~
    |      |           |
    |      |           (1) entry to ‘main’
    | 1777 | {
    | 1778 |    Int32  i, j;
    |      |    ~~~~~   
    |      |    |
    |      |    (2) registering ‘mySIGSEGVorSIGBUScatcher’ as signal handler
    |
  event 3


It doesn't point at smallMode anymore, but the Int32 type isn't the right place
either.

For reference this is the main method starting at line 1776:

IntNative main ( IntNative argc, Char *argv[] )
{
   Int32  i, j;
   Char   *tmp;
   Cell   *argList;
   Cell   *aa;
   Bool   decode;

   /*-- Be really really really paranoid :-) --*/
   if (sizeof(Int32) != 4 || sizeof(UInt32) != 4  ||
       sizeof(Int16) != 2 || sizeof(UInt16) != 2  ||
       sizeof(Char)  != 1 || sizeof(UChar)  != 1)
      configError();

   /*-- Initialise --*/
   outputHandleJustInCase  = NULL;
   smallMode               = False;
   keepInputFiles          = False;
   forceOverwrite          = False;
   noisy                   = True;
   verbosity               = 0;
   blockSize100k           = 9;
   testFailsExist          = False;
   unzFailsExist           = False;
   numFileNames            = 0;
   numFilesProcessed       = 0;
   workFactor              = 30;
   deleteOutputOnInterrupt = False;
   exitValue               = 0;
   i = j = 0; /* avoid bogus warning from egcs-1.1.X */

   /*-- Set up signal handlers for mem access errors --*/
   signal (SIGSEGV, mySIGSEGVorSIGBUScatcher);

[Bug analyzer/95188] analyzer-unsafe-call-within-signal-handler shows wrong statement for signal registration event

[dmalcolm at gcc dot gnu.org]

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

--- Comment #5 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
Thanks Mark.  What architecture are you on?

When I do those steps, there's a long wait and then in terminates with no
analyzer output.

If I add -Wanalyzer-too-complex I see lots of warnings about "terminating
analysis for this program point".

What do you see if you add -Wanalyzer-too-complex?

If you do, my guess as to what's happening is that there's a "random" factor in
how the worklist is being explored, and that on my machine it's hitting the
complexity limits before finding the issue, and on your machine it's finding
the issue first.  Perhaps it relates to pointer addresses; PR 96608 notes some
places where hash values could vary between runs, and that could be enough to
throw out the worklist traversal order.

[Bug analyzer/95188] analyzer-unsafe-call-within-signal-handler shows wrong statement for signal registration event

[mark at gcc dot gnu.org]

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

--- Comment #6 from Mark Wielaard <mark at gcc dot gnu.org> ---
Created attachment 49291
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=49291&action=edit
Output for gcc -Wanalyzer-too-complex -g -O2 -fanalyzer -c bzip2.c

(In reply to David Malcolm from comment #5)
> Thanks Mark.  What architecture are you on?

RHEL7 x86_64.

> When I do those steps, there's a long wait and then in terminates with no
> analyzer output.
> 
> If I add -Wanalyzer-too-complex I see lots of warnings about "terminating
> analysis for this program point".
> 
> What do you see if you add -Wanalyzer-too-complex?

Lots and lots of output saying "warning: terminating analysis for this program
point". log attached.

[Bug analyzer/95188] analyzer-unsafe-call-within-signal-handler shows wrong statement for signal registration event

[dmalcolm at gcc dot gnu.org]

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

--- Comment #7 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
Thanks.  I see a similar deluge of
  "terminating analysis for this program point"
warnings, but at different locations.

My warnings eventually terminate with:

  bzip2.c:1537:31: warning: analysis bailed out early (4561 'after-snode'
enodes; 15071 enodes) [-Wanalyzer-too-complex]

whereas yours don't - my machine is eventually hitting the overall complexity
limit (as opposed to merely hitting the per-program-point limit).

If I add
  -fparam-analyzer-bb-explosion-factor=50
to try to get past that limit (the default for that param is 5) then I see the
-Wanalyzer-unsafe-call-within-signal-handler warnings you're seeing (it takes
quite a while to run).

As in comment #5, I think what's happening is some "random" aspect of the
analysis affecting the order in which the worklist is processed, which leads to
my machine terminating early and yours running to completion.

So there are at least four issues here:

(a) the reported bug: that -Wanalyzer-unsafe-call-within-signal-handler uses
the wrong source location when reporting the signal registration event in the
diagnostic_path
(b) that -fanalyzer is hitting per-program-point limits
(c) that -fanalyzer can hit the overall enode limit
(d) that the behavior is sufficiently "random" that (c) can happen on one
machine and not on another, and that the log for the (b) events shows the order
of exploration varying between machines.

Mark: please can you add -fdump-analyzer-supergraph to the arguments and attach
the bzip2.c.supergraph.dot file to this bug.  Doing so may help track down (d).

[Bug analyzer/95188] analyzer-unsafe-call-within-signal-handler shows wrong statement for signal registration event

[cvs-commit at gcc dot gnu.org]

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

--- Comment #8 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:d60d63a00bb50ba6896939705c589578177b404d

commit r11-3537-gd60d63a00bb50ba6896939705c589578177b404d
Author: David Malcolm <dmalcolm@redhat.com>
Date:   Tue Sep 29 15:55:33 2020 -0400

    analyzer: fix signal-handler registration location [PR95188]

    PR analyzer/95188 reports that diagnostics from
    -Wanalyzer-unsafe-call-within-signal-handler use the wrong
    source location when reporting the signal-handler registration
    event in the diagnostic_path.  The diagnostics erroneously use the
    location of the first stmt in the basic block containing the call
    to "signal", rather than that of the call itself.

    Fixed thusly.

    gcc/analyzer/ChangeLog:
            PR analyzer/95188
            * engine.cc (stmt_requires_new_enode_p): Split enodes before
            "signal" calls.

    gcc/testsuite/ChangeLog:
            PR analyzer/95188
            * gcc.dg/analyzer/signal-registration-loc.c: New test.

[Bug analyzer/95188] analyzer-unsafe-call-within-signal-handler shows wrong statement for signal registration event

[dmalcolm at gcc dot gnu.org]

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

--- Comment #9 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
The above patch fixes (a) from comment #7 above, but (b), (c) and (d) still
need fixing, so keeping this open for now.

[Bug analyzer/95188] analyzer-unsafe-call-within-signal-handler shows wrong statement for signal registration event

[mark at gcc dot gnu.org]

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

--- Comment #10 from Mark Wielaard <mark at gcc dot gnu.org> ---
Created attachment 49293
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=49293&action=edit
supergraph

> Mark: please can you add -fdump-analyzer-supergraph to the arguments and attach > the bzip2.c.supergraph.dot file to this bug.  Doing so may help track down (d).

gcc -g -O2 -fanalyzer -fdump-analyzer-supergraph -c bzip2.c

[Bug analyzer/95188] analyzer-unsafe-call-within-signal-handler shows wrong statement for signal registration event

[dmalcolm at gcc dot gnu.org]

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

--- Comment #11 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
(In reply to Mark Wielaard from comment #10)
> Created attachment 49293 [details]
> supergraph

Thanks.  Compared to my testing, I'm seeing what appear to be differences in
the inputs to the analyzer at the gimple level, which are likely due to
differences in the rest of the compiler.

Is this with the same version of gcc as in comment #4, where you said "gcc
(GCC) 11.0.0 20200916 (experimental)".

You don't happen to know exactly which revision, do you?

[The md5sum of the bzip2.c I'm using is 23f66348f80345353d5b5b98e299ff15. 
There could also be differences in the system headers, I suppose]

[Bug analyzer/95188] analyzer-unsafe-call-within-signal-handler shows wrong statement for signal registration event

[mark at gcc dot gnu.org]

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

--- Comment #12 from Mark Wielaard <mark at gcc dot gnu.org> ---
(In reply to David Malcolm from comment #11)
> (In reply to Mark Wielaard from comment #10)
> > Created attachment 49293 [details]
> > supergraph
> 
> Thanks.  Compared to my testing, I'm seeing what appear to be differences in
> the inputs to the analyzer at the gimple level, which are likely due to
> differences in the rest of the compiler.
> 
> Is this with the same version of gcc as in comment #4, where you said "gcc
> (GCC) 11.0.0 20200916 (experimental)".
> 
> You don't happen to know exactly which revision, do you?

I am afraid I don't know exactly. I have been experimenting with having DWARF 5
as default in my builds, which is another difference.

> [The md5sum of the bzip2.c I'm using is 23f66348f80345353d5b5b98e299ff15. 
> There could also be differences in the system headers, I suppose]

The MD5 matches. But I am indeed using system headers from RHEL7 with DTS9
installed to bootstrap my GCC builds.

[Bug analyzer/95188] analyzer-unsafe-call-within-signal-handler shows wrong statement for signal registration event

[dmalcolm at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |99390

--- Comment #13 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
At least some of the state explosion appears to be due to malfunctioning call
summaries, e.g.:
  function ‘fileExists’, with call string: [(SN: 422 -> SN: 163 in main), (SN:
599 -> SN: 334 in uncompress)]
probably should have been summarized, but wasn't.

Adding this to the call summaries tracker bug.


Referenced Bugs:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99390
[Bug 99390] [meta-bug] tracker bug for call summaries in -fanalyzer

[Bug analyzer/95188] State explosion on bzip2-1.0.8/bzip2.c hides -Wanalyzer-unsafe-call-within-signal-handler

[dmalcolm at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|analyzer-unsafe-call-within |State explosion on
                   |-signal-handler shows wrong |bzip2-1.0.8/bzip2.c hides
                   |statement for signal        |-Wanalyzer-unsafe-call-with
                   |registration event          |in-signal-handler

--- Comment #14 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
Comment #9 noted that the original issue here (the wrong location) was fixed,
but the remaining issues are to do with state explosions.  I tested with
today's trunk, and the state explosions still happen (and, for me mask the
signal warning, unless I pass e.g. --param=analyzer-bb-explosion-factor=50

Updating subject to reflect that (rather than opening a new bug, to keep the
conversation in one place)