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
