diff mbox

Enable Ada bootstrap with LTO

Message ID 201011270042.36210.ebotcazou@adacore.com
State New
Headers show

Commit Message

Eric Botcazou Nov. 26, 2010, 11:42 p.m. UTC 
Hi,

this finally enables Ada bootstrap with LTO.  It successfully completed with 
only the gnatvsn.adb patchlet until yesterday but, after resyncing the tree 
this morning, it started crashing during stage #2 LTRANS for gnat1.

The failure mode is as follows: during the LGEN phase, a VAR_DECL with VLA 
type is gimplified, i.e. replaced with a pointer VAR_DECL in the code, and 
DECL_VALUE_EXPR for it is set to the dereference of the pointer.  Then this 
pointer VAR_DECL is put into the FRAME structure by the nested functions 
lowering pass and the reference to it present in the above DECL_VALUE_EXPR is 
replaced with a debug variable, which itself has DECL_VALUE_EXPR pointing to 
the rewritten access.  During the LTRANS phase, the VAR_DECL is correctly 
read in with DECL_VALUE_EXPR pointing to the dereference of the debug pointer 
variable.  Then lto_materialize_function triggers a GC for an unrelated 
function which sweeps the debug pointer variable and, consequently, removes 
its slot from the DECL_VALUE_EXPR table.  So the original VAR_DECL has its 
DECL_VALUE_EXPR pointing to the dereference of a dangling debug variable 
with DECL_HAS_VALUE_EXPR_P set but no entry in the DECL_VALUE_EXPR table and 
this crashes during debug info generation.

The patch addresses this issue in the nested functions lowering pass by 
replacing references to VAR_DECLs put into the FRAME structure present in 
DECL_VALUE_EXPRs by the rewritten access, instead of the debug variable.

Bootstrapped/regtested on x86_64-suse-linux, OK for the mainline?


2010-11-26  Eric Botcazou  <ebotcazou@adacore.com>

	* bootstrap-lto.mk (BOOT_ADAFLAGS): Delete.


2010-11-26  Eric Botcazou  <ebotcazou@adacore.com>

	* tree-nested.c (remap_vla_decls): Fully expand value expressions of
	VLA variables.


2010-11-26  Eric Botcazou  <ebotcazou@adacore.com>

	* gnatvsn.adb (Version_String): Change type to C-like array of chars.
	(Gnat_Version_String): Adjust to above change.

Comments

Ralf Wildenhues Nov. 27, 2010, 8:15 a.m. UTC | #1 
* Eric Botcazou wrote on Sat, Nov 27, 2010 at 12:42:36AM CET:
> 2010-11-26  Eric Botcazou  <ebotcazou@adacore.com>
> 
> 	* bootstrap-lto.mk (BOOT_ADAFLAGS): Delete.
> 

FWIW, the build parts are obvious.

Thanks,
Ralf
Ralf Wildenhues Nov. 27, 2010, 8:55 a.m. UTC | #2 
* Ralf Wildenhues wrote on Sat, Nov 27, 2010 at 09:15:55AM CET:
> * Eric Botcazou wrote on Sat, Nov 27, 2010 at 12:42:36AM CET:
> > 2010-11-26  Eric Botcazou  <ebotcazou@adacore.com>
> > 
> > 	* bootstrap-lto.mk (BOOT_ADAFLAGS): Delete.
> > 
> 
> FWIW, the build parts are obvious.

... but need syncing to src.

