Re: [PATCH v2] Implement no_sanitize function attribute

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

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.

[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
>>>>
>>>
>