diff mbox

[7/7] Libsanitizer merge from upstream r285547.

Message ID 58203BD6.7060800@samsung.com
State New
Headers show

Commit Message

Maxim Ostapenko Nov. 7, 2016, 8:31 a.m. UTC 
This patch tries to implement odr indicators functionality at compiler 
side.
We emit new __odr_asan_XXX symbol for each instrumented global that 
indicates whether this global was already registered and the library 
checks this indicator symbol at runtime. For some globals (e.g. static 
or hidden) the odr indicator is not needed, thus we can skip the 
indicator for them and pass zero to runtime.
If this patch is undesirable at this stage, we can probably postpone it 
until GCC 8 though.

Comments

Jakub Jelinek Nov. 7, 2016, 8:52 a.m. UTC | #1 
On Mon, Nov 07, 2016 at 11:31:18AM +0300, Maxim Ostapenko wrote:
> --- a/gcc/asan.c

> +++ b/gcc/asan.c

> @@ -1329,6 +1329,16 @@ asan_needs_local_alias (tree decl)

>    return DECL_WEAK (decl) || !targetm.binds_local_p (decl);

>  }

>  

> +/* Return true if DECL, a global var, is an artificial ODR indicator symbol

> +   therefore doesn't need protection.  */

> +

> +static bool

> +is_odr_indicator (tree decl)