Sorry for the noise, going for coffee now ...
Richard Biener Nov. 27, 2010, 11:27 a.m. UTC | #3 
On Sat, Nov 27, 2010 at 12:42 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
> Hi,
>
> this finally enables Ada bootstrap with LTO.  It successfully completed with
> only the gnatvsn.adb patchlet until yesterday but, after resyncing the tree
> this morning, it started crashing during stage #2 LTRANS for gnat1.
>
> The failure mode is as follows: during the LGEN phase, a VAR_DECL with VLA
> type is gimplified, i.e. replaced with a pointer VAR_DECL in the code, and
> DECL_VALUE_EXPR for it is set to the dereference of the pointer.  Then this
> pointer VAR_DECL is put into the FRAME structure by the nested functions
> lowering pass and the reference to it present in the above DECL_VALUE_EXPR is
> replaced with a debug variable, which itself has DECL_VALUE_EXPR pointing to
> the rewritten access.  During the LTRANS phase, the VAR_DECL is correctly
> read in with DECL_VALUE_EXPR pointing to the dereference of the debug pointer
> variable.  Then lto_materialize_function triggers a GC for an unrelated
> function which sweeps the debug pointer variable and, consequently, removes
> its slot from the DECL_VALUE_EXPR table.  So the original VAR_DECL has its
> DECL_VALUE_EXPR pointing to the dereference of a dangling debug variable
> with DECL_HAS_VALUE_EXPR_P set but no entry in the DECL_VALUE_EXPR table and
> this crashes during debug info generation.
>
> The patch addresses this issue in the nested functions lowering pass by
> replacing references to VAR_DECLs put into the FRAME structure present in
> DECL_VALUE_EXPRs by the rewritten access, instead of the debug variable.
>
> Bootstrapped/regtested on x86_64-suse-linux, OK for the mainline?

I suppose we re-gimplify the INDIRECT_REFs, but it would be better to
generate MEM_REFs in the first place - just check for MEM_REF and
use build_simple_mem_ref.

The patch is ok with that change.

Thanks,
Richard.

>
> 2010-11-26  Eric Botcazou  <ebotcazou@adacore.com>
>
>        * bootstrap-lto.mk (BOOT_ADAFLAGS): Delete.
>
>
> 2010-11-26  Eric Botcazou  <ebotcazou@adacore.com>
>
>        * tree-nested.c (remap_vla_decls): Fully expand value expressions of
>        VLA variables.
>
>
> 2010-11-26  Eric Botcazou  <ebotcazou@adacore.com>
>
>        * gnatvsn.adb (Version_String): Change type to C-like array of chars.
>        (Gnat_Version_String): Adjust to above change.
>
>
> --
> Eric Botcazou
>
Eric Botcazou Nov. 27, 2010, 11:38 a.m. UTC | #4 
> I suppose we re-gimplify the INDIRECT_REFs, but it would be better to
> generate MEM_REFs in the first place - just check for MEM_REF and
> use build_simple_mem_ref.

No, the INDIRECT_REFs present in DECL_VALUE_EXPRs aren't re-gimplified so they 
reach tree-nested.c, go through it and are rematerialized during LTRANS.
Richard Biener Nov. 27, 2010, 2:40 p.m. UTC | #5 
On Sat, Nov 27, 2010 at 12:38 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> I suppose we re-gimplify the INDIRECT_REFs, but it would be better to
>> generate MEM_REFs in the first place - just check for MEM_REF and
>> use build_simple_mem_ref.
>
> No, the INDIRECT_REFs present in DECL_VALUE_EXPRs aren't re-gimplified so they
> reach tree-nested.c, go through it and are rematerialized during LTRANS.

Huh, but we can't expand them.  We shouldn't have INDIRECT_REFs in the
IL anymore.  Hmm, maybe dwarf2out can still deal with them though.

Richard.
Eric Botcazou Nov. 27, 2010, 3:18 p.m. UTC | #6 
> Huh, but we can't expand them.  We shouldn't have INDIRECT_REFs in the
> IL anymore.  Hmm, maybe dwarf2out can still deal with them though.

Yes, loc_list_from_tree still supports them:

    case MEM_REF:
      /* ??? FIXME.  */
      if (!integer_zerop (TREE_OPERAND (loc, 1)))
	return 0;
      /* Fallthru.  */
    case INDIRECT_REF:
      list_ret = loc_list_from_tree (TREE_OPERAND (loc, 0), 0);
      have_address = 1;
      break;
