On Fri, Jun 9, 2017 at 2:51 PM, Martin Liška <mli...@suse.cz> wrote:
> On 06/09/2017 02:27 PM, Richard Biener wrote:
>>
>> 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.
>
>
> Yep, that would be possible. That will mean to tranform all no_sanitize_* to
> no_sanitize("...") attributes and then merge all these attributes with a
> string
> value to a single one with integer mask. Well, doable, but I still prefer to
> merge directly all to the new attribute. Plase take a look at incremental
> patch.
You can directly transform to no_sanitize with integer mask, not sure why
you'd need an intermediate step with a string?
> Martin
>
>
>>
>>>
>>>>
>>>>> 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
>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>
>>>
>
