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
