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. Apart from
that,
we want to merge all flags to a single attribute. Thus said, having an unique
name
will enable this.
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
