Re: [PATCH v3] Support ASan ODR indicators at compiler side.

[Maxim Ostapenko]

Jakub, thank you for review. I'll commit following patch if no issues 
occur after regtesting and bootstrapping.


On 01/12/16 13:42, Jakub Jelinek wrote:

On Thu, Dec 01, 2016 at 01:25:43PM +0300, Maxim Ostapenko wrote:

+  int len = strlen (IDENTIFIER_POINTER (decl_name))
+   + sizeof ("__odr_asan_") + 1;

Please use size_t len instead of int len.  Why the + 1?  sizeof ("__odr_asan_")
should be already strlen ("__odr_asan_") + 1.


+  name = XALLOCAVEC (char, len);
+  name[len] = '\0';

This is buffer overflow.  Why do you need it at all?


+  snprintf (name, len, "__odr_asan_%s", IDENTIFIER_POINTER (decl_name));

This should zero terminate the string.

Also, shouldn't this be followed by:
#ifndef NO_DOT_IN_LABEL
   name[sizeof ("__odr_asan") - 1] = '.';
#elif !defined(NO_DOLLAR_IN_LABEL)
   name[sizeof ("__odr_asan") - 1] = '$';
#endif

to make it not possible to clash with user symbols __odr_asan_foobar etc.
if possible (on targets which don't allow dots nor dollars in labels that is
not really possible, but at least elsewhere).

That said, if the __odr_asan* symbols are exported, it is part of ABI, so
what exactly does LLVM use in those cases?


AFAIR LLVM doesn't have such a protection. But I agree that preventing 
symbols clashing is reasonable here. So it should be added to LLVM though.


-Maxim




+/* { dg-final { scan-assembler-not ".*odr_asan_a.*" } } */
+/* { dg-final { scan-assembler-not ".*odr_asan_b.*" } } */
+/* { dg-final { scan-assembler ".*odr_asan_c.*" } } */

The .* on either side makes no sense, please remove those.
And, if the dot or dollar is replacing _, you need to use "odr_asan.a"
etc. in the regexps.

Otherwise LGTM.

Jakub





diff --git a/config/ChangeLog b/config/ChangeLog
index ed59787..a5d5ff5 100644
--- a/config/ChangeLog
+++ b/config/ChangeLog
@@ -1,3 +1,8 @@
+2016-12-01  Maxim Ostapenko  
+
+	* bootstrap-asan.mk: Replace LSAN_OPTIONS=detect_leaks=0 with
+	ASAN_OPTIONS=detect_leaks=0:use_odr_indicator=1.
+
 2016-11-30  Matthias Klose  
 
 	* pkg.m4: New file.
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 b3cc6305..f19ac9d 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,13 @@
+2016-12-01  Maxim Ostapenko  
+
+	* 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-12-01  Jakub Jelinek  
 
 	PR target/78614
diff --git a/gcc/asan.c b/gcc/asan.c
index cb5d615..5af9547 100644
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -1388,6 +1388,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)
+{
+  return (DECL_ARTIFICIAL (decl)
+	  && lookup_attribute ("asan odr indicator", DECL_ATTRIBUTES (decl)));
+}
+
 /* 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
@@ -1436,7 +1446,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);
@@ -2266,14 +2277,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,
@@ -2295,6 +2307,63 @@ asan_global_struct (void)
   return ret;
 }
 
+/* Create and 

Re: [PATCH v3] Support ASan ODR indicators at compiler side.

[Jakub Jelinek]

On Thu, Dec 01, 2016 at 01:25:43PM +0300, Maxim Ostapenko wrote:
> +  int len = strlen (IDENTIFIER_POINTER (decl_name))
> + + sizeof ("__odr_asan_") + 1;

Please use size_t len instead of int len.  Why the + 1?  sizeof ("__odr_asan_")
should be already strlen ("__odr_asan_") + 1.

> +  name = XALLOCAVEC (char, len);
> +  name[len] = '\0';

This is buffer overflow.  Why do you need it at all?

> +  snprintf (name, len, "__odr_asan_%s", IDENTIFIER_POINTER (decl_name));

This should zero terminate the string.

Also, shouldn't this be followed by:
#ifndef NO_DOT_IN_LABEL
  name[sizeof ("__odr_asan") - 1] = '.';
#elif !defined(NO_DOLLAR_IN_LABEL)
  name[sizeof ("__odr_asan") - 1] = '$';
#endif

to make it not possible to clash with user symbols __odr_asan_foobar etc.
if possible (on targets which don't allow dots nor dollars in labels that is
not really possible, but at least elsewhere).

That said, if the __odr_asan* symbols are exported, it is part of ABI, so
what exactly does LLVM use in those cases?

> +/* { dg-final { scan-assembler-not ".*odr_asan_a.*" } } */
> +/* { dg-final { scan-assembler-not ".*odr_asan_b.*" } } */
> +/* { dg-final { scan-assembler ".*odr_asan_c.*" } } */

The .* on either side makes no sense, please remove those.
And, if the dot or dollar is replacing _, you need to use "odr_asan.a"
etc. in the regexps.

Otherwise LGTM.

Jakub

[PATCH v3] Support ASan ODR indicators at compiler side.

[Maxim Ostapenko]

Hi,

this is the third attempt to support ASan odr indicators in GCC. I've 
fixed the issue with odr indicator name (now we don't use 
ASM_GENERATE_INTERNAL_LABEL and use XALLOCAVEC for avoid overflow).

Also several style issues from previous review were covered.
How does it look now?

Tested and ASan bootstrapped on x86_64-unknown-linux-gnu.

-Maxim
commit 35cb5eb935bb1e29357e93f8e9a4e87fb4e9f511
Author: Maxim Ostapenko 
Date:   Fri Oct 28 10:22:03 2016 +0300

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.

diff --git a/config/ChangeLog b/config/ChangeLog
index ed59787..a5d5ff5 100644
--- a/config/ChangeLog
+++ b/config/ChangeLog
@@ -1,3 +1,8 @@
+2016-12-01  Maxim Ostapenko  
+
+	* bootstrap-asan.mk: Replace LSAN_OPTIONS=detect_leaks=0 with
+	ASAN_OPTIONS=detect_leaks=0:use_odr_indicator=1.
+
 2016-11-30  Matthias Klose  
 
 	* pkg.m4: New file.
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 b3cc6305..f19ac9d 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,13 @@
+2016-12-01  Maxim Ostapenko  
+
+	* 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-12-01  Jakub Jelinek  
 
 	PR target/78614
diff --git a/gcc/asan.c b/gcc/asan.c
index cb5d615..9db7481 100644
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -1388,6 +1388,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)
+{
+  return (DECL_ARTIFICIAL (decl)
+	  && lookup_attribute ("asan odr indicator", DECL_ATTRIBUTES (decl)));
+}
+
 /* 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
@@ -1436,7 +1446,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);
@@ -2266,14 +2277,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,
@@ -2295,6 +2307,60 @@ 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 *name;
+  tree uptr = TREE_TYPE (DECL_CHAIN (TYPE_FIELDS (type)));
+  tree decl_name
+= (HAS_DECL_ASSEMBLER_NAME_P (decl) ? DECL_ASSEMBLER_NAME (decl)
+	: DECL_NAME (decl));
+  /* DECL_NAME theoretically might be NULL.  Bail out with 0 in this case.  */
+  if (decl_name == NULL_TREE)
+return build_int_cst (uptr, 0);
+  int len = strlen (IDENTIFIER_POINTER (decl_name))
+	+ sizeof