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