On Fri, Jun 9, 2017 at 2:08 PM, Martin Liška <mli...@suse.cz> wrote:
> On 06/09/2017 01:05 PM, Richard Biener wrote:
>>
>> On Fri, Jun 9, 2017 at 12:49 PM, Martin Liška <mli...@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 <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.
>>>
>>>
>>>
>>> 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?
>
>
> Unfortunately, we currently support no_sanitize_{address,thread,undefined}
> and no_address_safety_analysis. Thus we can't drop these.
Sure, but the internal representation of no_sanitize_{address,thread,undefined}
can be the same as no_sanitize("..."). Just add *no_add_attrs = true and
append/change no_sanitize in the handler.
>
>>
>>> 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?
>
>
> Yep, it's unique but as I mentioned we've got const struct attribute_spec
> c_common_attribute_table[]
> handlers that are executed on these no sanitize attributes. And as I want to
> store int mask to
> an attribute, I prefer to come up with a new attribute "sanitize no_flags"
> and I can set
> *no_add_attrs = true; in order to remove the original attributes:
>
> void
> __attribute__((no_sanitize(("address"))))
> __attribute__((no_sanitize(("undefined"))))
> __attribute__((no_sanitize(("address"))))
> __attribute__((no_sanitize(("null"))))
> fn1 (void) { char *ptr; char *ptr2; { char my_char[9]; ptr = &my_char[0];
> __builtin_memcpy (&ptr2, &ptr, sizeof (ptr2)); } *(ptr2+9) = 'c'; }
>
> will become:
>
> __attribute__((sanitize no_flags (16777187)))
> fn1 ()
> {
> ...
> }
>
> I'm going to test that.
>
> Martin
>
>
>>
>> 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
>>>>>>>>
>>>>>>>
>>>>>
>>>
>