Richard Biener Nov. 27, 2010, 3:27 p.m. UTC | #7 
On Sat, Nov 27, 2010 at 4:18 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Huh, but we can't expand them.  We shouldn't have INDIRECT_REFs in the
>> IL anymore.  Hmm, maybe dwarf2out can still deal with them though.
>
> Yes, loc_list_from_tree still supports them:
>
>    case MEM_REF:
>      /* ??? FIXME.  */
>      if (!integer_zerop (TREE_OPERAND (loc, 1)))
>        return 0;
>      /* Fallthru.  */
>    case INDIRECT_REF:
>      list_ret = loc_list_from_tree (TREE_OPERAND (loc, 0), 0);
>      have_address = 1;
>      break;

Patch is ok then.

Thanks,
Richard.

> --
> Eric Botcazou
>
Eric Botcazou Nov. 27, 2010, 3:55 p.m. UTC | #8 
> > FWIW, the build parts are obvious.
>
> ... but need syncing to src.

Thanks for the heads up, applied to both repositories.
diff mbox

Patch

Index: gcc/ada/gnatvsn.adb
===================================================================
--- gcc/ada/gnatvsn.adb	(revision 167175)
+++ gcc/ada/gnatvsn.adb	(working copy)
@@ -53,9 +53,10 @@  package body Gnatvsn is
         " FOR A PARTICULAR PURPOSE.";
    end Gnat_Free_Software;
 
-   Version_String : String (1 .. Ver_Len_Max);
+   type char_array is array (Natural range <>) of aliased Character;
+   Version_String : char_array (0 .. Ver_Len_Max - 1);
    --  Import the C string defined in the (language-independent) source file
-   --  version.c.
+   --  version.c using the zero-based convention of the C language.
    --  The size is not the real one, which does not matter since we will
    --  check for the nul character in Gnat_Version_String.
    pragma Import (C, Version_String, "version_string");
@@ -65,15 +66,17 @@  package body Gnatvsn is
    -------------------------
 
    function Gnat_Version_String return String is
-      NUL_Pos : Positive := 1;
+      S : String (1 .. Ver_Len_Max);
+      Pos : Natural := 0;
    begin
       loop
-         exit when Version_String (NUL_Pos) = ASCII.NUL;
+         exit when Version_String (Pos) = ASCII.NUL;
 
-         NUL_Pos := NUL_Pos + 1;
+         S (Pos + 1) := Version_String (Pos);
+         Pos := Pos + 1;
       end loop;
 
-      return Version_String (1 .. NUL_Pos - 1);
+      return S (1 .. Pos);
    end Gnat_Version_String;
 
 end Gnatvsn;
