This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH v2] Support ASan ODR indicators at compiler side.
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