On Mon, Nov 14, 2016 at 11:44:26AM +0300, Maxim Ostapenko wrote:
> this is the second attempt to support ASan odr indicators in GCC. I've fixed
> issues with several flags (e.g.TREE_ADDRESSABLE) and introduced new "asan
> odr indicator" attribute to distinguish indicators from other symbols.
> Looks better now?
>
> Tested and ASan bootstrapped on x86_64-unknown-linux-gnu.
>
> -Maxim
> 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/c-family/
>
> * c-attribs.c (asan odr indicator): New attribute.
> (handle_asan_odr_indicator_attribute): New function.
>
> gcc/testsuite/
>
> * c-c++-common/asan/no-redundant-odr-indicators-1.c: New test.
>
> diff --git a/config/ChangeLog b/config/ChangeLog
> index 3b0092b..0c75185 100644
> --- a/config/ChangeLog
> +++ b/config/ChangeLog
> @@ -1,3 +1,8 @@
> +2016-11-14 Maxim Ostapenko <m.ostape...@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+...@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 a76e3e8..64744b9 100644
> --- a/gcc/ChangeLog
> +++ b/gcc/ChangeLog
> @@ -1,3 +1,13 @@
> +2016-11-14 Maxim Ostapenko <m.ostape...@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-09 Kugan Vivekanandarajah <kug...@linaro.org>
>
> * ipa-cp.c (ipa_get_jf_pass_through_result): Handle unary expressions.
> diff --git a/gcc/asan.c b/gcc/asan.c
> index 6e93ea3..1191ebe 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));
Better use
return (DECL_ARTIFICIAL (decl)
&& lookup_attribute ("asan odr indicator", DECL_ATTRIBUTES (decl)));
at least emacs users most likely need that.
> - "__name", "__module_name", "__has_dynamic_init", "__location",
> "__odr_indicator"};
> - tree fields[8], ret;
> - int i;
> + "__name", "__module_name", "__has_dynamic_init", "__location",
> + "__odr_indicator"};
Please put space before };.
> +/* 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);
This is just weird. DECL_NAME in theory could be NULL, or can be a symbol
much longer than 100 bytes, at which point you have strlen (tmp_name) == 99
and ASM_GENERATE_INTERNAL_LABEL will just misbehave.
I fail to see why you need tmp_name at all, I'd go just with
char sym_name[40];
ASM_GENERATE_INTERNAL_LABEL (sym_name, "LASANODR", ++lasan_odr_ind_cnt);
or so.
> + char *asterisk = sym_name;
> + while ((asterisk = strchr (asterisk, '*')))
> + *asterisk = '_';
Can't * be just at the beginning? And other ASM_GENERATE_INTERNAL_LABEL +
build_decl with get_identifier spots in asan.c certainly don't strip any.
> @@ -2335,6 +2397,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);
Again for emacs users I think you want () around the whole RHS.
> +/* Handle an "asan odr indicator" attribute; arguments as in
> + struct attribute_spec.handler. */
> +
> +static tree
> +handle_asan_odr_indicator_attribute (tree *node, tree name, tree, int,
> + bool *no_add_attrs)
> +{
> + if (!VAR_P (*node))
> + {
> + warning (OPT_Wattributes, "%qE attribute ignored", name);
> + *no_add_attrs = true;
> + }
Why not just return NULL_TREE and all arguments nameless?
This isn't user accessible attribute, so it shouldn't be misplaced.
Jakub
