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
