Am Montag, dem 18.12.2023 um 20:14 +0100 schrieb Jakub Jelinek:
> Hi!
>
> The following patch changes -Walloc-size warning to no longer warn
> about int *p = calloc (1, sizeof (int));, because as discussed earlier,
> the size is IMNSHO sufficient in that case, for alloc_size with 2
> arguments warns if the product of the 2 arguments is insufficiently small.
>
> Also, it warns also for explicit casts of malloc/calloc etc. calls
> rather than just implicit, so not just
> int *p = malloc (1);
> but also
> int *p = (int *) malloc (1);
>
> It also fixes some ICEs where the code didn't verify the alloc_size
> arguments properly (Walloc-size-5.c testcase ICEs with vanilla trunk).
>
> And lastly, it introduces a coding style warning, -Wcalloc-transposed-args
> to warn for calloc (sizeof (struct S), 1) and similar calls (regardless
> of what they are cast to, warning whenever first argument is sizeof and
> the second is not).
I would generally see function arguments that are swapped relative
to the documented ABI as more than a coding style issue even in
cases where it can be expected to make no difference.
>
> Ok for trunk if this passes bootstrap/regtest?
I wonder whether we could turn on -Walloc-size for -Wall with this change?
Martin
>
> If yes, I'd implement it for C++ next.
> If not, we should at least fix the ICEs.
>
> 2023-12-18 Jakub Jelinek <ja...@redhat.com>
>
> gcc/
> * doc/invoke.texi (-Walloc-size): Add to the list of
> warning options, remove unnecessary line-break.
> (-Wcalloc-transposed-args): Document new warning.
> gcc/c-family/
> * c.opt (Wcalloc-transposed-args): New warning.
> * c-common.h (warn_for_calloc, warn_for_alloc_size): Declare.
> * c-warn.cc (warn_for_calloc, warn_for_alloc_size): New functions.
> gcc/c/
> * c-parser.cc (c_parser_postfix_expression_after_primary): Grow
> sizeof_arg and sizeof_arg_loc arrays to 6 elements. Call
> warn_for_calloc if warn_calloc_transposed_args for functions with
> alloc_size type attribute with 2 arguments.
> (c_parser_expr_list): Use 6 instead of 3.
> * c-typeck.cc (build_c_cast): Call warn_for_alloc_size for casts
> of calls to functions with alloc_size type attribute.
> (convert_for_assignment): Likewise.
> gcc/testsuite/
> * gcc.dg/Walloc-size-4.c: New test.
> * gcc.dg/Walloc-size-5.c: New test.
> * gcc.dg/Wcalloc-transposed-args-1.c: New test.
>
> --- gcc/doc/invoke.texi.jj 2023-12-18 09:39:49.411355496 +0100
> +++ gcc/doc/invoke.texi 2023-12-18 19:59:37.139525128 +0100
> @@ -328,7 +328,7 @@ Objective-C and Objective-C++ Dialects}.
> -pedantic-errors -fpermissive
> -w -Wextra -Wall -Wabi=@var{n}
> -Waddress -Wno-address-of-packed-member -Waggregate-return
> --Walloc-size-larger-than=@var{byte-size} -Walloc-zero
> +-Walloc-size -Walloc-size-larger-than=@var{byte-size} -Walloc-zero
> -Walloca -Walloca-larger-than=@var{byte-size}
> -Wno-aggressive-loop-optimizations
> -Warith-conversion
> @@ -344,6 +344,7 @@ Objective-C and Objective-C++ Dialects}.
> -Wc++20-compat
> -Wno-c++11-extensions -Wno-c++14-extensions -Wno-c++17-extensions
> -Wno-c++20-extensions -Wno-c++23-extensions
> +-Wcalloc-transposed-args
> -Wcast-align -Wcast-align=strict -Wcast-function-type -Wcast-qual
> -Wchar-subscripts
> -Wclobbered -Wcomment
> @@ -8260,8 +8261,7 @@ Warn about calls to allocation functions
> @code{alloc_size} that specify insufficient size for the target type of
> the pointer the result is assigned to, including those to the built-in
> forms of the functions @code{aligned_alloc}, @code{alloca},
> -@code{calloc},
> -@code{malloc}, and @code{realloc}.
> +@code{calloc}, @code{malloc}, and @code{realloc}.
>
> @opindex Wno-alloc-zero
> @opindex Walloc-zero
> @@ -8274,6 +8274,21 @@ when called with a zero size differs amo
> of @code{realloc} has been deprecated) relying on it may result in subtle
> portability bugs and should be avoided.
>
> +@opindex Wcalloc-transposed-args
> +@opindex Wno-calloc-transposed-args
> +@item -Wcalloc-transposed-args
> +Warn about calls to allocation functions decorated with attribute
> +@code{alloc_size} with two arguments, which use @code{sizeof} operator
> +as the earlier size argument and don't use it as the later size argument.
> +This is a coding style warning. The first argument to @code{calloc} is
> +documented to be number of elements in array, while the second argument
> +is size of each element, so @code{calloc (@var{n}, sizeof (int))} is
> preferred
> +over @code{calloc (sizeof (int), @var{n})}. If @code{sizeof} in the earlier
> +argument and not the latter is intentional, the warning can be suppressed
> +by using @code{calloc (sizeof (struct @var{S}) + 0, n)} or
> +@code{calloc (1 * sizeof (struct @var{S}), 4)} or using @code{sizeof} in the
> +later argument as well.
> +
> @opindex Walloc-size-larger-than=
> @opindex Wno-alloc-size-larger-than
> @item -Walloc-size-larger-than=@var{byte-size}
> --- gcc/c-family/c.opt.jj 2023-12-14 07:49:52.951583511 +0100
> +++ gcc/c-family/c.opt 2023-12-18 13:57:11.834184689 +0100
> @@ -502,6 +502,10 @@ Wc++26-extensions
> C++ ObjC++ Var(warn_cxx26_extensions) Warning Init(1)
> Warn about C++26 constructs in code compiled with an older standard.
>
> +Wcalloc-transposed-args
> +C ObjC C++ ObjC++ Var(warn_calloc_transposed_args) Warning LangEnabledBy(C
> ObjC C++ ObjC++,Wextra)
> +Warn about suspicious calls to calloc-like functions where sizeof expression
> is the earlier size argument and not the latter.
> +
> Wcast-function-type
> C ObjC C++ ObjC++ Var(warn_cast_function_type) Warning EnabledBy(Wextra)
> Warn about casts between incompatible function types.
> --- gcc/c-family/c-common.h.jj 2023-12-14 07:49:52.915584002 +0100
> +++ gcc/c-family/c-common.h 2023-12-18 16:26:40.420196735 +0100
> @@ -1599,6 +1599,9 @@ extern void warn_about_parentheses (loca
> extern void warn_for_unused_label (tree label);
> extern void warn_for_div_by_zero (location_t, tree divisor);
> extern void warn_for_memset (location_t, tree, tree, int);
> +extern void warn_for_calloc (location_t *, tree, vec<tree, va_gc> *,
> + tree *, tree);
> +extern void warn_for_alloc_size (location_t, tree, tree, tree);
> extern void warn_for_sign_compare (location_t,
> tree orig_op0, tree orig_op1,
> tree op0, tree op1,
> --- gcc/c-family/c-warn.cc.jj 2023-12-14 07:49:52.951583511 +0100
> +++ gcc/c-family/c-warn.cc 2023-12-18 19:30:47.285607029 +0100
> @@ -2263,6 +2263,76 @@ warn_for_memset (location_t loc, tree ar
> }
> }
>
> +/* Warn for calloc (sizeof (X), n). */
> +
> +void
> +warn_for_calloc (location_t *sizeof_arg_loc, tree callee,
> + vec<tree, va_gc> *params, tree *sizeof_arg, tree attr)
> +{
> + if (!TREE_VALUE (attr) || !TREE_CHAIN (TREE_VALUE (attr)))
> + return;
> +
> + int arg1 = TREE_INT_CST_LOW (TREE_VALUE (TREE_VALUE (attr))) - 1;
> + int arg2
> + = TREE_INT_CST_LOW (TREE_VALUE (TREE_CHAIN (TREE_VALUE (attr)))) - 1;
> + if (arg1 < 0
> + || (unsigned) arg1 >= vec_safe_length (params)
> + || arg1 >= 6
> + || arg2 < 0
> + || (unsigned) arg2 >= vec_safe_length (params)
> + || arg2 >= 6
> + || arg1 >= arg2)
> + return;
> +
> + if (sizeof_arg[arg1] == NULL_TREE || sizeof_arg[arg2] != NULL_TREE)
> + return;
> +
> + if (warning_at (sizeof_arg_loc[arg1], OPT_Wcalloc_transposed_args,
> + "%qD sizes specified with %<sizeof%> in the earlier "
> + "argument and not in the later argument", callee))
> + inform (sizeof_arg_loc[arg1], "earlier argument should specify number "
> + "of elements, later size of each element");
> +}
> +
> +/* Warn for allocator calls where the constant allocated size is smaller
> + than sizeof (TYPE). */
> +
> +void
> +warn_for_alloc_size (location_t loc, tree type, tree call, tree alloc_size)
> +{
> + if (!TREE_VALUE (alloc_size))
> + return;
> +
> + tree arg1 = TREE_VALUE (TREE_VALUE (alloc_size));
> + int idx1 = TREE_INT_CST_LOW (arg1) - 1;
> + if (idx1 < 0 || idx1 >= call_expr_nargs (call))
> + return;
> + arg1 = CALL_EXPR_ARG (call, idx1);
> + if (TREE_CODE (arg1) != INTEGER_CST)
> + return;
> + if (TREE_CHAIN (TREE_VALUE (alloc_size)))
> + {
> + tree arg2 = TREE_VALUE (TREE_CHAIN (TREE_VALUE (alloc_size)));
> + int idx2 = TREE_INT_CST_LOW (arg2) - 1;
> + if (idx2 < 0 || idx2 >= call_expr_nargs (call))
> + return;
> + arg2 = CALL_EXPR_ARG (call, idx2);
> + if (TREE_CODE (arg2) != INTEGER_CST)
> + return;
> + arg1 = int_const_binop (MULT_EXPR, fold_convert (sizetype, arg1),
> + fold_convert (sizetype, arg2));
> + if (TREE_CODE (arg1) != INTEGER_CST)
> + return;
> + }
> + if (!VOID_TYPE_P (type)
> + && TYPE_SIZE_UNIT (type)
> + && TREE_CODE (TYPE_SIZE_UNIT (type)) == INTEGER_CST
> + && tree_int_cst_lt (arg1, TYPE_SIZE_UNIT (type)))
> + warning_at (loc, OPT_Walloc_size,
> + "allocation of insufficient size %qE for type %qT with "
> + "size %qE", arg1, type, TYPE_SIZE_UNIT (type));
> +}
> +
> /* Subroutine of build_binary_op. Give warnings for comparisons
> between signed and unsigned quantities that may fail. Do the
> checking based on the original operand trees ORIG_OP0 and ORIG_OP1,
> --- gcc/c/c-parser.cc.jj 2023-12-14 07:49:52.969583266 +0100
> +++ gcc/c/c-parser.cc 2023-12-18 19:24:01.508298779 +0100
> @@ -12478,8 +12478,8 @@ c_parser_postfix_expression_after_primar
> {
> struct c_expr orig_expr;
> tree ident, idx;
> - location_t sizeof_arg_loc[3], comp_loc;
> - tree sizeof_arg[3];
> + location_t sizeof_arg_loc[6], comp_loc;
> + tree sizeof_arg[6];
> unsigned int literal_zero_mask;
> unsigned int i;
> vec<tree, va_gc> *exprlist;
> @@ -12512,7 +12512,7 @@ c_parser_postfix_expression_after_primar
> {
> matching_parens parens;
> parens.consume_open (parser);
> - for (i = 0; i < 3; i++)
> + for (i = 0; i < 6; i++)
> {
> sizeof_arg[i] = NULL_TREE;
> sizeof_arg_loc[i] = UNKNOWN_LOCATION;
> @@ -12577,6 +12577,13 @@ c_parser_postfix_expression_after_primar
> "not permitted in intervening code");
> parser->omp_for_parse_state->fail = true;
> }
> + if (warn_calloc_transposed_args)
> + if (tree attr = lookup_attribute ("alloc_size",
> + TYPE_ATTRIBUTES
> + (TREE_TYPE (expr.value))))
> + if (TREE_VALUE (attr) && TREE_CHAIN (TREE_VALUE (attr)))
> + warn_for_calloc (sizeof_arg_loc, expr.value, exprlist,
> + sizeof_arg, attr);
> }
>
> start = expr.get_start ();
> @@ -12861,7 +12868,7 @@ c_parser_expr_list (c_parser *parser, bo
> vec_safe_push (orig_types, expr.original_type);
> if (locations)
> locations->safe_push (expr.get_location ());
> - if (++idx < 3
> + if (++idx < 6
> && sizeof_arg != NULL
> && (expr.original_code == SIZEOF_EXPR
> || expr.original_code == PAREN_SIZEOF_EXPR))
> --- gcc/c/c-typeck.cc.jj 2023-12-14 07:49:52.990582980 +0100
> +++ gcc/c/c-typeck.cc 2023-12-18 19:01:40.013746860 +0100
> @@ -6054,6 +6054,19 @@ build_c_cast (location_t loc, tree type,
> c_addr_space_name (as_to),
> c_addr_space_name (as_from));
> }
> +
> + /* Warn of new allocations that are not big enough for the target
> + type. */
> + if (warn_alloc_size && TREE_CODE (value) == CALL_EXPR)
> + if (tree fndecl = get_callee_fndecl (value))
> + if (DECL_IS_MALLOC (fndecl))
> + {
> + tree attrs = TYPE_ATTRIBUTES (TREE_TYPE (fndecl));
> + tree alloc_size = lookup_attribute ("alloc_size", attrs);
> + if (alloc_size)
> + warn_for_alloc_size (loc, TREE_TYPE (type), value,
> + alloc_size);
> + }
> }
>
> /* Warn about possible alignment problems. */
> @@ -7277,32 +7290,15 @@ convert_for_assignment (location_t locat
>
> /* Warn of new allocations that are not big enough for the target
> type. */
> - tree fndecl;
> - if (warn_alloc_size
> - && TREE_CODE (rhs) == CALL_EXPR
> - && (fndecl = get_callee_fndecl (rhs)) != NULL_TREE
> - && DECL_IS_MALLOC (fndecl))
> - {
> - tree fntype = TREE_TYPE (fndecl);
> - tree fntypeattrs = TYPE_ATTRIBUTES (fntype);
> - tree alloc_size = lookup_attribute ("alloc_size", fntypeattrs);
> - if (alloc_size)
> + if (warn_alloc_size && TREE_CODE (rhs) == CALL_EXPR)
> + if (tree fndecl = get_callee_fndecl (rhs))
> + if (DECL_IS_MALLOC (fndecl))
> {
> - tree args = TREE_VALUE (alloc_size);
> - int idx = TREE_INT_CST_LOW (TREE_VALUE (args)) - 1;
> - /* For calloc only use the second argument. */
> - if (TREE_CHAIN (args))
> - idx = TREE_INT_CST_LOW (TREE_VALUE (TREE_CHAIN (args))) - 1;
> - tree arg = CALL_EXPR_ARG (rhs, idx);
> - if (TREE_CODE (arg) == INTEGER_CST
> - && !VOID_TYPE_P (ttl) && TYPE_SIZE_UNIT (ttl)
> - && INTEGER_CST == TREE_CODE (TYPE_SIZE_UNIT (ttl))
> - && tree_int_cst_lt (arg, TYPE_SIZE_UNIT (ttl)))
> - warning_at (location, OPT_Walloc_size, "allocation of "
> - "insufficient size %qE for type %qT with "
> - "size %qE", arg, ttl, TYPE_SIZE_UNIT (ttl));
> + tree attrs = TYPE_ATTRIBUTES (TREE_TYPE (fndecl));
> + tree alloc_size = lookup_attribute ("alloc_size", attrs);
> + if (alloc_size)
> + warn_for_alloc_size (location, ttl, rhs, alloc_size);
> }
> - }
>
> /* See if the pointers point to incompatible address spaces. */
> asl = TYPE_ADDR_SPACE (ttl);
> --- gcc/testsuite/gcc.dg/Walloc-size-4.c.jj 2023-12-18 19:06:41.069528176
> +0100
> +++ gcc/testsuite/gcc.dg/Walloc-size-4.c 2023-12-18 19:34:06.964824180
> +0100
> @@ -0,0 +1,54 @@
> +/* Tests the warnings for insufficient allocation size. */
> +/* { dg-do compile } */
> +/* { dg-options "-Walloc-size -Wno-calloc-transposed-args" } */
> +
> +struct S { int x[10]; };
> +void bar (struct S *);
> +typedef __SIZE_TYPE__ size_t;
> +void *myfree (void *, int, int);
> +void *mymalloc (int, int, size_t) __attribute__((malloc, malloc (myfree),
> alloc_size (3)));
> +void *mycalloc (int, int, size_t, size_t) __attribute__((malloc, malloc
> (myfree), alloc_size (3, 4)));
> +
> +void
> +foo (void)
> +{
> + struct S *p = (struct S *) __builtin_malloc (sizeof *p);
> + __builtin_free (p);
> + p = (struct S *) __builtin_malloc (sizeof p); /* { dg-warning
> "allocation of insufficient size" } */
> + __builtin_free (p);
> + p = (struct S *) __builtin_alloca (sizeof p); /* { dg-warning
> "allocation of insufficient size" } */
> + bar (p);
> + p = (struct S *) __builtin_calloc (1, sizeof p); /* { dg-warning
> "allocation of insufficient size" } */
> + __builtin_free (p);
> + bar ((struct S *) __builtin_malloc (4)); /* { dg-warning
> "allocation of insufficient size" } */
> + __builtin_free (p);
> + p = (struct S *) __builtin_calloc (sizeof *p, 1);
> + __builtin_free (p);
> + p = __builtin_calloc (sizeof *p, 1);
> + __builtin_free (p);
> +}
> +
> +void
> +baz (void)
> +{
> + struct S *p = (struct S *) mymalloc (42, 42, sizeof *p);
> + myfree (p, 42, 42);
> + p = (struct S *) mymalloc (42, 42, sizeof p); /* { dg-warning
> "allocation of insufficient size" } */
> + myfree (p, 42, 42);
> + p = (struct S *) mycalloc (42, 42, 1, sizeof p); /* { dg-warning
> "allocation of insufficient size" } */
> + myfree (p, 42, 42);
> + bar ((struct S *) mymalloc (42, 42, 4)); /* { dg-warning
> "allocation of insufficient size" } */
> + myfree (p, 42, 42);
> + p = (struct S *) mycalloc (42, 42, sizeof *p, 1);
> + myfree (p, 42, 42);
> + p = mycalloc (42, 42, sizeof *p, 1);
> + myfree (p, 42, 42);
> + p = mymalloc (42, 42, sizeof *p);
> + myfree (p, 42, 42);
> + p = mymalloc (42, 42, sizeof p); /* { dg-warning
> "allocation of insufficient size" } */
> + myfree (p, 42, 42);
> + p = mycalloc (42, 42, 1, sizeof p); /* { dg-warning
> "allocation of insufficient size" } */
> + myfree (p, 42, 42);
> + bar (mymalloc (42, 42, 4)); /* { dg-warning
> "allocation of insufficient size" } */
> + myfree (p, 42, 42);
> +}
> --- gcc/testsuite/gcc.dg/Walloc-size-5.c.jj 2023-12-18 19:18:13.451180896
> +0100
> +++ gcc/testsuite/gcc.dg/Walloc-size-5.c 2023-12-18 19:19:25.244173868
> +0100
> @@ -0,0 +1,20 @@
> +/* { dg-do compile } */
> +/* { dg-options "-Walloc-size -std=gnu11" } */
> +
> +struct S { int x[10]; };
> +void myfree ();
> +void *mymalloc () __attribute__((malloc, alloc_size (16)));
> +void *mycalloc () __attribute__((malloc, alloc_size (16, 17)));
> +
> +void
> +foo (void)
> +{
> + struct S *p = mymalloc (1);
> + myfree (p);
> + p = mycalloc (1, 1);
> + myfree (p);
> + p = (struct S *) mymalloc (1);
> + myfree (p);
> + p = (struct S *) mycalloc (1, 1);
> + myfree (p);
> +}
> --- gcc/testsuite/gcc.dg/Wcalloc-transposed-args-1.c.jj 2023-12-18
> 19:39:42.930196797 +0100
> +++ gcc/testsuite/gcc.dg/Wcalloc-transposed-args-1.c 2023-12-18
> 19:52:03.273855456 +0100
> @@ -0,0 +1,54 @@
> +/* { dg-do compile } */
> +/* { dg-options "-Wcalloc-transposed-args" } */
> +
> +typedef __SIZE_TYPE__ size_t;
> +void free (void *);
> +void *calloc (size_t, size_t);
> +void *myfree (void *, int, int);
> +void *mycalloc (int, int, size_t, size_t) __attribute__((malloc, malloc
> (myfree), alloc_size (3, 4)));
> +
> +void
> +foo (int n)
> +{
> + void *p;
> + p = __builtin_calloc (1, sizeof (int));
> + __builtin_free (p);
> + p = __builtin_calloc (n, sizeof (int));
> + __builtin_free (p);
> + p = __builtin_calloc (sizeof (int), 1); /* { dg-warning
> "'__builtin_calloc' sizes specified with 'sizeof' in the earlier argument and
> not in the later argument" } */
> + __builtin_free (p); /* { dg-message
> "earlier argument should specify number of elements, later size of each
> element" "" { target *-*-* } .-1 } */
> + p = __builtin_calloc (sizeof (int), n); /* { dg-warning
> "'__builtin_calloc' sizes specified with 'sizeof' in the earlier argument and
> not in the later argument" } */
> + __builtin_free (p); /* { dg-message
> "earlier argument should specify number of elements, later size of each
> element" "" { target *-*-* } .-1 } */
> + p = __builtin_calloc ((sizeof (int)), 1); /* { dg-warning
> "'__builtin_calloc' sizes specified with 'sizeof' in the earlier argument and
> not in the later argument" } */
> + __builtin_free (p); /* { dg-message
> "earlier argument should specify number of elements, later size of each
> element" "" { target *-*-* } .-1 } */
> + p = __builtin_calloc (sizeof (int) + 0, 1);
> + __builtin_free (p);
> + p = __builtin_calloc (sizeof (int), sizeof (char));
> + __builtin_free (p);
> + p = __builtin_calloc (1 * sizeof (int), 1);
> + __builtin_free (p);
> + p = calloc (1, sizeof (int));
> + free (p);
> + p = calloc (n, sizeof (int));
> + free (p);
> + p = calloc (sizeof (int), 1); /* { dg-warning
> "'calloc' sizes specified with 'sizeof' in the earlier argument and not in
> the later argument" } */
> + free (p); /* { dg-message
> "earlier argument should specify number of elements, later size of each
> element" "" { target *-*-* } .-1 } */
> + p = calloc (sizeof (int), n); /* { dg-warning
> "'calloc' sizes specified with 'sizeof' in the earlier argument and not in
> the later argument" } */
> + free (p); /* { dg-message
> "earlier argument should specify number of elements, later size of each
> element" "" { target *-*-* } .-1 } */
> + p = calloc (sizeof (int), sizeof (char));
> + free (p);
> + p = calloc (1 * sizeof (int), 1);
> + free (p);
> + p = mycalloc (42, 42, 1, sizeof (int));
> + myfree (p, 42, 42);
> + p = mycalloc (42, 42, n, sizeof (int));
> + myfree (p, 42, 42);
> + p = mycalloc (42, 42, sizeof (int), 1); /* { dg-warning
> "'mycalloc' sizes specified with 'sizeof' in the earlier argument and not in
> the later argument" } */
> + myfree (p, 42, 42); /* { dg-message
> "earlier argument should specify number of elements, later size of each
> element" "" { target *-*-* } .-1 } */
> + p = mycalloc (42, 42, sizeof (int), n); /* { dg-warning
> "'mycalloc' sizes specified with 'sizeof' in the earlier argument and not in
> the later argument" } */
> + myfree (p, 42, 42); /* { dg-message
> "earlier argument should specify number of elements, later size of each
> element" "" { target *-*-* } .-1 } */
> + p = mycalloc (42, 42, sizeof (int), sizeof (char));
> + myfree (p, 42, 42);
> + p = mycalloc (42, 42, 1 * sizeof (int), 1);
> + myfree (p, 42, 42);
> +}
>
> Jakub
>
