Re: [PATCH] declare tree_to_shwi et al. nonnull and pure

[Jeff Law <law at redhat dot com>]

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