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