Re: [PATCH v2] Implement no_sanitize function attribute

[Richard Biener <richard dot guenther at gmail dot com>]

On Fri, Jun 9, 2017 at 11:29 AM, Martin Liška <mliska@suse.cz> wrote:
> On 06/08/2017 03:47 PM, Jakub Jelinek wrote:
>>
>> Hi!
>>
>> I'd still prefer to handle it with the flags infrastructure instead, but
>> if
>> Richard wants to do it this way, then at least:
>>
>> On Thu, Jun 08, 2017 at 03:30:49PM +0200, Martin Liška wrote:
>>>
>>> +/* Return true when flag_sanitize & FLAG is non-zero.  If FN is
>>> non-null,
>>> +   remove all flags mentioned in "no_sanitize_flags" of DECL_ATTRIBUTES.
>>> */
>>> +
>>> +bool
>>> +sanitize_flags_p (unsigned int flag, const_tree fn)
>>> +{
>>> +  unsigned int result_flags = flag_sanitize & flag;
>>
>>
>> This function really should be either inline, or partly inline, partly out
>> of line, to handle the common case (sanitization of something not enabled)
>> in the fast path.
>
>
> Hello.
>
> Having that inlined would be great, however we'll need to put it to tree.h
> and thus we have to include "options.h" before tree.h in multiple source
> files.
> Please take a look at partial patch.
>
>>
>> And, it should have an early out,
>>    if (result_flags == 0)
>>      return false;
>
>
> Good idea!
>
>>
>>> +
>>> +  if (fn != NULL_TREE)
>>> +    {
>>> +      tree value = lookup_attribute ("no_sanitize_flags",
>>> DECL_ATTRIBUTES (fn));
>>
>>
>> The attribute, if it is internal only, should have spaces or similar
>> characters in its name, like "fn spec", "omp declare target" and many
>> others.
>
>
> Done that.

Whoo, wait -- this is for internal use only?  Can you step back and explain
why we need this?  We do, after all, have -fsanitize options already.

Richard.

>>
>> +add_no_sanitize_value (tree node, unsigned int flags)
>> +{
>> +  tree attr = lookup_attribute ("no_sanitize_flags", DECL_ATTRIBUTES
>> (node));
>> +  if (attr)
>> +    {
>> +      unsigned int old_value = tree_to_uhwi (TREE_VALUE (attr));
>> +      flags |= old_value;
>> +    }
>> +
>> +  DECL_ATTRIBUTES (node)
>> +    = tree_cons (get_identifier ("no_sanitize_flags"),
>> +                build_int_cst (unsigned_type_node, flags),
>> +                DECL_ATTRIBUTES (node));
>>
>> If there is a previous attribute already, can't you modify it in
>> place?  If not, as it could be perhaps shared? with other functions
>> somehow, at least you should avoid adding a new attribute if
>> (old_value | flags) == old_value.
>
>
> Yep, we should definitely share, I'll add test-case for that.
> I'm currently testing the incremental patch, may I install it
> after regression tests?
>
> Martin
>
>>
>>         Jakub
>>
>