[Bug analyzer/110902] New: Missing cast in region_model_manager::maybe_fold_binop on MULT_EXPR by 1

[Bug analyzer/110902] New: Missing cast in region_model_manager::maybe_fold_binop on MULT_EXPR by 1

[dmalcolm at gcc dot gnu.org]

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

            Bug ID: 110902
           Summary: Missing cast in region_model_manager::maybe_fold_binop
                    on MULT_EXPR by 1
           Product: gcc
           Version: 13.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: ---

Whilst trying to fix PR analyzer/110426, I noticed that
region_model_manager::maybe_fold_binop doesn't always return the correct type;
specifically, it fails to cast to TYPE when folding (VAL * 1) -> VAL:

diff --git a/gcc/analyzer/region-model-manager.cc
b/gcc/analyzer/region-model-manager.cc
index 46d271a295c..010906f1ec0 100644
--- a/gcc/analyzer/region-model-manager.cc
+++ b/gcc/analyzer/region-model-manager.cc
@@ -654,7 +654,7 @@ region_model_manager::maybe_fold_binop (tree type, enum
tree_code op,
        return get_or_create_constant_svalue (build_int_cst (type, 0));
       /* (VAL * 1) -> VAL.  */
       if (cst1 && integer_onep (cst1))
-       return arg0;
+       return get_or_create_cast (type, arg0);
       break;
     case BIT_AND_EXPR:
       if (cst1)

However, on adding the above cast, various bounds-checking tests fail,
seemingly due to confusion about ptrdiff_t vs size_t, and how to compare such
values:

FAIL: gcc.dg/analyzer/flexible-array-member-1.c  (test for warnings, line 96)

With -m64:
FAIL: gcc.dg/analyzer/out-of-bounds-diagram-3.c  (test for warnings, line 19)
FAIL: gcc.dg/analyzer/out-of-bounds-diagram-3.c  (test for warnings, line 24)
FAIL: gcc.dg/analyzer/out-of-bounds-diagram-3.c expected multiline pattern
lines 29-44

[Bug analyzer/110902] Missing cast in region_model_manager::maybe_fold_binop on MULT_EXPR by 1

[cvs-commit at gcc dot gnu.org]

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

--- 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:021077b94741c9300dfff3a24e95b3ffa3f508a7

commit r14-3001-g021077b94741c9300dfff3a24e95b3ffa3f508a7
Author: David Malcolm <dmalcolm@redhat.com>
Date:   Fri Aug 4 16:18:40 2023 -0400

    analyzer: handle function attribute "alloc_size" [PR110426]

    This patch makes -fanalyzer make use of the function attribute
    "alloc_size", allowing -fanalyzer to emit -Wanalyzer-allocation-size,
    -Wanalyzer-out-of-bounds, and -Wanalyzer-tainted-allocation-size on
    execution paths involving allocations using such functions.

    gcc/analyzer/ChangeLog:
            PR analyzer/110426
            * bounds-checking.cc (region_model::check_region_bounds): Handle
            symbolic base regions.
            * call-details.cc: Include "stringpool.h" and "attribs.h".
            (call_details::lookup_function_attribute): New function.
            * call-details.h (call_details::lookup_function_attribute): New
            function decl.
            * region-model-manager.cc
            (region_model_manager::maybe_fold_binop): Add reference to
            PR analyzer/110902.
            * region-model-reachability.cc (reachable_regions::handle_sval):
            Add symbolic regions for pointers that are conjured svalues for
            the LHS of a stmt.
            * region-model.cc (region_model::canonicalize): Purge dynamic
            extents for regions that aren't referenced.
            (get_result_size_in_bytes): New function.
            (region_model::on_call_pre): Use get_result_size_in_bytes and
            potentially set the dynamic extents of the region pointed to by
            the return value.
            (region_model::deref_rvalue): Add param "add_nonnull_constraint"
            and use it to conditionalize adding the constraint.
            (pending_diagnostic_subclass::dubious_allocation_size): Add "stmt"
            param to both ctors and use it to initialize new "m_stmt" field.
            (pending_diagnostic_subclass::operator==): Use m_stmt; don't use
            m_lhs or m_rhs.
            (pending_diagnostic_subclass::m_stmt): New field.
            (region_model::check_region_size): Generalize to any kind of
            pointer svalue by using deref_rvalue rather than checking for
            region_svalue.  Pass stmt to dubious_allocation_size ctor.
            * region-model.h (region_model::deref_rvalue): Add param
            "add_nonnull_constraint".
            * svalue.cc (conjured_svalue::lhs_value_p): New function.
            * svalue.h (conjured_svalue::lhs_value_p): New decl.

    gcc/testsuite/ChangeLog:
            PR analyzer/110426
            * gcc.dg/analyzer/allocation-size-1.c: Update expected message to
            reflect consolidation of size and assignment into a single event.
            * gcc.dg/analyzer/allocation-size-2.c: Likewise.
            * gcc.dg/analyzer/allocation-size-3.c: Likewise.
            * gcc.dg/analyzer/allocation-size-4.c: Likewise.
            * gcc.dg/analyzer/allocation-size-multiline-1.c: Likewise.
            * gcc.dg/analyzer/allocation-size-multiline-2.c: Likewise.
            * gcc.dg/analyzer/allocation-size-multiline-3.c: Likewise.
            * gcc.dg/analyzer/attr-alloc_size-1.c: New test.
            * gcc.dg/analyzer/attr-alloc_size-2.c: New test.
            * gcc.dg/analyzer/attr-alloc_size-3.c: New test.
            * gcc.dg/analyzer/explode-4.c: New test.
            * gcc.dg/analyzer/taint-size-1.c: Add test coverage for
            __attribute__ alloc_size.

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

[Bug analyzer/110902] Missing cast in region_model_manager::maybe_fold_binop on MULT_EXPR by 1

[cvs-commit at gcc dot gnu.org]

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

--- Comment #2 from GCC 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:1579394c9ecf3d1f678daa54b835c7fc3b76fb6d

commit r14-9527-g1579394c9ecf3d1f678daa54b835c7fc3b76fb6d
Author: David Malcolm <dmalcolm@redhat.com>
Date:   Mon Mar 18 18:44:34 2024 -0400

    analyzer: fix ICEs due to sloppy types in bounds-checking
[PR110902,PR110928,PR111305,PR111441]

    Various analyzer ICEs in our bugzilla relate to sloppy use of types
    within bounds-checking.

    The bounds-checking code works by comparing symbolic *bit* offsets, and
    we don't have a good user-facing type that can represent such an offset
    (ptrdiff_type_node is for *byte* offsets).

    ana::svalue doesn't enforce valid combinations of types for things like
    binary operations.  When I added the access diagrams for GCC 14, this
    could lead to attempts to generate trees for such svalues, leading to
    trees with invalid combinations of types (e.g. PLUS_EXPR or MULT_EXPR of
    incompatible types), leading to ICEs inside the tree folding logic.

    I tried two approaches to fixing this.

    My first approach was to fix the type-handling throughout the
    bounds-checking code to use correct types, using size_type_node for
    sizes, ptrdiff_type_node for byte offsets, and trying ptrdiff_type_node
    for bit offsets.  I implemented this, and it fixed the crashes, but
    unfortunately it led to:
    (a) numerous false negatives from the bounds-checking code, due to it
    becoming unable to be sure that the accessed offset was beyond the valid
    bounds, due to the expressions involved gaining complicated sets of
    nested casts.
    (b) ugly access diagrams full of nested casts (for capacities, gap
    measurements, etc)

    So my second approach, implemented in this patch, is to accept that we
    don't have a tree type for representing bit offsets.  The patch
    represents bit offsets using "typeless" symbolic values i.e. ones for
    which get_type () is NULL_TREE, and implements enough support for basic
    arithemetic as if these are mathematical integers (albeit ones for which
    concrete values within an expression must fit within a signed wide int).
    Such values can't be converted to tree, so the patch avoids such
    conversions, instead implementing a new svalue::maybe_print_for_user for
    printing them to a pretty_printer.  The patch uses ptrdiff_type_node for
    byte offsets.

    Doing so fixes the crashes, whilst appearing to preserve the behavior of
    -Wanalyzer-out-of-bounds in my testing.

    gcc/analyzer/ChangeLog:
            PR analyzer/110902
            PR analyzer/110928
            PR analyzer/111305
            PR analyzer/111441
            * access-diagram.cc: Include "analyzer/analyzer-selftests.h".
            (get_access_size_str): Reimplement for conversion of
            implmementation of bit_size_expr from tree to const svalue &.  Use
            svalue::maybe_print_for_user rather than tree printing routines.
            (remove_ssa_names): Make non-static.
            (bit_size_expr::get_formatted_str): Rename to...
            (bit_size_expr::maybe_get_formatted_str): ...this, adding "model"
            param and converting return type to a unique_ptr.  Update for
            conversion of implementation of bit_size_expr from tree to
            const svalue &.  Use svalue::maybe_print_for_user rather than tree
            printing routines.
            (bit_size_expr::print): Rename to...
            (bit_size_expr::maybe_print_for_user): ...this, adding "model"
            param and converting return type to bool.  Update for
            conversion of implementation of bit_size_expr from tree to
            const svalue &.  Use svalue::maybe_print_for_user rather than tree
            printing routines.
            (bit_size_expr::maybe_get_as_bytes): Add "mgr" param and convert
            return type from tree to const svalue *; reimplement.
            (access_range::access_range): Call strip_types when on
region_offset
            intializations.
            (access_range::get_size): Update for conversion of implementation
            of bit_size_expr from tree to const svalue &.
            (access_operation::get_valid_bits): Pass manager to access_range
            ctor.
            (access_operation::maybe_get_invalid_before_bits): Likewise.
            (access_operation::maybe_get_invalid_after_bits): Likewise.
            (boundaries::add): Likewise.
            (bit_to_table_map::populate): Add "mgr" param and pass it to
            access_range ctor.
            (access_diagram_impl::access_diagram_impl): Pass manager to
            bit_to_table_map::populate.
            (access_diagram_impl::maybe_add_gap): Use svalue rather than tree
            for symbolic bit offsets.  Port to new bit_size_expr
            representation.
            (access_diagram_impl::add_valid_vs_invalid_ruler): Port to new
            bit_size_expr representation.
            (selftest::assert_eq_typeless_integer): New.
            (ASSERT_EQ_TYPELESS_INTEGER): New.
            (selftest::test_bit_size_expr_to_bytes): New.
            (selftest::analyzer_access_diagram_cc_tests): New.
            * access-diagram.h (class bit_size_expr): Reimplement, converting
            implementation from tree to const svalue &.
            (access_range::access_range): Add "mgr" param.  Call strip_types
            on region_offset initializations.
            (access_range::get_size): Update decl for reimplementation.
            * analyzer-selftests.cc (selftest::run_analyzer_selftests): Call
            selftest::analyzer_access_diagram_cc_tests.
            * analyzer-selftests.h
            (selftest::analyzer_checker_script_cc_tests): Delete this stray
            typo.
            (selftest::analyzer_access_diagram_cc_tests): New decl.
            * analyzer.h (print_expr_for_user): New decl.
            (calc_symbolic_bit_offset): Update decl for reimplementation.
            (strip_types): New decls.
            (remove_ssa_names): New decl.
            * bounds-checking.cc (strip_types): New.
            (region_model::check_symbolic_bounds): Use typeless svalues.
            * region-model-manager.cc
            (region_model_manager::get_or_create_constant_svalue): Add "type"
            param.  Add overload with old signature.
            (region_model_manager::get_or_create_int_cst): Support type being
            NULL_TREE.
            (region_model_manager::maybe_fold_unaryop): Gracefully reject
folding
            of casts to NULL_TREE type.
            (get_code_for_cast): Use NOP_EXPR for "casting" svalues to
            NULL_TREE type.
            (region_model_manager::get_or_create_cast): Support "casting"
            svalues to NULL_TREE type.
            (region_model_manager::maybe_fold_binop): Don't crash on inputs
            with NULL_TREE type.  Handle folding of binops on constants with
            NULL_TREE type.  Add missing cast from PR analyzer/110902.
            Support enough folding of other ops on NULL_TREE type to support
            bounds checking.
            (region_model_manager::get_or_create_const_fn_result_svalue):
            Remove assertion that type is nonnull.
            * region-model-manager.h
            (region_model_manager::get_or_create_constant_svalue): Add
            overloaded decl taking a type.
            (region_model_manager::maybe_fold_binop): Make public.
            (region_model_manager::constants_map_t): Use
            constant_svalue::key_t for the key, rather than just tree.
            * region-model.cc (print_expr_for_user): New.
            (selftest::test_array_2): Handle casts.
            * region.cc (region_offset::calc_symbolic_bit_offset): Return
            const svalue & rather than tree, and reimplement accordingly.
            (region::calc_offset): Use ptrdiff_type_node for types of byte
            offsets.
            (region::maybe_print_for_user): New.
            (element_region::get_relative_symbolic_offset): Use NULL_TREE for
            types of bit offsets.
            (offset_region::get_bit_offset): Likewise.
            (sized_region::get_bit_size_sval): Likewise for bit sizes.
            * region.h (region::maybe_print_for_user): New decl.
            * svalue.cc (class auto_add_parens): New.
            (svalue::maybe_print_for_user): New.
            (svalue::cmp_ptr): Support typeless constant svalues.
            (tristate_from_boolean_tree_node): New, taken from...
            (constant_svalue::eval_condition): ...here.  Handle comparison of
            typeless integer svalue constants.
            * svalue.h (svalue::maybe_print_for_user): New decl.
            (class constant_svalue): Support the type of the svalue being
            NULL_TREE.
            (struct default_hash_traits<constant_svalue::key_t>): New.

    gcc/ChangeLog:
            PR analyzer/110902
            PR analyzer/110928
            PR analyzer/111305
            PR analyzer/111441
            * selftest.h (ASSERT_NE_AT): New macro.

    gcc/testsuite/ChangeLog:
            PR analyzer/110902
            PR analyzer/110928
            PR analyzer/111305
            PR analyzer/111441
            * c-c++-common/analyzer/out-of-bounds-const-fn.c: New test.
            * c-c++-common/analyzer/out-of-bounds-diagram-11.c: Update
            expected diagram output.
            * c-c++-common/analyzer/out-of-bounds-diagram-pr110928.c: New test.
            * c-c++-common/analyzer/out-of-bounds-diagram-pr111305.c: New test.
            * c-c++-common/analyzer/out-of-bounds-diagram-pr111441.c: New test.

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

[Bug analyzer/110902] Missing cast in region_model_manager::maybe_fold_binop on MULT_EXPR by 1

[dmalcolm at gcc dot gnu.org]

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

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

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

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