Re: [Ping][PATCH v3] Fix Incorrect ASan global variables alignment on arm (PR sanitizer/81697)

[Jakub Jelinek <jakub at redhat dot com>]

On Thu, Nov 30, 2017 at 02:38:25PM +0300, Maxim Ostapenko wrote:
> Hi Jakub, thanks for review.
> 
> I've fixed the issues you've pointed in review.
> Regarding a testcase -- I've cooked a runtime test, but it shows FP on
> unpatched GCC version only when linking with Gold (because it strips
> redzones more aggressively).

I think we can live with that.

> --- a/gcc/varasm.c
> +++ b/gcc/varasm.c
> @@ -6550,7 +6550,19 @@ categorize_decl_for_section (const_tree decl, int reloc)
>  	ret = reloc == 1 ? SECCAT_DATA_REL_RO_LOCAL : SECCAT_DATA_REL_RO;
>        else if (reloc || flag_merge_constants < 2
>  	       || ((flag_sanitize & SANITIZE_ADDRESS)
> -		   && asan_protect_global (CONST_CAST_TREE (decl))))
> +		   /* PR 81697: for architectures that use section anchors we
> +		      need to ignore DECL_RTL_SET_P (decl) for string constants
> +		      inside this asan_protect_global call because otherwise
> +		      we'll wrongly put them into SECCAT_RODATA_MERGE_CONST
> +		      section, set DECL_RTL (decl) later on and add DECL to
> +		      protected globals via successive asan_protect_global
> +		      calls.  In this scenario we'll end up with wrong
> +		      alignment of these strings at runtime and possible ASan
> +		      false positives.  */
> +		   && asan_protect_global (CONST_CAST_TREE (decl),
> +					   use_object_blocks_p ()
> +					     && use_blocks_for_decl_p (
> +						  CONST_CAST_TREE (decl)))))

Formatting is too bad here.  && should go below use_object_block_p..
The opening ( should either go on the next line, like:
					   use_object_blocks_p ()
					   && use_blocks_for_decl_p
						(CONST_CAST_TREE (decl)))))
or perhaps better just introduce a temporary somewhere:
   else if (VAR_P (decl))
     {
+      tree d = CONST_CAST_TREE (decl);
       if (bss_initializer_p (decl))
         ret = SECCAT_BSS;
and use d instead of CONST_CAST_TREE (decl) later?

Ok with those changes.

	Jakub