Re: [PATCH v3][PR lto/79061] Fix LTO plus ASAN fails with "AddressSanitizer: initialization-order-fiasco".

[Jakub Jelinek]

On Thu, Feb 02, 2017 at 06:02:07PM +0300, Maxim Ostapenko wrote:
> Hi,
> 
> PR lto/79061 actually affects gcc-{5, 6}-branch too
> Is it OK to apply following patch on branches?
> 
> -Maxim

> gcc/ChangeLog:
> 
> 2017-02-02  Maxim Ostapenko  
> 
>   PR lto/79061
>   * asan.c (asan_add_global): Force has_dynamic_init to zero in LTO mode.

Ok, thanks.

Jakub

Re: [PATCH v3][PR lto/79061] Fix LTO plus ASAN fails with "AddressSanitizer: initialization-order-fiasco".

[Maxim Ostapenko]

Hi,

PR lto/79061 actually affects gcc-{5, 6}-branch too
Is it OK to apply following patch on branches?

-Maxim
gcc/ChangeLog:

2017-02-02  Maxim Ostapenko  

	PR lto/79061
	* asan.c (asan_add_global): Force has_dynamic_init to zero in LTO mode.

diff --git a/gcc/asan.c b/gcc/asan.c
index 398a508..0f55dc0 100644
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -2275,7 +2275,11 @@ asan_add_global (tree decl, tree type, vec *v)
   CONSTRUCTOR_APPEND_ELT (vinner, NULL_TREE,
 			  fold_convert (const_ptr_type_node, module_name_cst));
   varpool_node *vnode = varpool_node::get (decl);
-  int has_dynamic_init = vnode ? vnode->dynamically_initialized : 0;
+  int has_dynamic_init = 0;
+  /* FIXME: Enable initialization order fiasco detection in LTO mode once
+ proper fix for PR 79061 will be applied.  */
+  if (!in_lto_p)
+has_dynamic_init = vnode ? vnode->dynamically_initialized : 0;
   CONSTRUCTOR_APPEND_ELT (vinner, NULL_TREE,
 			  build_int_cst (uptr, has_dynamic_init));
   tree locptr = NULL_TREE;

Re: [PATCH v3][PR lto/79061] Fix LTO plus ASAN fails with "AddressSanitizer: initialization-order-fiasco".

[Jakub Jelinek]

On Tue, Jan 31, 2017 at 08:59:27AM +0100, Richard Biener wrote:
> > The problem is that at least right now the handling of the init-order is
> > done in 2 separate places.
> > The C++ FE emits the special libasan calls with the name of the TU at the
> > beginning and end of the static initialization.
> > And then the variables need to be registered with the libasan runtime from
> > an even earlier constructor, and this is something that is done very late
> > (where we collect all the variables).
> 
> So we are basically telling libasan a list of dynamic initialized vars plus
> when they start/end being constructed so it can catch access to uninitialized
> vars during init of others?

We are telling libasan a list of all sanitized variables (those where we
managed to insert the padding around them in the end) and in that list mark
variables with dynamic initialization.  So, we can't e.g. register vars
where we for whatever reason can't add the padding around them, and this is
something that is decided when the variables are finalized.
Thus, constructing the list early (as a VAR_DECL with initializer) means
we'd then need to be able to remove stuff from it etc.
It looks much easier to me to carry this list only in the LTO data
structures (basically remember which TU owns each var).

> 
> I don't see why we need to compose that list of vars so late then.  Just
> generate it early, say, after the first swoop of unused var removal.  Use
> weak references so later removed ones get NULL.  Or simply bite the
> bullet of asan changing code gen (well, it does that anyway) and thus
> "pin" all vars life at that point as used.
> 
> Sounds much easier to me than carrying this over LTO...
> 
> And I suppose the TU name is only used for diagnostics?  Otherwise
> the symbol name (DECL_ASSEMBLER_NAME) of the symbol could
> be used as in C++ globals need to follow ODR?

The way it works is that you register the sanitized variables and each var
has a string for the owning TU (it is used also for diagnostics, so it is
desirable to not use random strings).  Then when the start of dynamic
initialization for some TU is called, libasan poisons all dynamic_init
global variables except those that have the owning TU string equal to the
current TU.  Then the construction is run (which means it will fail
if it accesses a dynamically initialized variable from some other TU),
and finally another libasan routine is called which will unpoison all the
variables it poisoned earlier.  So, for this use it is desirable the
names are actually the TU names, what you pass in corresponding static
initialization to the libasan functions.

Jakub

Re: [PATCH v3][PR lto/79061] Fix LTO plus ASAN fails with "AddressSanitizer: initialization-order-fiasco".

[Richard Biener]

