[PATCH v2] Implement no_sanitize function attribute

[Martin Liška]

On 06/09/2017 12:39 PM, Richard Biener wrote:
> On Fri, Jun 9, 2017 at 12:17 PM, Martin Liška <mliska@suse.cz> wrote:
>> On 06/09/2017 12:12 PM, Richard Biener wrote:
>>>
>>> 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.
>>
>>
>> Can be seen here:
>>
>> __attribute__((no_sanitize_thread, no_sanitize ("null"), no_sanitize
>> ("address"), no_sanitize ("undefined"), no_sanitize ("address"), sanitize
>> no_flags (16777195)))
>> fn1 ()
>> {
>> ...
>> }
>>
>> where no_sanitize_thread and no_sanitize are normal attributes used by
>> users.
>> But we want to aggregate all there attributes and build one integer mask
>> that
>> will drive sanitize_flags_p. And that's why we introduced 'sanitize
>> no_flags'
>> attribute, so that we don't have to iterate all attrs every time in
>> anitize_flags_p.
>>
>> Hope it explains situation?
> 
> Hum, ok ... but then you can "simply" have the no_sanitize attribute
> internal rep use a INTEGER_CST instread of a STRING_CST value,
> updating that in handle_attribute instead of adding new attributes?
> 
> There's nothing that forces internal representation to match what the
> user wrote.

I see, but consider following test-case:

void
__attribute__((no_sanitize_thread))
__attribute__((no_sanitize(("address"))))
__attribute__((no_sanitize(("undefined"))))
__attribute__((no_sanitize(("address"))))
__attribute__((no_sanitize(("null"))))
foo (void) {}

handle_no_sanitize_thread_attribute function is called for no_sanitize_thread and
changing first no_sanitize attribute to integer is wrongly doable. Apart from that,
we want to merge all flags to a single attribute. Thus said, having an unique name
will enable this.

Martin

> 
> [historically we've used random flags in decls/types for this kind of caching
> but I can see we don't want to waste as many bits there]
> 
> Richard.
> 
>> Martin
>>
>>
>>>
>>> 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
>>>>>
>>>>
>>