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.
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
>From 22f08924dc3fdbe31dfb6fa83717a2a694604ef8 Mon Sep 17 00:00:00 2001
From: marxin <mli...@suse.cz>
Date: Thu, 8 Jun 2017 16:39:33 +0200
Subject: [PATCH] Put sanitize_flags_t to asan.h and merge all no_sanitize_*
attributes.
---
gcc/asan.h | 20 +++++++++++++
gcc/c-family/c-attribs.c | 43 +++++++++++++--------------
gcc/c/c-convert.c | 1 +
gcc/c/c-decl.c | 1 +
gcc/c/c-typeck.c | 1 +
gcc/convert.c | 1 +
gcc/cp/class.c | 1 +
gcc/cp/cp-gimplify.c | 1 +
gcc/cp/cp-ubsan.c | 1 +
gcc/cp/decl.c | 1 +
gcc/cp/init.c | 1 +
gcc/cp/typeck.c | 1 +
gcc/gimple-fold.c | 1 +
gcc/ipa-inline.c | 1 +
gcc/testsuite/gcc.dg/asan/use-after-scope-4.c | 3 ++
gcc/tree.c | 18 -----------
gcc/tree.h | 4 ---
17 files changed, 55 insertions(+), 45 deletions(-)
diff --git a/gcc/asan.h b/gcc/asan.h
index a590d0a5ace..f4ab5b6acec 100644
--- a/gcc/asan.h
+++ b/gcc/asan.h
@@ -154,4 +154,24 @@ asan_protect_stack_decl (tree decl)
|| (asan_sanitize_use_after_scope () && TREE_ADDRESSABLE (decl)));
}
+/* Return true when flag_sanitize & FLAG is non-zero. If FN is non-null,
+ remove all flags mentioned in "sanitize no_flags" of DECL_ATTRIBUTES. */
+
+static inline bool
+sanitize_flags_p (unsigned int flag, const_tree fn = current_function_decl)
+{
+ unsigned int result_flags = flag_sanitize & flag;
+ if (result_flags == 0)
+ return false;
+
+ if (fn != NULL_TREE)
+ {
+ tree value = lookup_attribute ("sanitize no_flags", DECL_ATTRIBUTES (fn));
+ if (value)
+ result_flags &= ~tree_to_uhwi (TREE_VALUE (value));
+ }
+
+ return result_flags;
+}
+
#endif /* TREE_ASAN */
diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
index abb43d0d02c..a0b554bae59 100644
--- a/gcc/c-family/c-attribs.c
+++ b/gcc/c-family/c-attribs.c
@@ -558,17 +558,22 @@ handle_cold_attribute (tree *node, tree name, tree ARG_UNUSED (args),
void
add_no_sanitize_value (tree node, unsigned int flags)
{
- tree attr = lookup_attribute ("no_sanitize_flags", DECL_ATTRIBUTES (node));
+ tree attr = lookup_attribute ("sanitize no_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 (flags == old_value)
+ return;
+
+ TREE_VALUE (attr) = build_int_cst (unsigned_type_node, flags);
+ }
+ else
+ DECL_ATTRIBUTES (node)
+ = tree_cons (get_identifier ("sanitize no_flags"),
+ build_int_cst (unsigned_type_node, flags),
+ DECL_ATTRIBUTES (node));
}
/* Handle a "no_sanitize" attribute; arguments as in
@@ -578,11 +583,11 @@ static tree
handle_no_sanitize_attribute (tree *node, tree name, tree args, int,
bool *no_add_attrs)
{
+ *no_add_attrs = true;
tree id = TREE_VALUE (args);
if (TREE_CODE (*node) != FUNCTION_DECL)
{
warning (OPT_Wattributes, "%qE attribute ignored", name);
- *no_add_attrs = true;
return NULL_TREE;
}
@@ -614,11 +619,9 @@ static tree
handle_no_sanitize_address_attribute (tree *node, tree name, tree, int,
bool *no_add_attrs)
{
+ *no_add_attrs = true;
if (TREE_CODE (*node) != FUNCTION_DECL)
- {
- warning (OPT_Wattributes, "%qE attribute ignored", name);
- *no_add_attrs = true;
- }
+ warning (OPT_Wattributes, "%qE attribute ignored", name);
else
add_no_sanitize_value (*node, SANITIZE_ADDRESS);
@@ -632,11 +635,9 @@ static tree
handle_no_sanitize_thread_attribute (tree *node, tree name, tree, int,
bool *no_add_attrs)
{
+ *no_add_attrs = true;
if (TREE_CODE (*node) != FUNCTION_DECL)
- {
- warning (OPT_Wattributes, "%qE attribute ignored", name);
- *no_add_attrs = true;
- }
+ warning (OPT_Wattributes, "%qE attribute ignored", name);
else
add_no_sanitize_value (*node, SANITIZE_THREAD);
@@ -651,11 +652,9 @@ static tree
handle_no_address_safety_analysis_attribute (tree *node, tree name, tree, int,
bool *no_add_attrs)
{
+ *no_add_attrs = true;
if (TREE_CODE (*node) != FUNCTION_DECL)
- {
- warning (OPT_Wattributes, "%qE attribute ignored", name);
- *no_add_attrs = true;
- }
+ warning (OPT_Wattributes, "%qE attribute ignored", name);
else
add_no_sanitize_value (*node, SANITIZE_ADDRESS);
@@ -669,11 +668,9 @@ static tree
handle_no_sanitize_undefined_attribute (tree *node, tree name, tree, int,
bool *no_add_attrs)
{
+ *no_add_attrs = true;
if (TREE_CODE (*node) != FUNCTION_DECL)
- {
- warning (OPT_Wattributes, "%qE attribute ignored", name);
- *no_add_attrs = true;
- }
+ warning (OPT_Wattributes, "%qE attribute ignored", name);
else
add_no_sanitize_value (*node,
SANITIZE_UNDEFINED | SANITIZE_UNDEFINED_NONDEFAULT);
diff --git a/gcc/c/c-convert.c b/gcc/c/c-convert.c
index 65852ec71b4..33c9143e354 100644
--- a/gcc/c/c-convert.c
+++ b/gcc/c/c-convert.c
@@ -31,6 +31,7 @@ along with GCC; see the file COPYING3. If not see
#include "convert.h"
#include "langhooks.h"
#include "ubsan.h"
+#include "asan.h"
/* Change of width--truncation and extension of integers or reals--
is represented with NOP_EXPR. Proper functioning of many things
diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c
index 82ad178d442..80598b5b614 100644
--- a/gcc/c/c-decl.c
+++ b/gcc/c/c-decl.c
@@ -53,6 +53,7 @@ along with GCC; see the file COPYING3. If not see
#include "builtins.h"
#include "spellcheck-tree.h"
#include "gcc-rich-location.h"
+#include "asan.h"
/* In grokdeclarator, distinguish syntactic contexts of declarators. */
enum decl_context
diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
index b9ffd09c045..7f9ca58f9e3 100644
--- a/gcc/c/c-typeck.c
+++ b/gcc/c/c-typeck.c
@@ -50,6 +50,7 @@ along with GCC; see the file COPYING3. If not see
#include "gomp-constants.h"
#include "spellcheck-tree.h"
#include "gcc-rich-location.h"
+#include "asan.h"
/* Possible cases of implicit bad conversions. Used to select
diagnostic messages in convert_for_assignment. */
diff --git a/gcc/convert.c b/gcc/convert.c
index e023888091d..429f988cbde 100644
--- a/gcc/convert.c
+++ b/gcc/convert.c
@@ -33,6 +33,7 @@ along with GCC; see the file COPYING3. If not see
#include "langhooks.h"
#include "builtins.h"
#include "ubsan.h"
+#include "asan.h"
#define maybe_fold_build1_loc(FOLD_P, LOC, CODE, TYPE, EXPR) \
((FOLD_P) ? fold_build1_loc (LOC, CODE, TYPE, EXPR) \
diff --git a/gcc/cp/class.c b/gcc/cp/class.c
index e136deed457..a84b8aa0ea6 100644
--- a/gcc/cp/class.c
+++ b/gcc/cp/class.c
@@ -36,6 +36,7 @@ along with GCC; see the file COPYING3. If not see
#include "dumpfile.h"
#include "gimplify.h"
#include "intl.h"
+#include "asan.h"
/* Id for dumping the class hierarchy. */
int class_dump_id;
diff --git a/gcc/cp/cp-gimplify.c b/gcc/cp/cp-gimplify.c
index 0f13ff69efe..fc2c69455c0 100644
--- a/gcc/cp/cp-gimplify.c
+++ b/gcc/cp/cp-gimplify.c
@@ -33,6 +33,7 @@ along with GCC; see the file COPYING3. If not see
#include "c-family/c-ubsan.h"
#include "cilk.h"
#include "cp-cilkplus.h"
+#include "asan.h"
/* Forward declarations. */
diff --git a/gcc/cp/cp-ubsan.c b/gcc/cp/cp-ubsan.c
index 95817dfc1f7..f00f870bd3e 100644
--- a/gcc/cp/cp-ubsan.c
+++ b/gcc/cp/cp-ubsan.c
@@ -23,6 +23,7 @@ along with GCC; see the file COPYING3. If not see
#include "coretypes.h"
#include "cp-tree.h"
#include "ubsan.h"
+#include "asan.h"
/* Test if we should instrument vptr access. */
diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index 27dec0f0981..a02cea3c602 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -51,6 +51,7 @@ along with GCC; see the file COPYING3. If not see
#include "cilk.h"
#include "builtins.h"
#include "gimplify.h"
+#include "asan.h"
/* Possible cases of bad specifiers type used by bad_specifiers. */
enum bad_spec_place {
diff --git a/gcc/cp/init.c b/gcc/cp/init.c
index a742cb83bbc..90abd23a267 100644
--- a/gcc/cp/init.c
+++ b/gcc/cp/init.c
@@ -30,6 +30,7 @@ along with GCC; see the file COPYING3. If not see
#include "gimplify.h"
#include "c-family/c-ubsan.h"
#include "intl.h"
+#include "asan.h"
static bool begin_init_stmts (tree *, tree *);
static tree finish_init_stmts (bool, tree, tree);
diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c
index 925aab4d410..b3a554d9cd0 100644
--- a/gcc/cp/typeck.c
+++ b/gcc/cp/typeck.c
@@ -37,6 +37,7 @@ along with GCC; see the file COPYING3. If not see
#include "c-family/c-ubsan.h"
#include "params.h"
#include "gcc-rich-location.h"
+#include "asan.h"
static tree cp_build_addr_expr_strict (tree, tsubst_flags_t);
static tree cp_build_function_call (tree, tree, tsubst_flags_t);
diff --git a/gcc/gimple-fold.c b/gcc/gimple-fold.c
index 5579115108f..0f8e326a0e8 100644
--- a/gcc/gimple-fold.c
+++ b/gcc/gimple-fold.c
@@ -56,6 +56,7 @@ along with GCC; see the file COPYING3. If not see
#include "ipa-chkp.h"
#include "tree-cfg.h"
#include "fold-const-call.h"
+#include "asan.h"
/* Return true when DECL can be referenced from current unit.
FROM_DECL (if non-null) specify constructor of variable DECL was taken from.
diff --git a/gcc/ipa-inline.c b/gcc/ipa-inline.c
index 6bd9af0ada6..8fb6f505eb7 100644
--- a/gcc/ipa-inline.c
+++ b/gcc/ipa-inline.c
@@ -117,6 +117,7 @@ along with GCC; see the file COPYING3. If not see
#include "auto-profile.h"
#include "builtins.h"
#include "fibonacci_heap.h"
+#include "asan.h"
typedef fibonacci_heap <sreal, cgraph_edge> edge_heap_t;
typedef fibonacci_node <sreal, cgraph_edge> edge_heap_node_t;
diff --git a/gcc/testsuite/gcc.dg/asan/use-after-scope-4.c b/gcc/testsuite/gcc.dg/asan/use-after-scope-4.c
index 781d70d6038..44dc79535d2 100644
--- a/gcc/testsuite/gcc.dg/asan/use-after-scope-4.c
+++ b/gcc/testsuite/gcc.dg/asan/use-after-scope-4.c
@@ -16,6 +16,9 @@ NAME (void) \
void
__attribute__((no_sanitize(("address"))))
+__attribute__((no_sanitize(("undefined"))))
+__attribute__((no_sanitize(("address"))))
+__attribute__((no_sanitize(("null"))))
FN (fn1)
void
diff --git a/gcc/tree.c b/gcc/tree.c
index 8979819adf7..a58f9aaa69e 100644
--- a/gcc/tree.c
+++ b/gcc/tree.c
@@ -14442,24 +14442,6 @@ nonnull_arg_p (const_tree arg)
return false;
}
-/* 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;
-
- if (fn != NULL_TREE)
- {
- tree value = lookup_attribute ("no_sanitize_flags", DECL_ATTRIBUTES (fn));
- if (value)
- result_flags &= ~tree_to_uhwi (TREE_VALUE (value));
- }
-
- return result_flags;
-}
-
/* Combine LOC and BLOCK to a combined adhoc loc, retaining any range
information. */
diff --git a/gcc/tree.h b/gcc/tree.h
index 22b9ec3f0e7..c6e883c489f 100644
--- a/gcc/tree.h
+++ b/gcc/tree.h
@@ -4248,10 +4248,6 @@ extern tree merge_dllimport_decl_attributes (tree, tree);
/* Handle a "dllimport" or "dllexport" attribute. */
extern tree handle_dll_attribute (tree *, tree, tree, int, bool *);
-
-extern bool sanitize_flags_p (unsigned int flag,
- const_tree fn = current_function_decl);
-
/* Returns true iff CAND and BASE have equivalent language-specific
qualifiers. */
--
2.13.0