On Mon, Jan 30, 2017 at 4:26 PM, Jakub Jelinek  wrote:
> On Mon, Jan 30, 2017 at 04:14:40PM +0100, Richard Biener wrote:
>> > as was figured out in PR, using DECL_NAME (TRANSLATION_UNIT_DECL) does not
>> > always give us a correct module name in LTO mode because e.g. DECL_CONTEXT 
>> > of
>> > some variables can be NAMESPACE_DECL and LTO merges NAMESPACE_DECLs.
>>
>> Yes, it indeed does.  Note that GCC8+ both TU decls and NAMESPACE_DECLs
>> should no longer be neccessary and eventually vanish completely...
>> (in lto1, that is).  Can we code-gen the init order stuff early before
>> LTO write-out?
>
> The problem is that at least right now the handling of the init-order is
> done in 2 separate places.
> The C++ FE emits the special libasan calls with the name of the TU at the
> beginning and end of the static initialization.
> And then the variables need to be registered with the libasan runtime from
> an even earlier constructor, and this is something that is done very late
> (where we collect all the variables).

So we are basically telling libasan a list of dynamic initialized vars plus
when they start/end being constructed so it can catch access to uninitialized
vars during init of others?

I don't see why we need to compose that list of vars so late then.  Just
generate it early, say, after the first swoop of unused var removal.  Use
weak references so later removed ones get NULL.  Or simply bite the
bullet of asan changing code gen (well, it does that anyway) and thus
"pin" all vars life at that point as used.

Sounds much easier to me than carrying this over LTO...

And I suppose the TU name is only used for diagnostics?  Otherwise
the symbol name (DECL_ASSEMBLER_NAME) of the symbol could
be used as in C++ globals need to follow ODR?

> So the options for LTO are:
> 1) somewhere preserve (at least for dynamically_initialized vars) the TU
> it came originally from, if some dynamically_initialized var is owned by
> more TUs, just drop that flag
> 2) rewrite in LTO the dynamic_init libasan calls (register with the whole
> LTO partition name rather than individual original TUs); this has the major
> disadvantage that it will not diagnose initialization order bugs between
> vars from TUs from the same LTO partition
> 3) create the table of global vars for dynamically_initialized vars early
> (save the artificial array with the var addresses + ctor into LTO bytecode),
> at least for LTO, and then just register the non-dynamically_initialized
> vars later (not really good idea for non-LTO, we want to register all the
> vars from the whole TU together
>
> 1) looks easiest to mebut can grow varpool_node struct by a pointer size
>
> Jakub

Re: [PATCH v3][PR lto/79061] Fix LTO plus ASAN fails with "AddressSanitizer: initialization-order-fiasco".

[Jakub Jelinek]

On Mon, Jan 30, 2017 at 04:14:40PM +0100, Richard Biener wrote:
> > as was figured out in PR, using DECL_NAME (TRANSLATION_UNIT_DECL) does not
> > always give us a correct module name in LTO mode because e.g. DECL_CONTEXT 
> > of
> > some variables can be NAMESPACE_DECL and LTO merges NAMESPACE_DECLs.
> 
> Yes, it indeed does.  Note that GCC8+ both TU decls and NAMESPACE_DECLs
> should no longer be neccessary and eventually vanish completely...
> (in lto1, that is).  Can we code-gen the init order stuff early before
> LTO write-out?

The problem is that at least right now the handling of the init-order is
done in 2 separate places.
The C++ FE emits the special libasan calls with the name of the TU at the
beginning and end of the static initialization.
And then the variables need to be registered with the libasan runtime from
an even earlier constructor, and this is something that is done very late
(where we collect all the variables).
So the options for LTO are:
1) somewhere preserve (at least for dynamically_initialized vars) the TU
it came originally from, if some dynamically_initialized var is owned by
more TUs, just drop that flag
2) rewrite in LTO the dynamic_init libasan calls (register with the whole
LTO partition name rather than individual original TUs); this has the major
disadvantage that it will not diagnose initialization order bugs between
vars from TUs from the same LTO partition
3) create the table of global vars for dynamically_initialized vars early
(save the artificial array with the var addresses + ctor into LTO bytecode),
at least for LTO, and then just register the non-dynamically_initialized
vars later (not really good idea for non-LTO, we want to register all the
vars from the whole TU together

1) looks easiest to mebut can grow varpool_node struct by a pointer size

Jakub

Re: [PATCH v3][PR lto/79061] Fix LTO plus ASAN fails with "AddressSanitizer: initialization-order-fiasco".

[Jakub Jelinek]

On Mon, Jan 30, 2017 at 06:09:36PM +0300, Maxim Ostapenko wrote:
> Hi,
> 
> as was figured out in PR, using DECL_NAME (TRANSLATION_UNIT_DECL) does not
> always give us a correct module name in LTO mode because e.g. DECL_CONTEXT
> of some variables can be NAMESPACE_DECL and LTO merges NAMESPACE_DECLs. The
> easiest fix is just to disable the initialization order checking altogether
> for LTO by forcing dynamically_initialized = 0 in LTO for now but we'll
> probably want to restore initialization order fiasco detection in the future
> (e.g. for GCC 8).
> This patch just disables initialization order fiasco detection for LTO for
> now. Tested and bootstrapped on x86_64-unknown-linux-gnu, OK to apply?
> Or do I need to cook a proper fix for GCC 7 (and branches) and come back
> then?
> 
> -Maxim

> gcc/ChangeLog:
> 
> 2017-01-30  Maxim Ostapenko  
> 

