On Mon, Nov 21, 2016 at 11:38 AM, Jakub Jelinek <ja...@redhat.com> wrote: > 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.
Given that feature is quite new and hasn't been tested too much (it's off by default in Clang), having a descriptive name may aid with debugging bug reports. >> + 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