| Message ID | 3c256a9e-1ee2-8eb3-e772-6fb860e287ce@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | declare tree_to_shwi et al. nonnull and pure | expand |
On 9/26/18 5:54 PM, Martin Sebor wrote: > The attached patch adds attributes nonnull and pure to > tree_to_shwi() and a small number of other heavily used functions > that will benefit from both. > > First, I noticed that there are a bunch of functions in tree.c that > deal gracefully with null pointers right alongside a bunch of other > related functions that don't. > > For example, tree_fits_shwi_p() is null-safe but integer_zerop() > is not. There a number of others. I never remember which ones > are in which group and so I either add unnecessary checks or > forget to add them, for which we then all pay when the missing > check triggers an ICE. In patch reviews I see I'm not the only > one who's not sure. The attribute should help avoid some of > these problems: both visually and via warnings. > > Second, while testing the nonnull patch, I noticed that GCC would > not optimize some null tests after calls to nonnull functions that > take tree as an argument and that I expected it to optimize, like > > n = tree_to_shwi (TYPE_SIZE (type)); > if (TYPE_SIZE (type)) > ... > > The reason is that tree_to_shwi() isn't declared pure, even though > tree_fits_shwi_p() is (though as I mentioned, the latter is null > safe and so not declarted nonnull, and so it doesn't make the same > optimization possible). > > Tested on x86_64-linux. The patch exposed a couple of issues > in Ada. At least the first one is a false positive caused by > GCC being unaware that tree_fits_uhwi_p() returns false for > a null argument (propagating this knowledge via an attribute > seems like an opportunity for a future enhancement). > I suppressed the warning by introducing a local temporary. > > I suspect the second warning is caused by the Ada TYPE_RM_SIZE() > macro expanding to a conditional with an explicit null operand: > > #define TYPE_RM_SIZE(NODE) TYPE_RM_VALUE ((NODE), 0) > > #define TYPE_RM_VALUE(NODE, N) \ > (TYPE_RM_VALUES (NODE) \ > ? TREE_VEC_ELT (TYPE_RM_VALUES (NODE), (N)) : NULL_TREE) > > but I wasn't able to reduce it to a small test case to confirm > that. I suppressed this warning by introducing a temporary as > well. > > Martin > > gcc-tree-nonnull.diff > > gcc/ChangeLog: > > * tree.h (tree_to_shwi): Add attribute nonnull. > (tree_to_poly_int64, tree_to_uhwi, tree_to_poly_uint64): Same. > (integer_zerop, integer_onep, int_fits_type_p): Same. > > gcc/ada/ChangeLog: > > * gcc-interface/utils.c (make_packable_type): Introduce a temporary > to avoid -Wnonnull. > (unchecked_convert): Same. No doubt we have not been diligent about marking non-null, const, pure, etc over time. I thought we had warnings for functions which are const/pure but not suitably marked. Can you peek a bit at why those didn't trigger. See ipa-pure-const.c. Ignore the initial comment -- it applies to both functions and data. ISTM we could probably build a missing non-null attribute warning. If a NULL pointer argument unconditionally leads to an assert, then the function should be marked. Similarly if a pointer argument is unconditionally dereferenced, then it should be marked. I strongly suspect this would be too noisy to enable by default. ANyway, the patch is OK for the trunk. jeff
On 09/27/2018 12:25 PM, Jeff Law wrote: > On 9/26/18 5:54 PM, Martin Sebor wrote: >> The attached patch adds attributes nonnull and pure to >> tree_to_shwi() and a small number of other heavily used functions >> that will benefit from both. >> >> First, I noticed that there are a bunch of functions in tree.c that >> deal gracefully with null pointers right alongside a bunch of other >> related functions that don't. >> >> For example, tree_fits_shwi_p() is null-safe but integer_zerop() >> is not. There a number of others. I never remember which ones >> are in which group and so I either add unnecessary checks or >> forget to add them, for which we then all pay when the missing >> check triggers an ICE. In patch reviews I see I'm not the only >> one who's not sure. The attribute should help avoid some of >> these problems: both visually and via warnings. >> >> Second, while testing the nonnull patch, I noticed that GCC would >> not optimize some null tests after calls to nonnull functions that >> take tree as an argument and that I expected it to optimize, like >> >> n = tree_to_shwi (TYPE_SIZE (type)); >> if (TYPE_SIZE (type)) >> ... >> >> The reason is that tree_to_shwi() isn't declared pure, even though >> tree_fits_shwi_p() is (though as I mentioned, the latter is null >> safe and so not declarted nonnull, and so it doesn't make the same >> optimization possible). >> >> Tested on x86_64-linux. The patch exposed a couple of issues >> in Ada. At least the first one is a false positive caused by >> GCC being unaware that tree_fits_uhwi_p() returns false for >> a null argument (propagating this knowledge via an attribute >> seems like an opportunity for a future enhancement). >> I suppressed the warning by introducing a local temporary. >> >> I suspect the second warning is caused by the Ada TYPE_RM_SIZE() >> macro expanding to a conditional with an explicit null operand: >> >> #define TYPE_RM_SIZE(NODE) TYPE_RM_VALUE ((NODE), 0) >> >> #define TYPE_RM_VALUE(NODE, N) \ >> (TYPE_RM_VALUES (NODE) \ >> ? TREE_VEC_ELT (TYPE_RM_VALUES (NODE), (N)) : NULL_TREE) >> >> but I wasn't able to reduce it to a small test case to confirm >> that. I suppressed this warning by introducing a temporary as >> well. >> >> Martin >> >> gcc-tree-nonnull.diff >> >> gcc/ChangeLog: >> >> * tree.h (tree_to_shwi): Add attribute nonnull. >> (tree_to_poly_int64, tree_to_uhwi, tree_to_poly_uint64): Same. >> (integer_zerop, integer_onep, int_fits_type_p): Same. >> >> gcc/ada/ChangeLog: >> >> * gcc-interface/utils.c (make_packable_type): Introduce a temporary >> to avoid -Wnonnull. >> (unchecked_convert): Same. > No doubt we have not been diligent about marking non-null, const, pure, > etc over time. I thought we had warnings for functions which are > const/pure but not suitably marked. Can you peek a bit at why those > didn't trigger. See ipa-pure-const.c. Ignore the initial comment -- it > applies to both functions and data. The -Wsuggest-attribute=const|pure warnings (or any others) are not enabled by default and GCC doesn't explicitly enable them. Maybe it should? FWIW, I tried add the options as a test in a bootstrap but no matter what make variables I set I couldn't figure out how to add them to the GCC command line, or find where it's documented. Do you have any idea how to do that? > ISTM we could probably build a missing non-null attribute warning. If a > NULL pointer argument unconditionally leads to an assert, then the > function should be marked. Similarly if a pointer argument is > unconditionally dereferenced, then it should be marked. I strongly > suspect this would be too noisy to enable by default. Yes, that would be useful but I'm sure you're right that it would also be noisy with most code. > ANyway, the patch is OK for the trunk. Committed in r264680. Martin
On 9/27/18 5:03 PM, Martin Sebor wrote: > On 09/27/2018 12:25 PM, Jeff Law wrote: >> On 9/26/18 5:54 PM, Martin Sebor wrote: >>> The attached patch adds attributes nonnull and pure to >>> tree_to_shwi() and a small number of other heavily used functions >>> that will benefit from both. >>> >>> First, I noticed that there are a bunch of functions in tree.c that >>> deal gracefully with null pointers right alongside a bunch of other >>> related functions that don't. >>> >>> For example, tree_fits_shwi_p() is null-safe but integer_zerop() >>> is not. There a number of others. I never remember which ones >>> are in which group and so I either add unnecessary checks or >>> forget to add them, for which we then all pay when the missing >>> check triggers an ICE. In patch reviews I see I'm not the only >>> one who's not sure. The attribute should help avoid some of >>> these problems: both visually and via warnings. >>> >>> Second, while testing the nonnull patch, I noticed that GCC would >>> not optimize some null tests after calls to nonnull functions that >>> take tree as an argument and that I expected it to optimize, like >>> >>> n = tree_to_shwi (TYPE_SIZE (type)); >>> if (TYPE_SIZE (type)) >>> ... >>> >>> The reason is that tree_to_shwi() isn't declared pure, even though >>> tree_fits_shwi_p() is (though as I mentioned, the latter is null >>> safe and so not declarted nonnull, and so it doesn't make the same >>> optimization possible). >>> >>> Tested on x86_64-linux. The patch exposed a couple of issues >>> in Ada. At least the first one is a false positive caused by >>> GCC being unaware that tree_fits_uhwi_p() returns false for >>> a null argument (propagating this knowledge via an attribute >>> seems like an opportunity for a future enhancement). >>> I suppressed the warning by introducing a local temporary. >>> >>> I suspect the second warning is caused by the Ada TYPE_RM_SIZE() >>> macro expanding to a conditional with an explicit null operand: >>> >>> #define TYPE_RM_SIZE(NODE) TYPE_RM_VALUE ((NODE), 0) >>> >>> #define TYPE_RM_VALUE(NODE, N) \ >>> (TYPE_RM_VALUES (NODE) \ >>> ? TREE_VEC_ELT (TYPE_RM_VALUES (NODE), (N)) : NULL_TREE) >>> >>> but I wasn't able to reduce it to a small test case to confirm >>> that. I suppressed this warning by introducing a temporary as >>> well. >>> >>> Martin >>> >>> gcc-tree-nonnull.diff >>> >>> gcc/ChangeLog: >>> >>> * tree.h (tree_to_shwi): Add attribute nonnull. >>> (tree_to_poly_int64, tree_to_uhwi, tree_to_poly_uint64): Same. >>> (integer_zerop, integer_onep, int_fits_type_p): Same. >>> >>> gcc/ada/ChangeLog: >>> >>> * gcc-interface/utils.c (make_packable_type): Introduce a temporary >>> to avoid -Wnonnull. >>> (unchecked_convert): Same. >> No doubt we have not been diligent about marking non-null, const, pure, >> etc over time. I thought we had warnings for functions which are >> const/pure but not suitably marked. Can you peek a bit at why those >> didn't trigger. See ipa-pure-const.c. Ignore the initial comment -- it >> applies to both functions and data. > > The -Wsuggest-attribute=const|pure warnings (or any others) are > not enabled by default and GCC doesn't explicitly enable them. > Maybe it should? Weird. I could have swore I saw missing-const/pure warnings during a bootstrap test a month or so ago. > > FWIW, I tried add the options as a test in a bootstrap but no > matter what make variables I set I couldn't figure out how to > add them to the GCC command line, or find where it's documented. > Do you have any idea how to do that? I'd try EXTRA_BOOTSTRAP_FLAGS. But I pass nonstandard args so rarely that I just hack it into the generated Makefiles as needed or hack it into the sources themselves. Jeff
gcc/ChangeLog: * tree.h (tree_to_shwi): Add attribute nonnull. (tree_to_poly_int64, tree_to_uhwi, tree_to_poly_uint64): Same. (integer_zerop, integer_onep, int_fits_type_p): Same. gcc/ada/ChangeLog: * gcc-interface/utils.c (make_packable_type): Introduce a temporary to avoid -Wnonnull. (unchecked_convert): Same. Index: gcc/ada/gcc-interface/utils.c =================================================================== --- gcc/ada/gcc-interface/utils.c (revision 264652) +++ gcc/ada/gcc-interface/utils.c (working copy) @@ -990,15 +990,16 @@ make_packable_type (tree type, bool in_record, uns } else { + tree type_size = TYPE_ADA_SIZE (type); /* Do not try to shrink the size if the RM size is not constant. */ if (TYPE_CONTAINS_TEMPLATE_P (type) - || !tree_fits_uhwi_p (TYPE_ADA_SIZE (type))) + || !tree_fits_uhwi_p (type_size)) return type; /* Round the RM size up to a unit boundary to get the minimal size for a BLKmode record. Give up if it's already the size and we don't need to lower the alignment. */ - new_size = tree_to_uhwi (TYPE_ADA_SIZE (type)); + new_size = tree_to_uhwi (type_size); new_size = (new_size + BITS_PER_UNIT - 1) & -BITS_PER_UNIT; if (new_size == size && (max_align == 0 || align <= max_align)) return type; @@ -5307,20 +5308,21 @@ unchecked_convert (tree type, tree expr, bool notr to its size, sign- or zero-extend the result. But we need not do this if the input is also an integral type and both are unsigned or both are signed and have the same precision. */ + tree type_rm_size; if (!notrunc_p && INTEGRAL_TYPE_P (type) && !(code == INTEGER_TYPE && TYPE_BIASED_REPRESENTATION_P (type)) - && TYPE_RM_SIZE (type) - && tree_int_cst_compare (TYPE_RM_SIZE (type), TYPE_SIZE (type)) < 0 + && (type_rm_size = TYPE_RM_SIZE (type)) + && tree_int_cst_compare (type_rm_size, TYPE_SIZE (type)) < 0 && !(INTEGRAL_TYPE_P (etype) && type_unsigned_for_rm (type) == type_unsigned_for_rm (etype) && (type_unsigned_for_rm (type) - || tree_int_cst_compare (TYPE_RM_SIZE (type), + || tree_int_cst_compare (type_rm_size, TYPE_RM_SIZE (etype) ? TYPE_RM_SIZE (etype) : TYPE_SIZE (etype)) == 0))) { - if (integer_zerop (TYPE_RM_SIZE (type))) + if (integer_zerop (type_rm_size)) expr = build_int_cst (type, 0); else { @@ -5330,7 +5332,7 @@ unchecked_convert (tree type, tree expr, bool notr tree shift_expr = convert (base_type, size_binop (MINUS_EXPR, - TYPE_SIZE (type), TYPE_RM_SIZE (type))); + TYPE_SIZE (type), type_rm_size)); expr = convert (type, build_binary_op (RSHIFT_EXPR, base_type, Index: gcc/tree.h =================================================================== --- gcc/tree.h (revision 264653) +++ gcc/tree.h (working copy) @@ -4231,16 +4231,23 @@ extern tree purpose_member (const_tree, tree); extern bool vec_member (const_tree, vec<tree, va_gc> *); extern tree chain_index (int, tree); +/* Arguments may be null. */ extern int tree_int_cst_equal (const_tree, const_tree); +/* The following predicates are safe to call with a null argument. */ extern bool tree_fits_shwi_p (const_tree) ATTRIBUTE_PURE; extern bool tree_fits_poly_int64_p (const_tree) ATTRIBUTE_PURE; extern bool tree_fits_uhwi_p (const_tree) ATTRIBUTE_PURE; extern bool tree_fits_poly_uint64_p (const_tree) ATTRIBUTE_PURE; -extern HOST_WIDE_INT tree_to_shwi (const_tree); -extern poly_int64 tree_to_poly_int64 (const_tree); -extern unsigned HOST_WIDE_INT tree_to_uhwi (const_tree); -extern poly_uint64 tree_to_poly_uint64 (const_tree); + +extern HOST_WIDE_INT tree_to_shwi (const_tree) + ATTRIBUTE_NONNULL (1) ATTRIBUTE_PURE; +extern poly_int64 tree_to_poly_int64 (const_tree) + ATTRIBUTE_NONNULL (1) ATTRIBUTE_PURE; +extern unsigned HOST_WIDE_INT tree_to_uhwi (const_tree) + ATTRIBUTE_NONNULL (1) ATTRIBUTE_PURE; +extern poly_uint64 tree_to_poly_uint64 (const_tree) + ATTRIBUTE_NONNULL (1) ATTRIBUTE_PURE; #if !defined ENABLE_TREE_CHECKING && (GCC_VERSION >= 4003) extern inline __attribute__ ((__gnu_inline__)) HOST_WIDE_INT tree_to_shwi (const_tree t) @@ -4893,7 +4900,8 @@ extern bool really_constant_p (const_tree); extern bool ptrdiff_tree_p (const_tree, poly_int64_pod *); extern bool decl_address_invariant_p (const_tree); extern bool decl_address_ip_invariant_p (const_tree); -extern bool int_fits_type_p (const_tree, const_tree); +extern bool int_fits_type_p (const_tree, const_tree) + ATTRIBUTE_NONNULL (1) ATTRIBUTE_NONNULL (2) ATTRIBUTE_PURE; #ifndef GENERATOR_FILE extern void get_type_static_bounds (const_tree, mpz_t, mpz_t); #endif
The attached patch adds attributes nonnull and pure to tree_to_shwi() and a small number of other heavily used functions that will benefit from both. First, I noticed that there are a bunch of functions in tree.c that deal gracefully with null pointers right alongside a bunch of other related functions that don't. For example, tree_fits_shwi_p() is null-safe but integer_zerop() is not. There a number of others. I never remember which ones are in which group and so I either add unnecessary checks or forget to add them, for which we then all pay when the missing check triggers an ICE. In patch reviews I see I'm not the only one who's not sure. The attribute should help avoid some of these problems: both visually and via warnings. Second, while testing the nonnull patch, I noticed that GCC would not optimize some null tests after calls to nonnull functions that take tree as an argument and that I expected it to optimize, like n = tree_to_shwi (TYPE_SIZE (type)); if (TYPE_SIZE (type)) ... The reason is that tree_to_shwi() isn't declared pure, even though tree_fits_shwi_p() is (though as I mentioned, the latter is null safe and so not declarted nonnull, and so it doesn't make the same optimization possible). Tested on x86_64-linux. The patch exposed a couple of issues in Ada. At least the first one is a false positive caused by GCC being unaware that tree_fits_uhwi_p() returns false for a null argument (propagating this knowledge via an attribute seems like an opportunity for a future enhancement). I suppressed the warning by introducing a local temporary. I suspect the second warning is caused by the Ada TYPE_RM_SIZE() macro expanding to a conditional with an explicit null operand: #define TYPE_RM_SIZE(NODE) TYPE_RM_VALUE ((NODE), 0) #define TYPE_RM_VALUE(NODE, N) \ (TYPE_RM_VALUES (NODE) \ ? TREE_VEC_ELT (TYPE_RM_VALUES (NODE), (N)) : NULL_TREE) but I wasn't able to reduce it to a small test case to confirm that. I suppressed this warning by introducing a temporary as well. Martin