* [PATCH] c: Split -Wcalloc-transposed-args warning from -Walloc-size, -Walloc-size fixes
@ 2023-12-18 19:14 Jakub Jelinek
2023-12-19 7:11 ` Martin Uecker
2023-12-19 19:32 ` Joseph Myers
0 siblings, 2 replies; 7+ messages in thread
From: Jakub Jelinek @ 2023-12-18 19:14 UTC (permalink / raw)
To: Joseph S. Myers, Marek Polacek, Jason Merrill, Richard Biener,
Martin Uecker
Cc: gcc-patches
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).
Ok for trunk if this passes bootstrap/regtest?
If yes, I'd implement it for C++ next.
If not, we should at least fix the ICEs.
2023-12-18 Jakub Jelinek <jakub@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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] c: Split -Wcalloc-transposed-args warning from -Walloc-size, -Walloc-size fixes
2023-12-18 19:14 [PATCH] c: Split -Wcalloc-transposed-args warning from -Walloc-size, -Walloc-size fixes Jakub Jelinek
@ 2023-12-19 7:11 ` Martin Uecker
2023-12-19 8:47 ` Jakub Jelinek
2023-12-19 19:32 ` Joseph Myers
1 sibling, 1 reply; 7+ messages in thread
From: Martin Uecker @ 2023-12-19 7:11 UTC (permalink / raw)
To: Jakub Jelinek, Joseph S. Myers, Marek Polacek, Jason Merrill,
Richard Biener
Cc: gcc-patches
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 <jakub@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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] c: Split -Wcalloc-transposed-args warning from -Walloc-size, -Walloc-size fixes
2023-12-19 7:11 ` Martin Uecker
@ 2023-12-19 8:47 ` Jakub Jelinek
2023-12-19 10:34 ` Martin Uecker
2023-12-19 17:20 ` Jason Merrill
0 siblings, 2 replies; 7+ messages in thread
From: Jakub Jelinek @ 2023-12-19 8:47 UTC (permalink / raw)
To: Martin Uecker
Cc: Joseph S. Myers, Marek Polacek, Jason Merrill, Richard Biener,
gcc-patches
On Tue, Dec 19, 2023 at 08:11:11AM +0100, Martin Uecker wrote:
> 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.
If you have suggestions how to reword the documentation, would that be
sufficient for you? I still don't see why given correct alignment one can't
store struct S into sizeof (struct S) sized heap char array, but if the
documentation explain reasons why should one write it one way and not the
other except for coding style, sure.
> > Ok for trunk if this passes bootstrap/regtest?
>
> I wonder whether we could turn on -Walloc-size for -Wall with this change?
I think that is a possibility, yes.
BTW, the patch passed bootstrap/regtest on x86_64-linux and i686-linux.
Jakub
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] c: Split -Wcalloc-transposed-args warning from -Walloc-size, -Walloc-size fixes
2023-12-19 8:47 ` Jakub Jelinek
@ 2023-12-19 10:34 ` Martin Uecker
2023-12-19 17:20 ` Jason Merrill
1 sibling, 0 replies; 7+ messages in thread
From: Martin Uecker @ 2023-12-19 10:34 UTC (permalink / raw)
To: Jakub Jelinek
Cc: Joseph S. Myers, Marek Polacek, Jason Merrill, Richard Biener,
gcc-patches
Am Dienstag, dem 19.12.2023 um 09:47 +0100 schrieb Jakub Jelinek:
> On Tue, Dec 19, 2023 at 08:11:11AM +0100, Martin Uecker wrote:
> > 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.
>
> If you have suggestions how to reword the documentation, would that be
> sufficient for you?
Maybe simple remove "This is a coding style warning." ?
> I still don't see why given correct alignment one can't
> store struct S into sizeof (struct S) sized heap char array, but if the
> documentation explain reasons why should one write it one way and not the
> other except for coding style, sure.
I do not think we need to argue one way or the other in
the documentation.
>
> > > Ok for trunk if this passes bootstrap/regtest?
> >
> > I wonder whether we could turn on -Walloc-size for -Wall with this change?
>
> I think that is a possibility, yes.
>
> BTW, the patch passed bootstrap/regtest on x86_64-linux and i686-linux.
>
> Jakub
Anyway, thank you for fixing / improving this warning!
Martin
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] c: Split -Wcalloc-transposed-args warning from -Walloc-size, -Walloc-size fixes
2023-12-19 8:47 ` Jakub Jelinek
2023-12-19 10:34 ` Martin Uecker
@ 2023-12-19 17:20 ` Jason Merrill
2023-12-19 17:45 ` Martin Uecker
1 sibling, 1 reply; 7+ messages in thread
From: Jason Merrill @ 2023-12-19 17:20 UTC (permalink / raw)
To: Jakub Jelinek, Martin Uecker
Cc: Joseph S. Myers, Marek Polacek, Richard Biener, gcc-patches
On 12/19/23 03:47, Jakub Jelinek wrote:
> On Tue, Dec 19, 2023 at 08:11:11AM +0100, Martin Uecker wrote:
>> 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.
>
> If you have suggestions how to reword the documentation, would that be
> sufficient for you? I still don't see why given correct alignment one can't
> store struct S into sizeof (struct S) sized heap char array,
Seems to me one can in C++, anyway. An unsigned char array can provide
storage for another type, and the call to calloc can be interpreted as
creating such an array if that gives the program defined behavior.
https://eel.is/c++draft/intro.object#def:provides_storage
https://eel.is/c++draft/intro.object#def:object,implicit_creation
Jason
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] c: Split -Wcalloc-transposed-args warning from -Walloc-size, -Walloc-size fixes
2023-12-19 17:20 ` Jason Merrill
@ 2023-12-19 17:45 ` Martin Uecker
0 siblings, 0 replies; 7+ messages in thread
From: Martin Uecker @ 2023-12-19 17:45 UTC (permalink / raw)
To: Jason Merrill, Jakub Jelinek
Cc: Joseph S. Myers, Marek Polacek, Richard Biener, gcc-patches
Am Dienstag, dem 19.12.2023 um 12:20 -0500 schrieb Jason Merrill:
> On 12/19/23 03:47, Jakub Jelinek wrote:
> > On Tue, Dec 19, 2023 at 08:11:11AM +0100, Martin Uecker wrote:
> > > 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.
> >
> > If you have suggestions how to reword the documentation, would that be
> > sufficient for you? I still don't see why given correct alignment one can't
> > store struct S into sizeof (struct S) sized heap char array,
>
> Seems to me one can in C++, anyway. An unsigned char array can provide
> storage for another type, and the call to calloc can be interpreted as
> creating such an array if that gives the program defined behavior.
> https://eel.is/c++draft/intro.object#def:provides_storage
> https://eel.is/c++draft/intro.object#def:object,implicit_creation
This is also true in C. There is nothing wrong with calloc(10, 1)
allocating a char array with 10 elements and then storing a struct
of size 10 in it.
Martin
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] c: Split -Wcalloc-transposed-args warning from -Walloc-size, -Walloc-size fixes
2023-12-18 19:14 [PATCH] c: Split -Wcalloc-transposed-args warning from -Walloc-size, -Walloc-size fixes Jakub Jelinek
2023-12-19 7:11 ` Martin Uecker
@ 2023-12-19 19:32 ` Joseph Myers
1 sibling, 0 replies; 7+ messages in thread
From: Joseph Myers @ 2023-12-19 19:32 UTC (permalink / raw)
To: Jakub Jelinek
Cc: Marek Polacek, Jason Merrill, Richard Biener, Martin Uecker, gcc-patches
On Mon, 18 Dec 2023, Jakub Jelinek wrote:
> 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).
>
> Ok for trunk if this passes bootstrap/regtest?
OK.
--
Joseph S. Myers
joseph@codesourcery.com
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-12-19 19:32 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-12-18 19:14 [PATCH] c: Split -Wcalloc-transposed-args warning from -Walloc-size, -Walloc-size fixes Jakub Jelinek
2023-12-19 7:11 ` Martin Uecker
2023-12-19 8:47 ` Jakub Jelinek
2023-12-19 10:34 ` Martin Uecker
2023-12-19 17:20 ` Jason Merrill
2023-12-19 17:45 ` Martin Uecker
2023-12-19 19:32 ` Joseph Myers
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).