On Tue, 2023-07-04 at 18:25 +0200, priour...@gmail.com wrote:
> From: benjamin priour <priour...@gmail.com>
>
> Script contrib/check_GNU_style.sh complains about there being a space
> before a left square bracket ("operator new []").
> Though, it is actually within a literal string, and the space
> is required to correctly detect the function.
>
> Succesfully regstrapped on x86_64-linux-gnu against trunk
> 3c776fdf1a8.
> Is it OK for trunk ?
Thanks for the patch.
Overall, looks almost ready, but some nitpicks below,,,
[..snip...]
> diff --git a/gcc/analyzer/kf-lang-cp.cc b/gcc/analyzer/kf-lang-cp.cc
> index 393b4f25e79..258d92919d7 100644
> --- a/gcc/analyzer/kf-lang-cp.cc
> +++ b/gcc/analyzer/kf-lang-cp.cc
> @@ -35,6 +35,34 @@ along with GCC; see the file COPYING3. If not see
>
> #if ENABLE_ANALYZER
>
> +/* Return TRUE if CALL is non-allocating operator new or operator
> new[]*/
> +
> +bool is_placement_new_p (const gcall *call)
Please can you extend the leading comment, giving the expected
signatures of the functions, and a link to cppreference.org.
In particular, there's some special-casing here of "nothrow_t" which
would make more sense with a comment up here.
> +{
> + gcc_assert (call);
> +
> + tree fndecl = gimple_call_fndecl (call);
> + if (!fndecl)
> + return false;
> +
> + if (!is_named_call_p (fndecl, "operator new", call, 2)
> + && !is_named_call_p (fndecl, "operator new []", call, 2))
> + return false;
> + tree arg1 = gimple_call_arg (call, 1);
> +
> + if (!POINTER_TYPE_P (TREE_TYPE (arg1)))
> + return false;
> +
> + /* Sadly, for non-throwing new, the second argument type
> + is not REFERENCE_TYPE but also POINTER_TYPE
> + so a simple check is out of the way. */
> + tree identifier = TYPE_IDENTIFIER (TREE_TYPE (TREE_TYPE (arg1)));
> + if (!identifier)
> + return true;
> + const char *name = IDENTIFIER_POINTER (identifier);
> + return 0 != strcmp (name, "nothrow_t");
> +}
> +
> namespace ana {
>
> /* Implementations of specific functions. */
> @@ -46,7 +74,7 @@ class kf_operator_new : public known_function
> public:
> bool matches_call_types_p (const call_details &cd) const final
> override
> {
> - return cd.num_args () == 1;
> + return cd.num_args () == 1 || cd.num_args () == 2;
Looks like we should also check that arg 0 is of integral type, and
that arg 1 is of pointer type.
> }
>
> void impl_call_pre (const call_details &cd) const final override
> @@ -54,13 +82,60 @@ public:
> region_model *model = cd.get_model ();
> region_model_manager *mgr = cd.get_manager ();
> const svalue *size_sval = cd.get_arg_svalue (0);
> - const region *new_reg
> - = model->get_or_create_region_for_heap_alloc (size_sval,
> cd.get_ctxt ());
> - if (cd.get_lhs_type ())
> + region_model_context *ctxt = cd.get_ctxt ();
> + const gcall *call = cd.get_call_stmt ();
> +
> + /* If the call is an allocating new, then create a heap
> allocated
> + region. */
> + if (!is_placement_new_p (call))
> + {
You have:
if (!condition)
suite_a;
else
suite_b; // this is implicitly a double negative
Please change it to:
if (condition)
suite_b;
else
suite_a;
to avoid the implicit double negative.
> + const region *new_reg
> + = model->get_or_create_region_for_heap_alloc (size_sval, ctxt);
> + if (cd.get_lhs_type ())
> + {
> + const svalue *ptr_sval
> + = mgr->get_ptr_svalue (cd.get_lhs_type (), new_reg);
> + cd.maybe_set_lhs (ptr_sval);
> + }
> + }
> + /* If the call was actually a placement new, check that
> accessing
> + the buffer lhs is placed into does not result in out-of-bounds.
> */
> + else
> {
> + const region *ptr_reg = cd.maybe_get_arg_region (1);
> + if (ptr_reg && cd.get_lhs_type ())
> + {
> + const region *base_reg = ptr_reg->get_base_region ();
> + const svalue *num_bytes_sval = cd.get_arg_svalue (0);
> + const region *sized_new_reg = mgr->get_sized_region (base_reg,
> + cd.get_lhs_type (),
> + num_bytes_sval);
> + model->check_region_for_write (sized_new_reg,
> + nullptr,
> + ctxt);
> const svalue *ptr_sval
> - = mgr->get_ptr_svalue (cd.get_lhs_type (), new_reg);
> + = mgr->get_ptr_svalue (cd.get_lhs_type (), sized_new_reg);
> cd.maybe_set_lhs (ptr_sval);
> + }
> + }
> + }
[...snip...]
> diff --git a/gcc/testsuite/g++.dg/analyzer/new-2.C
> b/gcc/testsuite/g++.dg/analyzer/new-2.C
> new file mode 100644
> index 00000000000..4e696040a54
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/analyzer/new-2.C
> @@ -0,0 +1,50 @@
> +// { dg-additional-options "-O0" }
> +
> +struct A
> +{
> + int x;
> + int y;
> +};
We've run into issues with bounds-checking testcases when using types
like "int" that have target-specific sizes.
Please use <stdint.h> in these test cases, and types with explicit
sizes, such as int32_t, to avoid the behavior of the test cases being
affected by sizeof the various types.
[..snip...]
Other than those issues, looks good
Thanks again
Dave