> +{

> +  const char *sym_name = IDENTIFIER_POINTER (DECL_NAME (decl));

> +  return strstr(sym_name, "_.__odr_asan_") == sym_name;


Formatting, missing space before (.
Plus strstr (x, y) == x is very inefficient, strncmp would be cheaper.
But more importantly, you are relying on what exactly does
ASM_GENERATE_INTERNAL_LABEL, that differs between targets, not all of them
e.g. allow . in symbol names, other targets use $, others can only use _,
etc.  I think you'd better just add "asan odr indicator" attribute
(including the spaces, so it isn't something users can add to their
variables) to the artificial vars and lookup_attribute in the
is_odr_indicator predicate (after testing some cheap flags like
DECL_ARTIFICIAL).

> +  tree var = build_decl (UNKNOWN_LOCATION, VAR_DECL, get_identifier (sym_name),

> +			 char_type_node);

> +  TREE_ADDRESSABLE (var) = TREE_ADDRESSABLE (decl);


How is addressability of the original decl related to addressability of the
indicator?  If you take address of the indicator (it is stored in the
structure), it should be just 1.

> +  TREE_READONLY (var) = 0;

> +  TREE_THIS_VOLATILE (var) = 1;

> +  DECL_GIMPLE_REG_P (var) = DECL_GIMPLE_REG_P (decl);


Again, how is this related?  Just store 0.

> +  DECL_ARTIFICIAL (var) = 1;

> +  DECL_IGNORED_P (var) = DECL_IGNORED_P (decl);


The indicators should be surely not recorded in debug info, so DEC_IGNORED_P
should be 1.

> +  TREE_STATIC (var) = 1;

> +  TREE_PUBLIC (var) = 1;

> +  DECL_VISIBILITY (var) = DECL_VISIBILITY (decl);


Are they meant to have the same visibility and be exported from DSOs if the
original var is?

> @@ -2313,8 +2374,7 @@ asan_add_global (tree decl, tree type, vec<constructor_elt, va_gc> *v)

>    else

>      locptr = build_int_cst (uptr, 0);

>    CONSTRUCTOR_APPEND_ELT (vinner, NULL_TREE, locptr);

> -  /* TODO: support ODR indicators.  */

> -  CONSTRUCTOR_APPEND_ELT(vinner, NULL_TREE, build_int_cst (uptr, 0));

> +  CONSTRUCTOR_APPEND_ELT(vinner, NULL_TREE, odr_indicator_ptr);


Formatting, missing space before (, both in this patch and in the previous
one.

	Jakub
diff mbox

Patch

From 137f139972a89259b9d8521e13ecb76fd2cef433 Mon Sep 17 00:00:00 2001
From: Maxim Ostapenko <m.ostapenko@samsung.com>
Date: Fri, 28 Oct 2016 10:22:03 +0300
Subject: [PATCH 7/7] Add support for ASan odr_indicator.

config/

	* bootstrap-asan.mk: Replace LSAN_OPTIONS=detect_leaks=0 with
	ASAN_OPTIONS=detect_leaks=0:use_odr_indicator=1.

gcc/

	* asan.c (asan_global_struct): Refactor.
	(create_odr_indicator): New function.
	(asan_needs_odr_indicator_p): Likewise.
	(is_odr_indicator): Likewise.
	(asan_add_global): Introduce odr_indicator_ptr. Pass it into global's
	constructor.
	(asan_protect_global): Do not protect odr indicators.

gcc/testsuite/

	* c-c++-common/asan/no-redundant-odr-indicators-1.c: New test.
---
 config/ChangeLog                                   |  5 ++
 config/bootstrap-asan.mk                           |  2 +-
 gcc/ChangeLog                                      | 10 +++
 gcc/asan.c                                         | 76 +++++++++++++++++++---
 gcc/testsuite/ChangeLog                            |  4 ++
 .../asan/no-redundant-odr-indicators-1.c           | 17 +++++
 6 files changed, 105 insertions(+), 9 deletions(-)
 create mode 100644 gcc/testsuite/c-c++-common/asan/no-redundant-odr-indicators-1.c

diff --git a/config/ChangeLog b/config/ChangeLog
index 3b0092b..0c75185 100644
--- a/config/ChangeLog
+++ b/config/ChangeLog
@@ -1,3 +1,8 @@ 
+2016-11-07  Maxim Ostapenko  <m.ostapenko@samsung.com>
+
+	* bootstrap-asan.mk: Replace LSAN_OPTIONS=detect_leaks=0 with
+	ASAN_OPTIONS=detect_leaks=0:use_odr_indicator=1.
+
 2016-06-21  Trevor Saunders  <tbsaunde+gcc@tbsaunde.org>
 
 	* elf.m4: Remove interix support.
diff --git a/config/bootstrap-asan.mk b/config/bootstrap-asan.mk
index 70baaf9..e73d4c2 100644
--- a/config/bootstrap-asan.mk
+++ b/config/bootstrap-asan.mk
@@ -1,7 +1,7 @@ 
 # This option enables -fsanitize=address for stage2 and stage3.
 
 # Suppress LeakSanitizer in bootstrap.
-export LSAN_OPTIONS="detect_leaks=0"
+export ASAN_OPTIONS=detect_leaks=0:use_odr_indicator=1
 
 STAGE2_CFLAGS += -fsanitize=address
 STAGE3_CFLAGS += -fsanitize=address
diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 1da0ef9..527cafa 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,5 +1,15 @@ 
 2016-11-07  Maxim Ostapenko  <m.ostapenko@samsung.com>
 
+	* asan.c (asan_global_struct): Refactor.
+	(create_odr_indicator): New function.
+	(asan_needs_odr_indicator_p): Likewise.
+	(is_odr_indicator): Likewise.
+	(asan_add_global): Introduce odr_indicator_ptr. Pass it into global's
+	constructor.
+	(asan_protect_global): Do not protect odr indicators.
+
+2016-11-07  Maxim Ostapenko  <m.ostapenko@samsung.com>
+
 	* asan.h (asan_intercepted_p): Handle BUILT_IN_STRCSPN,
 	BUILT_IN_STRPBRK, BUILT_IN_STRSPN and BUILT_IN_STRSTR.
 
diff --git a/gcc/asan.c b/gcc/asan.c
index fdc84bd..b54110a 100644
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -1329,6 +1329,16 @@  asan_needs_local_alias (tree decl)
   return DECL_WEAK (decl) || !targetm.binds_local_p (decl);
 }
 
+/* Return true if DECL, a global var, is an artificial ODR indicator symbol
+   therefore doesn't need protection.  */
+
+static bool
+is_odr_indicator (tree decl)
+{
+  const char *sym_name = IDENTIFIER_POINTER (DECL_NAME (decl));
+  return strstr(sym_name, "_.__odr_asan_") == sym_name;
+}
+
 /* Return true if DECL is a VAR_DECL that should be protected
    by Address Sanitizer, by appending a red zone with protected
    shadow memory after it and aligning it to at least
@@ -1377,7 +1387,8 @@  asan_protect_global (tree decl)
       || ASAN_RED_ZONE_SIZE * BITS_PER_UNIT > MAX_OFILE_ALIGNMENT
       || !valid_constant_size_p (DECL_SIZE_UNIT (decl))
       || DECL_ALIGN_UNIT (decl) > 2 * ASAN_RED_ZONE_SIZE
-      || TREE_TYPE (decl) == ubsan_get_source_location_type ())
+      || TREE_TYPE (decl) == ubsan_get_source_location_type ()
+      || is_odr_indicator (decl))
     return false;
 
   rtl = DECL_RTL (decl);
@@ -2197,14 +2208,15 @@  asan_dynamic_init_call (bool after_p)
 static tree
 asan_global_struct (void)
 {
-  static const char *field_names[8]
+  static const char *field_names[]
     = { "__beg", "__size", "__size_with_redzone",
-	"__name", "__module_name", "__has_dynamic_init", "__location", "__odr_indicator"};
-  tree fields[8], ret;
-  int i;
+	"__name", "__module_name", "__has_dynamic_init", "__location",
+	"__odr_indicator"};
+  tree fields[ARRAY_SIZE (field_names)], ret;
+  unsigned i;
 
   ret = make_node (RECORD_TYPE);
-  for (i = 0; i < 8; i++)
+  for (i = 0; i < ARRAY_SIZE (field_names); i++)
     {
       fields[i]
 	= build_decl (UNKNOWN_LOCATION, FIELD_DECL,
@@ -2226,6 +2238,52 @@  asan_global_struct (void)
   return ret;
 }
 
+/* Create and return odr indicator symbol for DECL.
+   TYPE is __asan_global struct type as returned by asan_global_struct.  */
+
+static tree
+create_odr_indicator (tree decl, tree type)
+{
+  char sym_name[100], tmp_name[100];
+  static int lasan_odr_ind_cnt = 0;
+  tree uptr = TREE_TYPE (DECL_CHAIN (TYPE_FIELDS (type)));
+
+  snprintf (tmp_name, sizeof (tmp_name), "__odr_asan_%s_",
+	    IDENTIFIER_POINTER (DECL_NAME (decl)));
+  ASM_GENERATE_INTERNAL_LABEL (sym_name, tmp_name, ++lasan_odr_ind_cnt);
+  char *asterisk = sym_name;
+  while ((asterisk = strchr (asterisk, '*')))
+    *asterisk = '_';
+  tree var = build_decl (UNKNOWN_LOCATION, VAR_DECL, get_identifier (sym_name),
+			 char_type_node);
+  TREE_ADDRESSABLE (var) = TREE_ADDRESSABLE (decl);
+  TREE_READONLY (var) = 0;
+  TREE_THIS_VOLATILE (var) = 1;
+  DECL_GIMPLE_REG_P (var) = DECL_GIMPLE_REG_P (decl);
+  DECL_ARTIFICIAL (var) = 1;
+  DECL_IGNORED_P (var) = DECL_IGNORED_P (decl);
+  TREE_STATIC (var) = 1;
+  TREE_PUBLIC (var) = 1;
+  DECL_VISIBILITY (var) = DECL_VISIBILITY (decl);
+  TREE_USED (var) = 1;
+  tree ctor = build_constructor_va (TREE_TYPE (var), 1, NULL_TREE,
+				    build_int_cst (unsigned_type_node, 0));
+  TREE_CONSTANT (ctor) = 1;
+  TREE_STATIC (ctor) = 1;
+  DECL_INITIAL (var) = ctor;
+  varpool_node::finalize_decl (var);
+  return fold_convert (uptr, build_fold_addr_expr (var));
+}
+
+/* Return true if DECL, a global var, might be overridden and needs
+   an additional odr indicator symbol.  */
+
+static bool
+asan_needs_odr_indicator_p (tree decl)
+{
+  return !DECL_ARTIFICIAL (decl) && !DECL_WEAK (decl) && TREE_PUBLIC (decl);
+}
+
 /* Append description of a single global DECL into vector V.
    TYPE is __asan_global struct type as returned by asan_global_struct.  */
 
@@ -2266,6 +2324,9 @@  asan_add_global (tree decl, tree type, vec<constructor_elt, va_gc> *v)
       assemble_alias (refdecl, DECL_ASSEMBLER_NAME (decl));
     }
 
+  tree odr_indicator_ptr = asan_needs_odr_indicator_p (decl)
+			     ? create_odr_indicator (decl, type)
+			     : build_int_cst (uptr, 0);
   CONSTRUCTOR_APPEND_ELT (vinner, NULL_TREE,
 			  fold_convert (const_ptr_type_node,
 					build_fold_addr_expr (refdecl)));
@@ -2313,8 +2374,7 @@  asan_add_global (tree decl, tree type, vec<constructor_elt, va_gc> *v)
   else
     locptr = build_int_cst (uptr, 0);
   CONSTRUCTOR_APPEND_ELT (vinner, NULL_TREE, locptr);
-  /* TODO: support ODR indicators.  */
-  CONSTRUCTOR_APPEND_ELT(vinner, NULL_TREE, build_int_cst (uptr, 0));
+  CONSTRUCTOR_APPEND_ELT(vinner, NULL_TREE, odr_indicator_ptr);
   init = build_constructor (type, vinner);
   CONSTRUCTOR_APPEND_ELT (v, NULL_TREE, init);
 }
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index afa77a8..7d11a7c 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,5 +1,9 @@ 
 2016-11-07  Maxim Ostapenko  <m.ostapenko@samsung.com>
 
+	* c-c++-common/asan/no-redundant-odr-indicators-1.c: New test.
+
+2016-11-07  Maxim Ostapenko  <m.ostapenko@samsung.com>
+
 	* c-c++-common/asan/default_options.h: New file.
 	* c-c++-common/asan/strcasestr-1.c: New test.
 	* c-c++-common/asan/strcasestr-2.c: Likewise.
diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-odr-indicators-1.c b/gcc/testsuite/c-c++-common/asan/no-redundant-odr-indicators-1.c
new file mode 100644
index 0000000..ff1f272
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/asan/no-redundant-odr-indicators-1.c
@@ -0,0 +1,17 @@ 
+/* { dg-do compile } */
+/* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */
+
+/* Local variables should not have odr indicators.  */
+static int a = 2;
+/* Thread local variables should not have odr indicators.  */
+__thread int b = 3;
+/* Externally visible  variables should have odr indicators.  */
+int c = 1;
+
+int main () {
+    return 0;
+}
+
+/* { dg-final { scan-assembler-not ".*odr_asan_a.*" } } */
+/* { dg-final { scan-assembler-not ".*odr_asan_b.*" } } */
+/* { dg-final { scan-assembler ".*odr_asan_c.*" } } */
-- 
1.9.1