Please add
PR lto/79061
here.
>   * asan.c (get_translation_unit_decl): Remove function.
>   (asan_add_global): Force has_dynamic_init to zero in LTO mode.

Ok with that change.

Jakub

Re: [PATCH v3][PR lto/79061] Fix LTO plus ASAN fails with "AddressSanitizer: initialization-order-fiasco".

[Richard Biener]

On Mon, 30 Jan 2017, Maxim Ostapenko wrote:

> Hi,
> 
> as was figured out in PR, using DECL_NAME (TRANSLATION_UNIT_DECL) does not
> always give us a correct module name in LTO mode because e.g. DECL_CONTEXT of
> some variables can be NAMESPACE_DECL and LTO merges NAMESPACE_DECLs.

Yes, it indeed does.  Note that GCC8+ both TU decls and NAMESPACE_DECLs
should no longer be neccessary and eventually vanish completely...
(in lto1, that is).  Can we code-gen the init order stuff early before
LTO write-out?

> The easiest fix is just to disable the initialization order checking 
> altogether
> for LTO by forcing dynamically_initialized = 0 in LTO for now but we'll
> probably want to restore initialization order fiasco detection in the future
> (e.g. for GCC 8).
> This patch just disables initialization order fiasco detection for LTO for
> now. Tested and bootstrapped on x86_64-unknown-linux-gnu, OK to apply?
> Or do I need to cook a proper fix for GCC 7 (and branches) and come back then?

The patch works for me.

Richard.

[PATCH v3][PR lto/79061] Fix LTO plus ASAN fails with "AddressSanitizer: initialization-order-fiasco".

[Maxim Ostapenko]

Hi,

as was figured out in PR, using DECL_NAME (TRANSLATION_UNIT_DECL) does 
not always give us a correct module name in LTO mode because e.g. 
DECL_CONTEXT of some variables can be NAMESPACE_DECL and LTO merges 
NAMESPACE_DECLs. The easiest fix is just to disable the initialization 
order checking altogether for LTO by forcing dynamically_initialized = 0 
in LTO for now but we'll probably want to restore initialization order 
fiasco detection in the future (e.g. for GCC 8).
This patch just disables initialization order fiasco detection for LTO 
for now. Tested and bootstrapped on x86_64-unknown-linux-gnu, OK to apply?
Or do I need to cook a proper fix for GCC 7 (and branches) and come back 
then?


-Maxim
gcc/ChangeLog:

2017-01-30  Maxim Ostapenko  

	* asan.c (get_translation_unit_decl): Remove function.
	(asan_add_global): Force has_dynamic_init to zero in LTO mode.

diff --git a/gcc/asan.c b/gcc/asan.c
index 9098121..6cdd59b 100644
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -2373,22 +2373,6 @@ asan_needs_odr_indicator_p (tree decl)
 	  && TREE_PUBLIC (decl));
 }
 
-/* For given DECL return its corresponding TRANSLATION_UNIT_DECL.  */
-
-static const_tree
-get_translation_unit_decl (tree decl)
-{
-  const_tree context = decl;
-  while (context && TREE_CODE (context) != TRANSLATION_UNIT_DECL)
-{
-  if (TREE_CODE (context) == BLOCK)
-	context = BLOCK_SUPERCONTEXT (context);
-  else
-	context = get_containing_scope (context);
-}
-  return context;
-}
-
 /* Append description of a single global DECL into vector V.
TYPE is __asan_global struct type as returned by asan_global_struct.  */
 
@@ -2408,14 +2392,7 @@ asan_add_global (tree decl, tree type, vec *v)
 pp_string (_pp, "");
   str_cst = asan_pp_string (_pp);
 
-  const char *filename = main_input_filename;
-  if (in_lto_p)
-{
-  const_tree translation_unit_decl = get_translation_unit_decl (decl);
-  if (translation_unit_decl && DECL_NAME (translation_unit_decl) != NULL)
-	filename = IDENTIFIER_POINTER (DECL_NAME (translation_unit_decl));
-}
-  pp_string (_name_pp, filename);
+  pp_string (_name_pp, main_input_filename);
   module_name_cst = asan_pp_string (_name_pp);
 
   if (asan_needs_local_alias (decl))
@@ -2451,7 +2428,11 @@ asan_add_global (tree decl, tree type, vec *v)
   CONSTRUCTOR_APPEND_ELT (vinner, NULL_TREE,
 			  fold_convert (const_ptr_type_node, module_name_cst));
   varpool_node *vnode = varpool_node::get (decl);
-  int has_dynamic_init = vnode ? vnode->dynamically_initialized : 0;
+  int has_dynamic_init = 0;
+  /* FIXME: Enable initialization order fiasco detection in LTO mode once
+ proper fix for PR 79061 will be applied.  */
+  if (!in_lto_p)
+has_dynamic_init = vnode ? vnode->dynamically_initialized : 0;
   CONSTRUCTOR_APPEND_ELT (vinner, NULL_TREE,
 			  build_int_cst (uptr, has_dynamic_init));
   tree locptr = NULL_TREE;