This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH v2] Implement no_sanitize function attribute
On Fri, Jun 9, 2017 at 12:49 PM, Martin Liška <mliska@suse.cz> wrote:
> 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.
Just change no_sanitize_thread to add no_sanitize instead?
> Apart
> from that,
> we want to merge all flags to a single attribute. Thus said, having an
> unique name
> will enable this.
no_sanitize looks like the unique name to me -- I suppose no_sanitize("thread")
works?
Richard.
> 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
>>>>>>
>>>>>
>>>
>