On Fri, Jun 9, 2017 at 12:17 PM, Martin Liška <mli...@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 <mli...@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
>>>>
>>>
>
