Message ID | ZYM+cZIJOmWDZTSH@tucnak |
---|---|
State | New |
Headers | show |
Series | c++: Enable -Walloc-size and -Wcalloc-transposed-args warnings for C++ | expand |
On 12/20/23 14:20, Jakub Jelinek wrote: > Hi! > > The following patch enables the -Walloc-size and -Wcalloc-transposed-args > warnings for C++ as well. > > Ok for trunk if it passes bootstrap/regtest? > > 2023-12-20 Jakub Jelinek <jakub@redhat.com> > > gcc/c-family/ > * c.opt (Walloc-size): Enable also for C++ and ObjC++. > gcc/cp/ > * cp-gimplify.cc (cp_genericize_r): If warn_alloc_size, call > warn_for_alloc_size for -Walloc-size diagnostics. > * semantics.cc (finish_call_expr): If warn_calloc_transposed_args, > call warn_for_calloc for -Wcalloc-transposed-args diagnostics. > gcc/testsuite/ > * g++.dg/warn/Walloc-size-1.C: New test. > * g++.dg/warn/Wcalloc-transposed-args-1.C: New test. > > --- gcc/c-family/c.opt.jj 2023-12-20 11:31:07.897806698 +0100 > +++ gcc/c-family/c.opt 2023-12-20 20:02:36.889910599 +0100 > @@ -332,7 +332,7 @@ C ObjC C++ ObjC++ Var(warn_alloca) Warni > Warn on any use of alloca. > > Walloc-size > -C ObjC Var(warn_alloc_size) Warning LangEnabledBy(C ObjC, Wextra) > +C ObjC C++ ObjC++ Var(warn_alloc_size) Warning LangEnabledBy(C ObjC C++ ObjC++, Wextra) > Warn when allocating insufficient storage for the target type of the assigned pointer. > > Walloc-size-larger-than= > --- gcc/cp/cp-gimplify.cc.jj 2023-12-15 10:08:53.877236695 +0100 > +++ gcc/cp/cp-gimplify.cc 2023-12-20 20:10:59.689914648 +0100 > @@ -2048,6 +2048,25 @@ cp_genericize_r (tree *stmt_p, int *walk > > case NOP_EXPR: > *stmt_p = predeclare_vla (*stmt_p); > + > + /* Warn of new allocations that are not big enough for the target > + type. */ > + if (warn_alloc_size > + && TREE_CODE (TREE_OPERAND (stmt, 0)) == CALL_EXPR > + && POINTER_TYPE_P (TREE_TYPE (stmt))) > + { > + if (tree fndecl = get_callee_fndecl (TREE_OPERAND (stmt, 0))) > + 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 (EXPR_LOCATION (stmt), > + TREE_TYPE (TREE_TYPE (stmt)), > + TREE_OPERAND (stmt, 0), alloc_size); > + } > + } > + > if (!wtd->no_sanitize_p > && sanitize_flags_p (SANITIZE_NULL | SANITIZE_ALIGNMENT) > && TYPE_REF_P (TREE_TYPE (stmt))) > --- gcc/cp/semantics.cc.jj 2023-12-14 07:49:53.150580801 +0100 > +++ gcc/cp/semantics.cc 2023-12-20 20:13:33.773772759 +0100 > @@ -2939,15 +2939,24 @@ finish_call_expr (tree fn, vec<tree, va_ > > if (!result) > { > - if (warn_sizeof_pointer_memaccess > + tree alloc_size_attr = NULL_TREE; > + if (warn_calloc_transposed_args > + && TREE_CODE (fn) == FUNCTION_DECL > + && (alloc_size_attr > + = lookup_attribute ("alloc_size", > + TYPE_ATTRIBUTES (TREE_TYPE (fn))))) > + if (TREE_VALUE (alloc_size_attr) == NULL_TREE > + || TREE_CHAIN (TREE_VALUE (alloc_size_attr)) == NULL_TREE) > + alloc_size_attr = NULL_TREE; > + if ((warn_sizeof_pointer_memaccess || alloc_size_attr) > && (complain & tf_warning) > && !vec_safe_is_empty (*args) > && !processing_template_decl) > { > - location_t sizeof_arg_loc[3]; > - tree sizeof_arg[3]; > + location_t sizeof_arg_loc[6]; > + tree sizeof_arg[6]; Why do we need to check 6 args for calloc? The patch is OK, just wondering. Jason
On Wed, Dec 20, 2023 at 05:45:05PM -0500, Jason Merrill wrote: > On 12/20/23 14:20, Jakub Jelinek wrote: > > + if ((warn_sizeof_pointer_memaccess || alloc_size_attr) > > && (complain & tf_warning) > > && !vec_safe_is_empty (*args) > > && !processing_template_decl) > > { > > - location_t sizeof_arg_loc[3]; > > - tree sizeof_arg[3]; > > + location_t sizeof_arg_loc[6]; > > + tree sizeof_arg[6]; > > Why do we need to check 6 args for calloc? The patch is OK, just wondering. The 3 for warn_sizeof_pointer_memaccess comes from the fact that we only warn on certain builtins and don't really need it on 4th and later arguments. For the calloc case it warns about any 2 argument alloc_size attribute, so generally it could be even INT_MAX or so; while for the C++ FE which still has SIZEOF_EXPRs around it could be implementable if the function is adjusted to be called with different arguments, for the C FE we need to remember whether each argument was a sizeof or not because we fold everything immediately. I've grepped my /usr/include/ with what arguments alloc_size attribute is used and found some 1,2 and 2,3 and 3,4 cases, so I went for 6 as a compromise between not wasting too much compile time on it vs. covering majority of functions with alloc_size 2 argument attributes. It is unlikely allocation functions would need dozens of arguments... Anyway, bootstrap fails with the patch due to: ../../gcc/gimple-fold.cc:7089:15: error: allocation of insufficient size ‘72’ for type ‘tree_node’ with size ‘216’ [-Werror=alloc-size] ../../gcc/gimple-fold.cc:7096:15: error: allocation of insufficient size ‘72’ for type ‘tree_node’ with size ‘216’ [-Werror=alloc-size] This is on /* Allocate SSA names(lhs1) on the stack. */ tree lhs1 = (tree)XALLOCA (tree_ssa_name); memset (lhs1, 0, sizeof (tree_ssa_name)); TREE_SET_CODE (lhs1, SSA_NAME); TREE_TYPE (lhs1) = type; init_ssa_name_imm_use (lhs1); where tree is a union and we don't allocate enough memory for the whole union, just for one member of it. Guess that is a case the warning is meant to warn about, so we need some workaround. I find it really weird to allocate constant 72 bytes using alloca... Then there is also: ../../gcc/collect2.cc:625:39: error: ‘void* xcalloc(size_t, size_t)’ sizes specified with ‘sizeof’ in the earlier argument and not in the later argument [-Werror=calloc-transposed-args] which is what the warning is meant to warn about. So perhaps incremental: 2023-12-21 Jakub Jelinek <jakub@redhat.com> * gimple-fold.cc (maybe_fold_comparisons_from_match_pd): Use unsigned char buffers for lhs1 and lhs2 instead of allocating them through XALLOCA. * collect2.cc (maybe_run_lto_and_relink): Swap xcalloc arguments. --- gcc/gimple-fold.cc.jj 2023-12-17 22:50:01.077589250 +0100 +++ gcc/gimple-fold.cc 2023-12-21 00:25:26.254807466 +0100 @@ -7086,14 +7086,16 @@ maybe_fold_comparisons_from_match_pd (tr gimple_set_bb (stmt2, NULL); /* Allocate SSA names(lhs1) on the stack. */ - tree lhs1 = (tree)XALLOCA (tree_ssa_name); + alignas (tree_node) unsigned char lhs1buf[sizeof (tree_ssa_name)]; + tree lhs1 = (tree) &lhs1buf[0]; memset (lhs1, 0, sizeof (tree_ssa_name)); TREE_SET_CODE (lhs1, SSA_NAME); TREE_TYPE (lhs1) = type; init_ssa_name_imm_use (lhs1); /* Allocate SSA names(lhs2) on the stack. */ - tree lhs2 = (tree)XALLOCA (tree_ssa_name); + alignas (tree_node) unsigned char lhs2buf[sizeof (tree_ssa_name)]; + tree lhs2 = (tree) &lhs2buf[0]; memset (lhs2, 0, sizeof (tree_ssa_name)); TREE_SET_CODE (lhs2, SSA_NAME); TREE_TYPE (lhs2) = type; --- gcc/collect2.cc.jj 2023-11-10 22:48:01.356867230 +0100 +++ gcc/collect2.cc 2023-12-21 00:18:31.821159336 +0100 @@ -622,7 +622,7 @@ maybe_run_lto_and_relink (char **lto_ld_ LTO object replaced by all partitions and other LTO objects removed. */ - lto_c_argv = (char **) xcalloc (sizeof (char *), num_lto_c_args); + lto_c_argv = (char **) xcalloc (num_lto_c_args, sizeof (char *)); lto_c_ptr = CONST_CAST2 (const char **, char **, lto_c_argv); *lto_c_ptr++ = lto_wrapper; Jakub
On 12/20/23 18:29, Jakub Jelinek wrote: > On Wed, Dec 20, 2023 at 05:45:05PM -0500, Jason Merrill wrote: >> On 12/20/23 14:20, Jakub Jelinek wrote: >>> + if ((warn_sizeof_pointer_memaccess || alloc_size_attr) >>> && (complain & tf_warning) >>> && !vec_safe_is_empty (*args) >>> && !processing_template_decl) >>> { >>> - location_t sizeof_arg_loc[3]; >>> - tree sizeof_arg[3]; >>> + location_t sizeof_arg_loc[6]; >>> + tree sizeof_arg[6]; >> >> Why do we need to check 6 args for calloc? The patch is OK, just wondering. > > The 3 for warn_sizeof_pointer_memaccess comes from the fact that we only > warn on certain builtins and don't really need it on 4th and later > arguments. > For the calloc case it warns about any 2 argument alloc_size attribute, so > generally it could be even INT_MAX or so; while for the C++ FE which > still has SIZEOF_EXPRs around it could be implementable if the function is > adjusted to be called with different arguments, for the C FE we need to > remember whether each argument was a sizeof or not because we fold > everything immediately. > I've grepped my /usr/include/ with what arguments alloc_size attribute is > used and found some 1,2 and 2,3 and 3,4 cases, so I went for 6 as a > compromise between not wasting too much compile time on it vs. covering > majority of functions with alloc_size 2 argument attributes. > It is unlikely allocation functions would need dozens of arguments... > > Anyway, bootstrap fails with the patch due to: > ../../gcc/gimple-fold.cc:7089:15: error: allocation of insufficient size ‘72’ for type ‘tree_node’ with size ‘216’ [-Werror=alloc-size] > ../../gcc/gimple-fold.cc:7096:15: error: allocation of insufficient size ‘72’ for type ‘tree_node’ with size ‘216’ [-Werror=alloc-size] > This is on > /* Allocate SSA names(lhs1) on the stack. */ > tree lhs1 = (tree)XALLOCA (tree_ssa_name); > memset (lhs1, 0, sizeof (tree_ssa_name)); > TREE_SET_CODE (lhs1, SSA_NAME); > TREE_TYPE (lhs1) = type; > init_ssa_name_imm_use (lhs1); > where tree is a union and we don't allocate enough memory for the > whole union, just for one member of it. > Guess that is a case the warning is meant to warn about, > so we need some workaround. I find it really weird to allocate > constant 72 bytes using alloca... > > Then there is also: > ../../gcc/collect2.cc:625:39: error: ‘void* xcalloc(size_t, size_t)’ sizes specified with ‘sizeof’ in the earlier argument and not in the later argument [-Werror=calloc-transposed-args] > which is what the warning is meant to warn about. > > So perhaps incremental: LGTM. Jason
--- gcc/c-family/c.opt.jj 2023-12-20 11:31:07.897806698 +0100 +++ gcc/c-family/c.opt 2023-12-20 20:02:36.889910599 +0100 @@ -332,7 +332,7 @@ C ObjC C++ ObjC++ Var(warn_alloca) Warni Warn on any use of alloca. Walloc-size -C ObjC Var(warn_alloc_size) Warning LangEnabledBy(C ObjC, Wextra) +C ObjC C++ ObjC++ Var(warn_alloc_size) Warning LangEnabledBy(C ObjC C++ ObjC++, Wextra) Warn when allocating insufficient storage for the target type of the assigned pointer. Walloc-size-larger-than= --- gcc/cp/cp-gimplify.cc.jj 2023-12-15 10:08:53.877236695 +0100 +++ gcc/cp/cp-gimplify.cc 2023-12-20 20:10:59.689914648 +0100 @@ -2048,6 +2048,25 @@ cp_genericize_r (tree *stmt_p, int *walk case NOP_EXPR: *stmt_p = predeclare_vla (*stmt_p); + + /* Warn of new allocations that are not big enough for the target + type. */ + if (warn_alloc_size + && TREE_CODE (TREE_OPERAND (stmt, 0)) == CALL_EXPR + && POINTER_TYPE_P (TREE_TYPE (stmt))) + { + if (tree fndecl = get_callee_fndecl (TREE_OPERAND (stmt, 0))) + 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 (EXPR_LOCATION (stmt), + TREE_TYPE (TREE_TYPE (stmt)), + TREE_OPERAND (stmt, 0), alloc_size); + } + } + if (!wtd->no_sanitize_p && sanitize_flags_p (SANITIZE_NULL | SANITIZE_ALIGNMENT) && TYPE_REF_P (TREE_TYPE (stmt))) --- gcc/cp/semantics.cc.jj 2023-12-14 07:49:53.150580801 +0100 +++ gcc/cp/semantics.cc 2023-12-20 20:13:33.773772759 +0100 @@ -2939,15 +2939,24 @@ finish_call_expr (tree fn, vec<tree, va_ if (!result) { - if (warn_sizeof_pointer_memaccess + tree alloc_size_attr = NULL_TREE; + if (warn_calloc_transposed_args + && TREE_CODE (fn) == FUNCTION_DECL + && (alloc_size_attr + = lookup_attribute ("alloc_size", + TYPE_ATTRIBUTES (TREE_TYPE (fn))))) + if (TREE_VALUE (alloc_size_attr) == NULL_TREE + || TREE_CHAIN (TREE_VALUE (alloc_size_attr)) == NULL_TREE) + alloc_size_attr = NULL_TREE; + if ((warn_sizeof_pointer_memaccess || alloc_size_attr) && (complain & tf_warning) && !vec_safe_is_empty (*args) && !processing_template_decl) { - location_t sizeof_arg_loc[3]; - tree sizeof_arg[3]; + location_t sizeof_arg_loc[6]; + tree sizeof_arg[6]; unsigned int i; - for (i = 0; i < 3; i++) + for (i = 0; i < (alloc_size_attr ? 6 : 3); i++) { tree t; @@ -2964,9 +2973,15 @@ finish_call_expr (tree fn, vec<tree, va_ sizeof_arg[i] = TREE_OPERAND (t, 0); sizeof_arg_loc[i] = EXPR_LOCATION (t); } - sizeof_pointer_memaccess_warning - (sizeof_arg_loc, fn, *args, - sizeof_arg, same_type_ignoring_top_level_qualifiers_p); + if (warn_sizeof_pointer_memaccess) + { + auto same_p = same_type_ignoring_top_level_qualifiers_p; + sizeof_pointer_memaccess_warning (sizeof_arg_loc, fn, *args, + sizeof_arg, same_p); + } + if (alloc_size_attr) + warn_for_calloc (sizeof_arg_loc, fn, *args, sizeof_arg, + alloc_size_attr); } if ((complain & tf_warning) --- gcc/testsuite/g++.dg/warn/Wcalloc-transposed-args-1.C.jj 2023-12-20 19:52:10.303641708 +0100 +++ gcc/testsuite/g++.dg/warn/Wcalloc-transposed-args-1.C 2023-12-20 20:16:09.239611657 +0100 @@ -0,0 +1,54 @@ +// { dg-do compile } +// { dg-options "-Wcalloc-transposed-args" } + +typedef __SIZE_TYPE__ size_t; +extern "C" void free (void *); +extern "C" 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); +} --- gcc/testsuite/g++.dg/warn/Walloc-size-1.C.jj 2023-12-20 19:51:45.828982748 +0100 +++ gcc/testsuite/g++.dg/warn/Walloc-size-1.C 2023-12-20 20:01:40.385697949 +0100 @@ -0,0 +1,52 @@ +// 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 (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) +{ + S *p = (S *) __builtin_malloc (sizeof *p); + __builtin_free (p); + p = (S *) __builtin_malloc (sizeof p); // { dg-warning "allocation of insufficient size" } + __builtin_free (p); + p = (S *) __builtin_alloca (sizeof p); // { dg-warning "allocation of insufficient size" } + bar (p); + p = (S *) __builtin_calloc (1, sizeof p); // { dg-warning "allocation of insufficient size" } + __builtin_free (p); + bar ((S *) __builtin_malloc (4)); // { dg-warning "allocation of insufficient size" } + __builtin_free (p); + p = (S *) __builtin_calloc (sizeof *p, 1); + __builtin_free (p); +} + +void +baz (void) +{ + S *p = (S *) mymalloc (42, 42, sizeof *p); + myfree (p, 42, 42); + p = (S *) mymalloc (42, 42, sizeof p); // { dg-warning "allocation of insufficient size" } + myfree (p, 42, 42); + p = (S *) mycalloc (42, 42, 1, sizeof p); // { dg-warning "allocation of insufficient size" } + myfree (p, 42, 42); + bar ((S *) mymalloc (42, 42, 4)); // { dg-warning "allocation of insufficient size" } + myfree (p, 42, 42); + p = (S *) mycalloc (42, 42, sizeof *p, 1); + myfree (p, 42, 42); + p = static_cast <S *> (mycalloc (42, 42, sizeof *p, 1)); + myfree (p, 42, 42); + p = static_cast <S *> (mymalloc (42, 42, sizeof *p)); + myfree (p, 42, 42); + p = static_cast <S *> (mymalloc (42, 42, sizeof p)); // { dg-warning "allocation of insufficient size" } + myfree (p, 42, 42); + p = static_cast <S *> (mycalloc (42, 42, 1, sizeof p)); // { dg-warning "allocation of insufficient size" } + myfree (p, 42, 42); + bar (static_cast <S *> (mymalloc (42, 42, 4))); // { dg-warning "allocation of insufficient size" } + myfree (p, 42, 42); +}