Index: gcc/tree-nested.c
===================================================================
--- gcc/tree-nested.c	(revision 167175)
+++ gcc/tree-nested.c	(working copy)
@@ -2192,18 +2192,21 @@  remap_vla_decls (tree block, struct nest
     remap_vla_decls (subblock, root);
 
   for (var = BLOCK_VARS (block); var; var = DECL_CHAIN (var))
-    {
-      if (TREE_CODE (var) == VAR_DECL
-	  && variably_modified_type_p (TREE_TYPE (var), NULL)
-	  && DECL_HAS_VALUE_EXPR_P (var))
-	{
-	  type = TREE_TYPE (var);
-	  val = DECL_VALUE_EXPR (var);
-	  if (walk_tree (&type, contains_remapped_vars, root, NULL) != NULL
-	      ||  walk_tree (&val, contains_remapped_vars, root, NULL) != NULL)
-	    break;
-	}
-    }
+    if (TREE_CODE (var) == VAR_DECL && DECL_HAS_VALUE_EXPR_P (var))
+      {
+	val = DECL_VALUE_EXPR (var);
+	type = TREE_TYPE (var);
+
+	if (!(TREE_CODE (val) == INDIRECT_REF
+	      && TREE_CODE (TREE_OPERAND (val, 0)) == VAR_DECL
+	      && variably_modified_type_p (type, NULL)))
+	  continue;
+
+	if (pointer_map_contains (root->var_map, TREE_OPERAND (val, 0))
+	    || walk_tree (&type, contains_remapped_vars, root, NULL))
+	  break;
+      }
+
   if (var == NULL_TREE)
     return;
 
@@ -2213,17 +2216,22 @@  remap_vla_decls (tree block, struct nest
   id.root = root;
 
   for (; var; var = DECL_CHAIN (var))
-    if (TREE_CODE (var) == VAR_DECL
-	&& variably_modified_type_p (TREE_TYPE (var), NULL)
-	&& DECL_HAS_VALUE_EXPR_P (var))
+    if (TREE_CODE (var) == VAR_DECL && DECL_HAS_VALUE_EXPR_P (var))
       {
 	struct nesting_info *i;
-	tree newt, t, context;
+	tree newt, context;
+	void **slot;
 
-	t = type = TREE_TYPE (var);
 	val = DECL_VALUE_EXPR (var);
-	if (walk_tree (&type, contains_remapped_vars, root, NULL) == NULL
-	    && walk_tree (&val, contains_remapped_vars, root, NULL) == NULL)
+	type = TREE_TYPE (var);
+
+	if (!(TREE_CODE (val) == INDIRECT_REF
+	      && TREE_CODE (TREE_OPERAND (val, 0)) == VAR_DECL
+	      && variably_modified_type_p (type, NULL)))
+	  continue;
+
+	slot = pointer_map_contains (root->var_map, TREE_OPERAND (val, 0));
+	if (!slot && !walk_tree (&type, contains_remapped_vars, root, NULL))
 	  continue;
 
 	context = decl_function_context (var);
@@ -2234,6 +2242,15 @@  remap_vla_decls (tree block, struct nest
 	if (i == NULL)
 	  continue;
 
+	/* Fully expand value expressions.  This avoids having debug variables
+	   only referenced from them and that can be swept during GC.  */
+        if (slot)
+	  {
+	    tree t = (tree) *slot;
+	    gcc_assert (DECL_P (t) && DECL_HAS_VALUE_EXPR_P (t));
+	    val = build1 (INDIRECT_REF, TREE_TYPE (val), DECL_VALUE_EXPR (t));
+	  }
+
 	id.cb.src_fn = i->context;
 	id.cb.dst_fn = i->context;
 	id.cb.src_cfun = DECL_STRUCT_FUNCTION (root->context);
@@ -2242,13 +2259,13 @@  remap_vla_decls (tree block, struct nest
 	while (POINTER_TYPE_P (newt) && !TYPE_NAME (newt))
 	  {
 	    newt = TREE_TYPE (newt);
-	    t = TREE_TYPE (t);
+	    type = TREE_TYPE (type);
 	  }
 	if (TYPE_NAME (newt)
 	    && TREE_CODE (TYPE_NAME (newt)) == TYPE_DECL
 	    && DECL_ORIGINAL_TYPE (TYPE_NAME (newt))
-	    && newt != t
-	    && TYPE_NAME (newt) == TYPE_NAME (t))
+	    && newt != type
+	    && TYPE_NAME (newt) == TYPE_NAME (type))
 	  TYPE_NAME (newt) = remap_decl (TYPE_NAME (newt), &id.cb);
 
 	walk_tree (&val, copy_tree_body_r, &id.cb, NULL);
Index: config/bootstrap-lto.mk
===================================================================
--- config/bootstrap-lto.mk	(revision 167172)
+++ config/bootstrap-lto.mk	(working copy)
@@ -3,6 +3,3 @@ 
 
 STAGE2_CFLAGS += -flto=jobserver -fuse-linker-plugin -frandom-seed=1
 STAGE3_CFLAGS += -flto=jobserver -fuse-linker-plugin -frandom-seed=1
-
-# Ada fails to build with LTO, turn it off for now.
-BOOT_ADAFLAGS += -fno-lto