| Message ID | 5829796A.4080602@samsung.com |
|---|---|
| State | New |
| Headers | show |
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.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 a76e3e8..64744b9 100644 > --- a/gcc/ChangeLog > +++ b/gcc/ChangeLog > @@ -1,3 +1,13 @@ > +2016-11-14 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-09 Kugan Vivekanandarajah <kuganv@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
On Mon, Nov 21, 2016 at 11:38 AM, Jakub Jelinek <jakub@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.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 a76e3e8..64744b9 100644 >> --- a/gcc/ChangeLog >> +++ b/gcc/ChangeLog >> @@ -1,3 +1,13 @@ >> +2016-11-14 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-09 Kugan Vivekanandarajah <kuganv@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
On Mon, Nov 21, 2016 at 11:43:56AM +0000, Yuri Gribov wrote: > > 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. What would those symbols help with in debugging bug reports? You need to have a source reproducer anyway, then anybody can try it himself. Even from just pure *.s file, you can look up where the .LASANODR1234 is used and from there find the corresponding symbol. Plus, as I said, with 95 chars or longer symbols (approx.) you get a buffer overflow. We don't use descriptive symbols in other internal asan, dwarf2 etc. labels. Jakub
On Mon, Nov 21, 2016 at 11:50 AM, Jakub Jelinek <jakub@redhat.com> wrote: > On Mon, Nov 21, 2016 at 11:43:56AM +0000, Yuri Gribov wrote: >> > 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. > > What would those symbols help with in debugging bug reports? > You need to have a source reproducer anyway, then anybody can try it > himself. Well, in case of some weird symbol error at startup we can at least understand which library/symbol to blame. > Even from just pure *.s file, you can look up where the > .LASANODR1234 is used and from there find the corresponding symbol. > Plus, as I said, with 95 chars or longer symbols (approx.) you get a buffer > overflow. We don't use descriptive symbols in other internal asan, dwarf2 > etc. labels. Note that indicators need to have default visibility so simple scheme like this will cause runtime collisions. -Iurii
On Mon, Nov 21, 2016 at 12:09:30PM +0000, Yuri Gribov wrote: > On Mon, Nov 21, 2016 at 11:50 AM, Jakub Jelinek <jakub@redhat.com> wrote: > > On Mon, Nov 21, 2016 at 11:43:56AM +0000, Yuri Gribov wrote: > >> > 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. > > > > What would those symbols help with in debugging bug reports? > > You need to have a source reproducer anyway, then anybody can try it > > himself. > > Well, in case of some weird symbol error at startup we can at least > understand which library/symbol to blame. > > > Even from just pure *.s file, you can look up where the > > .LASANODR1234 is used and from there find the corresponding symbol. > > Plus, as I said, with 95 chars or longer symbols (approx.) you get a buffer > > overflow. We don't use descriptive symbols in other internal asan, dwarf2 > > etc. labels. > > Note that indicators need to have default visibility so simple scheme > like this will cause runtime collisions. But then why do you use ASM_GENERATE_INTERNAL_LABEL? That is for internal labels. If the indicators are visible outside of TUs, then their mangling is an ABI thing. In that case you shouldn't add any kind of counter to them, but you should use something like __asan_odr.<name> or something similar, and if . is not supported in symbol names, use $ instead and if neither, then just disable the odr indicators. Or how exactly are these odr indicators supposed to work? Jakub
On 21/11/16 15:17, Jakub Jelinek wrote: > On Mon, Nov 21, 2016 at 12:09:30PM +0000, Yuri Gribov wrote: >> On Mon, Nov 21, 2016 at 11:50 AM, Jakub Jelinek <jakub@redhat.com> wrote: >>> On Mon, Nov 21, 2016 at 11:43:56AM +0000, Yuri Gribov wrote: >>>>> 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. >>> What would those symbols help with in debugging bug reports? >>> You need to have a source reproducer anyway, then anybody can try it >>> himself. >> Well, in case of some weird symbol error at startup we can at least >> understand which library/symbol to blame. >> >>> Even from just pure *.s file, you can look up where the >>> .LASANODR1234 is used and from there find the corresponding symbol. >>> Plus, as I said, with 95 chars or longer symbols (approx.) you get a buffer >>> overflow. We don't use descriptive symbols in other internal asan, dwarf2 >>> etc. labels. >> Note that indicators need to have default visibility so simple scheme >> like this will cause runtime collisions. > But then why do you use ASM_GENERATE_INTERNAL_LABEL? That is for internal > labels. If the indicators are visible outside of TUs, then their mangling > is an ABI thing. In that case you shouldn't add any kind of counter > to them, but you should use something like __asan_odr.<name> or something > similar, and if . is not supported in symbol names, use $ instead and if > neither, then just disable the odr indicators. > > Or how exactly are these odr indicators supposed to work? Yes, you just caught an error, __asan_odr.<name> is right thing to do here. I'm sorry about this. > > Jakub > > >
On 21/11/16 15:17, Jakub Jelinek wrote: > On Mon, Nov 21, 2016 at 12:09:30PM +0000, Yuri Gribov wrote: >> On Mon, Nov 21, 2016 at 11:50 AM, Jakub Jelinek <jakub@redhat.com> wrote: >>> On Mon, Nov 21, 2016 at 11:43:56AM +0000, Yuri Gribov wrote: >>>>> 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. >>> What would those symbols help with in debugging bug reports? >>> You need to have a source reproducer anyway, then anybody can try it >>> himself. >> Well, in case of some weird symbol error at startup we can at least >> understand which library/symbol to blame. >> >>> Even from just pure *.s file, you can look up where the >>> .LASANODR1234 is used and from there find the corresponding symbol. >>> Plus, as I said, with 95 chars or longer symbols (approx.) you get a buffer >>> overflow. We don't use descriptive symbols in other internal asan, dwarf2 >>> etc. labels. >> Note that indicators need to have default visibility so simple scheme >> like this will cause runtime collisions. > But then why do you use ASM_GENERATE_INTERNAL_LABEL? That is for internal > labels. If the indicators are visible outside of TUs, then their mangling > is an ABI thing. In that case you shouldn't add any kind of counter > to them, but you should use something like __asan_odr.<name> or something > similar, and if . is not supported in symbol names, use $ instead and if > neither, then just disable the odr indicators. > > Or how exactly are these odr indicators supposed to work? Odr indicators act as visible "delegates" of protected by ASan globals (their private aliases actually). We introduce them to catch cross-dso symbols clashing at runtime. Of course, some people intentionally use ELF interposition and ASan would generate false alarm there, but a) we can use suppressions to avoid false alarms here and b) I believe in most cases such an alarm would indicate a real bug in user's code. -Maxim > > Jakub > > >
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.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 a76e3e8..64744b9 100644 --- a/gcc/ChangeLog +++ b/gcc/ChangeLog @@ -1,3 +1,13 @@ +2016-11-14 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-09 Kugan Vivekanandarajah <kuganv@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)); +} + /* 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,56 @@ 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) = 1; + TREE_READONLY (var) = 0; + TREE_THIS_VOLATILE (var) = 1; + DECL_GIMPLE_REG_P (var) = 0; + DECL_ARTIFICIAL (var) = 1; + DECL_IGNORED_P (var) = 1; + TREE_STATIC (var) = 1; + TREE_PUBLIC (var) = 1; + DECL_VISIBILITY (var) = VISIBILITY_DEFAULT; + DECL_VISIBILITY_SPECIFIED (var) = 1; + + 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; + DECL_ATTRIBUTES (var) = tree_cons (get_identifier ("asan odr indicator"), + NULL, DECL_ATTRIBUTES (var)); + 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. */ @@ -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); CONSTRUCTOR_APPEND_ELT (vinner, NULL_TREE, fold_convert (const_ptr_type_node, build_fold_addr_expr (refdecl))); @@ -2382,8 +2447,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/c-family/c-attribs.c b/gcc/c-family/c-attribs.c index 925f1b2..44b1f51 100644 --- a/gcc/c-family/c-attribs.c +++ b/gcc/c-family/c-attribs.c @@ -57,6 +57,8 @@ static tree handle_no_address_safety_analysis_attribute (tree *, tree, tree, int, bool *); static tree handle_no_sanitize_undefined_attribute (tree *, tree, tree, int, bool *); +static tree handle_asan_odr_indicator_attribute (tree *, tree, tree, int, + bool *); static tree handle_stack_protect_attribute (tree *, tree, tree, int, bool *); static tree handle_noinline_attribute (tree *, tree, tree, int, bool *); static tree handle_noclone_attribute (tree *, tree, tree, int, bool *); @@ -292,6 +294,9 @@ const struct attribute_spec c_common_attribute_table[] = { "no_sanitize_undefined", 0, 0, true, false, false, handle_no_sanitize_undefined_attribute, false }, + { "asan odr indicator", 0, 0, true, false, false, + handle_asan_odr_indicator_attribute, + false }, { "warning", 1, 1, true, false, false, handle_error_attribute, false }, { "error", 1, 1, true, false, false, @@ -591,6 +596,22 @@ handle_no_sanitize_undefined_attribute (tree *node, tree name, tree, int, return NULL_TREE; } +/* 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; + } + + return NULL_TREE; +} + /* Handle a "stack_protect" attribute; arguments as in struct attribute_spec.handler. */ diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog index e822e6f..67361a3 100644 --- a/gcc/testsuite/ChangeLog +++ b/gcc/testsuite/ChangeLog @@ -1,3 +1,7 @@ +2016-11-14 Maxim Ostapenko <m.ostapenko@samsung.com> + + * c-c++-common/asan/no-redundant-odr-indicators-1.c: New test. + 2016-11-09 Kugan Vivekanandarajah <kuganv@linaro.org> * gcc.dg/ipa/vrp7.c: New test. 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.*" } } */