[Bug analyzer/96374] Analyzer erroneously rejects certain diagnostics due to path-feasibility being used on shortest path

["cvs-commit at gcc dot gnu.org" <gcc-bugzilla@gcc.gnu.org>]

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

--- 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:3857edb5d32dcdc11d9a2fe3ad7c156c52a1ec7f

commit r11-7640-g3857edb5d32dcdc11d9a2fe3ad7c156c52a1ec7f
Author: David Malcolm <dmalcolm@redhat.com>
Date:   Thu Mar 11 17:46:37 2021 -0500

    analyzer: new implementation of shortest feasible path [PR96374]

    The analyzer builds an exploded graph of (point,state) pairs and when
    it finds a problem, records a diagnostic at the relevant exploded node.
    Once it has finished exploring the graph, the analyzer needs to generate
    the shortest feasible path through the graph to each diagnostic's node.
    This is used:
    - for rejecting diagnostics that are infeasible (due to impossible sets
      of constraints),
    - for use in determining which diagnostic to use in each deduplication
      set (the one with the shortest path), and
    - for building checker_paths for the "winning" diagnostics, giving a
      list of events

    Prior to this patch the analyzer simply found the shortest path to the
    node, and then checked it for feasibility, which could lead to falsely
    rejecting diagnostics: "the shortest path, if feasible" is not the same
    as "the shortest feasible path" (PR analyzer/96374).
    An example is PR analyzer/93355, where this issue causes the analyzer
    to fail to emit a leak warning for a missing fclose on an error-handling
    path in intl/localealias.c.

    This patch implements a new algorithm for finding the shortest feasible
    path to an exploded node: instead of simply finding the shortest path,
    the new algorithm uses a worklist to iteratively build a tree of path
    prefixes, which are feasible paths by construction, until a path to the
    target node is found.  The worklist is prioritized, so that the first
    feasible path discovered is the shortest possible feasible path.  The
    algorithm continues trying paths until the target node is reached or a
    limit is exceeded, in which case the diagnostic is treated as being
    infeasible (which could still be a false negative, but is much less
    likely to happen than before).  Iteratively building a tree of paths
    allows for work to be reused, and the tree can be dumped in .dot form
    (via a new -fdump-analyzer-feasibility option), making it much easier to
    debug compared to other approaches I tried.

    Doing so fixes the missing leak warning for PR analyzer/93355 and
    various other test cases.

    Testing:
    - I manually verified that the behavior is determistic using 50 builds
      of pr93355-localealias.c.  All dumps were identical.
    - I manually verified that it still builds with --disable-analyzer.
    - Lightly tested with valgrind; no additional issues.
    - Lightly performance tested, showing a slight speed regression to the
      analyzer relative to before the patch, but correctness for this issue
      is more important than the slight performance hit for the analyzer.

    gcc/ChangeLog:
            PR analyzer/96374
            * Makefile.in (ANALYZER_OBJS): Add analyzer/feasible-graph.o and
            analyzer/trimmed-graph.o.
            * doc/analyzer.texi (Analyzer Paths): Rewrite description of
            feasibility checking to reflect new implementation.
            * doc/invoke.texi (-fdump-analyzer-feasibility): Document new
            option.
            * shortest-paths.h (shortest_paths::get_shortest_distance): New.

    gcc/analyzer/ChangeLog:
            PR analyzer/96374
            * analyzer.opt (-param=analyzer-max-infeasible-edges=): New param.
            (fdump-analyzer-feasibility): New flag.
            * diagnostic-manager.cc: Include "analyzer/trimmed-graph.h" and
            "analyzer/feasible-graph.h".
            (epath_finder::epath_finder): Convert m_sep to a pointer and
            only create it if !flag_analyzer_feasibility.
            (epath_finder::~epath_finder): New.
            (epath_finder::m_sep): Convert to a pointer.
            (epath_finder::get_best_epath): Add param "diag_idx" and use it
            when logging.  Rather than finding the shortest path and then
            checking feasibility, instead use explore_feasible_paths unless
            !flag_analyzer_feasibility, in which case simply use the shortest
            path, and note if it is infeasible.  Update for m_sep becoming a
            pointer.
            (class feasible_worklist): New.
            (epath_finder::explore_feasible_paths): New.
            (epath_finder::process_worklist_item): New.
            (class dump_eg_with_shortest_path): New.
            (epath_finder::dump_trimmed_graph): New.
            (epath_finder::dump_feasible_graph): New.
            (saved_diagnostic::saved_diagnostic): Add "idx" param, using it
            on new field m_idx.
            (saved_diagnostic::to_json): Dump m_idx.
            (saved_diagnostic::calc_best_epath): Pass m_idx to get_best_epath.
            Remove assertion that m_problem was set when m_best_epath is NULL.
            (diagnostic_manager::add_diagnostic): Pass an index when created
            saved_diagnostic instances.
            * diagnostic-manager.h (saved_diagnostic::saved_diagnostic): Add
            "idx" param.
            (saved_diagnostic::get_index): New accessor.
            (saved_diagnostic::m_idx): New field.
            * engine.cc (exploded_node::dump_dot): Call args.dump_extra_info.
            Move code to...
            (exploded_node::dump_processed_stmts): ...this new function and...
            (exploded_node::dump_saved_diagnostics): ...this new function.
            Add index of each diagnostic.
            (exploded_edge::dump_dot):  Move bulk of code to...
            (exploded_edge::dump_dot_label): ...this new function.
            * exploded-graph.h (eg_traits::dump_args_t::dump_extra_info): New
            vfunc.
            (exploded_node::dump_processed_stmts): New decl.
            (exploded_node::dump_saved_diagnostics): New decl.
            (exploded_edge::dump_dot_label): New decl.
            * feasible-graph.cc: New file.
            * feasible-graph.h: New file.
            * trimmed-graph.cc: New file.
            * trimmed-graph.h: New file.

    gcc/testsuite/ChangeLog:
            PR analyzer/96374
            * gcc.dg/analyzer/dot-output.c: Add -fdump-analyzer-feasibility
            to options.
            * gcc.dg/analyzer/feasibility-1.c (test_6): Remove xfail.
            (test_7): New.
            * gcc.dg/analyzer/pr93355-localealias-feasibility-2.c: Remove
xfail.
            * gcc.dg/analyzer/pr93355-localealias-feasibility-3.c: Remove
xfails.
            * gcc.dg/analyzer/pr93355-localealias-feasibility.c: Remove
            -fno-analyzer-feasibility from options.
            * gcc.dg/analyzer/pr93355-localealias.c: Likewise.
            * gcc.dg/analyzer/unknown-fns-4.c: Remove xfail.