[PATCH] PR debug/53927: fix value for DW_AT_static_link

[PATCH] PR debug/53927: fix value for DW_AT_static_link

[Pierre-Marie de Rodat]

Hello,

This patch fixes the static link description in DWARF to comply with the 
specification. In order to do so, it appends a field to all FRAME 
objects to hold the frame base address (DW_AT_frame_base) so that the 
nested subrograms can directly reference this field.

See <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53927> for the context 
(in particular why we need this additional field in FRAME objects).

Bootstrapped and regtested successfuly on x86_64-linux. Ok for trunk? 
Thank you in advance!

gcc/ChangeLog:

         PR debug/53927
         * tree-nested.c (finalize_nesting_tree_1): Append a field to
         hold the frame base address.
         * dwarf2out.c (gen_subprogram_die): Generate for
         DW_AT_static_link a location description that computes the value
         of this field.

-- 
Pierre-Marie de Rodat

[PATCH, PING] PR debug/53927: fix value for DW_AT_static_link

[Pierre-Marie de Rodat]

On 07/20/2015 09:39 AM, Pierre-Marie de Rodat wrote:
> This patch fixes the static link description in DWARF to comply with the
> specification. In order to do so, it appends a field to all FRAME
> objects to hold the frame base address (DW_AT_frame_base) so that the
> nested subrograms can directly reference this field.
>
> See <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53927> for the context
> (in particular why we need this additional field in FRAME objects).

Ping for the patch submitted in 
<https://gcc.gnu.org/ml/gcc-patches/2015-07/msg01629.html>.

-- 
Pierre-Marie de Rodat

[PATCH, PING*2] PR debug/53927: fix value for DW_AT_static_link

[Pierre-Marie de Rodat]

On 07/20/2015 09:39 AM, Pierre-Marie de Rodat wrote:
> This patch fixes the static link description in DWARF to comply with the
> specification. In order to do so, it appends a field to all FRAME
> objects to hold the frame base address (DW_AT_frame_base) so that the
> nested subrograms can directly reference this field.
>
> See <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53927> for the context
> (in particular why we need this additional field in FRAME objects).

Ping for the patch submitted in
<https://gcc.gnu.org/ml/gcc-patches/2015-07/msg01629.html>. Thanks!

-- 
Pierre-Marie de Rodat

[PATCH, PING*3] PR debug/53927: fix value for DW_AT_static_link

[Pierre-Marie de Rodat]

On 07/20/2015 09:39 AM, Pierre-Marie de Rodat wrote:
> This patch fixes the static link description in DWARF to comply with the
> specification. In order to do so, it appends a field to all FRAME
> objects to hold the frame base address (DW_AT_frame_base) so that the
> nested subrograms can directly reference this field.
>
> See <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53927> for the context
> (in particular why we need this additional field in FRAME objects).

Ping for the patch submitted in 
<https://gcc.gnu.org/ml/gcc-patches/2015-07/msg01629.html>. Thanks!

-- 
Pierre-Marie de Rodat

[PATCH, PING*4] PR debug/53927: fix value for DW_AT_static_link

[Pierre-Marie de Rodat]

On 07/20/2015 09:39 AM, Pierre-Marie de Rodat wrote:
> This patch fixes the static link description in DWARF to comply with the
> specification. In order to do so, it appends a field to all FRAME
> objects to hold the frame base address (DW_AT_frame_base) so that the
> nested subrograms can directly reference this field.
>
> See <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53927> for the context
> (in particular why we need this additional field in FRAME objects).

Ping for the patch submitted in 
<https://gcc.gnu.org/ml/gcc-patches/2015-07/msg01629.html>.

-- 
Pierre-Marie de Rodat

Re: [PATCH, PING*4] PR debug/53927: fix value for DW_AT_static_link

[Pierre-Marie de Rodat]

[-- Attachment #1: Type: text/plain, Size: 401 bytes --]

On 08/31/2015 09:28 AM, Pierre-Marie de Rodat wrote:
> Ping for the patch submitted in
> <https://gcc.gnu.org/ml/gcc-patches/2015-07/msg01629.html>.

As Jason noticed in another thread, I forgot to attach the patch here 
(although it’s filed in the bug tracker). Here it is! Rebased against 
trunk, bootstrapped and regtested again on x86_64-linux.

Thanks in advance!

-- 
Pierre-Marie de Rodat

[-- Attachment #2: 0001-DWARF-fix-loc.-descr.-generation-for-DW_AT_static_li.patch --]
[-- Type: text/x-diff, Size: 5965 bytes --]

From d49b9ba6e40683a01c3fbf351c661a501c72ced1 Mon Sep 17 00:00:00 2001
From: Pierre-Marie de Rodat <derodat@adacore.com>
Date: Wed, 25 Feb 2015 14:48:24 +0100
Subject: [PATCH] DWARF: fix loc. descr. generation for DW_AT_static_link

gcc/ChangeLog:

	PR debug/53927
	* tree-nested.c (finalize_nesting_tree_1): Append a field to
	hold the frame base address.
	* dwarf2out.c (gen_subprogram_die): Generate for
	DW_AT_static_link a location description that computes the value
	of this field.
---
 gcc/dwarf2out.c   | 20 ++++++++++++---
 gcc/tree-nested.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++++------
 2 files changed, 83 insertions(+), 10 deletions(-)

diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index f184750..5249fca 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -19113,9 +19113,23 @@ gen_subprogram_die (tree decl, dw_die_ref context_die)
       compute_frame_pointer_to_fb_displacement (cfa_fb_offset);
 
       if (fun->static_chain_decl)
-	add_AT_location_description
-	  (subr_die, DW_AT_static_link,
-	   loc_list_from_tree (fun->static_chain_decl, 2, NULL));
+	{
+	  /* DWARF requires here a location expression that computes the
+	     address of the enclosing subprogram's frame base.  The machinery
+	     in tree-nested.c is supposed to store this specific address in the
+	     last field of the FRAME record.  */
+	  const tree frame_type
+	    = TREE_TYPE (TREE_TYPE (fun->static_chain_decl));
+	  const tree fb_decl = tree_last (TYPE_FIELDS (frame_type));
+
+	  tree fb_expr
+	    = build1 (INDIRECT_REF, frame_type, fun->static_chain_decl);
+	  fb_expr = build3 (COMPONENT_REF, TREE_TYPE (fb_decl),
+			    fb_expr, fb_decl, NULL_TREE);
+
+	  add_AT_location_description (subr_die, DW_AT_static_link,
+				       loc_list_from_tree (fb_expr, 0, NULL));
+	}
     }
 
   /* Generate child dies for template paramaters.  */
diff --git a/gcc/tree-nested.c b/gcc/tree-nested.c
index 1f6311c..06484a2 100644
--- a/gcc/tree-nested.c
+++ b/gcc/tree-nested.c
@@ -2718,10 +2718,10 @@ fold_mem_refs (tree *const &e, void *data ATTRIBUTE_UNUSED)
   return true;
 }
 
-/* Do "everything else" to clean up or complete state collected by the
-   various walking passes -- lay out the types and decls, generate code
-   to initialize the frame decl, store critical expressions in the
-   struct function for rtl to find.  */
+/* Do "everything else" to clean up or complete state collected by the various
+   walking passes -- create a field to hold the frame base address, lay out the
+   types and decls, generate code to initialize the frame decl, store critical
+   expressions in the struct function for rtl to find.  */
 
 static void
 finalize_nesting_tree_1 (struct nesting_info *root)
@@ -2737,16 +2737,75 @@ finalize_nesting_tree_1 (struct nesting_info *root)
      out at this time.  */
   if (root->frame_type)
     {
+      int save_warn_padded;
+      tree *adjust, *field_decl_p;
+      char *name;
+      tree fb_decl, fb_ref, fb_tmp;
+      gcall *fb_gimple;
+      gimple_stmt_iterator gsi;
+
+      /* Debugging information needs to compute the frame base address of the
+	 nestee frame out of the static chain from the nested frame.
+
+	 The static chain is the address of the FRAME record, so one could
+	 imagine it would be possible to compute the frame base address just
+	 adding a constant offset to this address.  Unfortunately, this is not
+	 possible: if the FRAME object has alignment constraints that are
+	 stronger than the stack, then the offset between the frame base and
+	 the FRAME object will be dynamic.
+
+	 What we do instead is to append a field to the FRAME object that holds
+	 the frame base address: then debug. info. just has to fetch this
+	 field.  */
+
+      /* Debugging information will refer to the CFA as the frame base
+	 address: we will do the same here.  */
+      const tree frame_addr_fndecl
+        = builtin_decl_explicit (BUILT_IN_DWARF_CFA);
+
+      /* Create a field in the FRAME record to hold the frame base address for
+	 this stack frame.  Since it will be used only by the debugger (through
+	 the debugging information), put it at the end of the record not to
+	 shift all other offsets.  */
+      fb_decl = make_node (FIELD_DECL);
+      name = concat ("FRAME_BASE.",
+		     IDENTIFIER_POINTER (DECL_NAME (root->context)),
+		     NULL);
+      DECL_NAME (fb_decl) = get_identifier (name);
+      free (name);
+      TREE_TYPE (fb_decl) = ptr_type_node;
+      TREE_ADDRESSABLE (fb_decl) = 1;
+      DECL_CONTEXT (fb_decl) = root->frame_type;
+      field_decl_p = &TYPE_FIELDS (root->frame_type);
+      while (*field_decl_p != NULL_TREE)
+	field_decl_p = &DECL_CHAIN (*field_decl_p);
+      *field_decl_p = fb_decl;
+
       /* In some cases the frame type will trigger the -Wpadded warning.
 	 This is not helpful; suppress it. */
-      int save_warn_padded = warn_padded;
-      tree *adjust;
-
+      save_warn_padded = warn_padded;
       warn_padded = 0;
       layout_type (root->frame_type);
       warn_padded = save_warn_padded;
       layout_decl (root->frame_decl, 0);
 
+      /* Initialize the frame base address field.  If the builtin we need is
+	 not available, set it to NULL so that debugging information does not
+	 reference junk.  */
+      fb_ref = build3 (COMPONENT_REF, TREE_TYPE (fb_decl),
+		       root->frame_decl, fb_decl, NULL_TREE);
+      if (frame_addr_fndecl != NULL_TREE)
+	{
+	  fb_gimple = gimple_build_call (frame_addr_fndecl, 1,
+					 integer_zero_node);
+	  gsi = gsi_last (stmt_list);
+	  fb_tmp = init_tmp_var_with_call (root, &gsi, fb_gimple);
+	}
+      else
+	fb_tmp = build_int_cst (TREE_TYPE (fb_ref), 0);
+      gimple_seq_add_stmt (&stmt_list,
+			   gimple_build_assign (fb_ref, fb_tmp));
+
       /* Remove root->frame_decl from root->new_local_var_chain, so
 	 that we can declare it also in the lexical blocks, which
 	 helps ensure virtual regs that end up appearing in its RTL
-- 
2.3.3.199.g52cae64

Re: [PATCH, PING*4] PR debug/53927: fix value for DW_AT_static_link

[Jason Merrill]

The DWARF change is OK if the tree-nested.c changes make sense to Eric.

Jason

Re: [PATCH, PING*4] PR debug/53927: fix value for DW_AT_static_link

[Eric Botcazou]

> The DWARF change is OK if the tree-nested.c changes make sense to Eric.

As discussed in the audit trail of the PR, I'd rather have avoided the 
additional indirection, but the DWARF requirement seems to force it if we base 
the computation on the static chain and I don't see another approach.

A few comments:

+      int save_warn_padded;
+      tree *adjust, *field_decl_p;
+      char *name;
+      tree fb_decl, fb_ref, fb_tmp;
+      gcall *fb_gimple;
+      gimple_stmt_iterator gsi;

We try to declare variables only at the first use point now I think.

+      /* Debugging information needs to compute the frame base address of the
+	 nestee frame out of the static chain from the nested frame.

"parent frame"

+	 The static chain is the address of the FRAME record, so one could
+	 imagine it would be possible to compute the frame base address just
+	 adding a constant offset to this address.  Unfortunately, this is not
+	 possible: if the FRAME object has alignment constraints that are
+	 stronger than the stack, then the offset between the frame base and
+	 the FRAME object will be dynamic.
+
+	 What we do instead is to append a field to the FRAME object that 
holds
+	 the frame base address: then debug. info. just has to fetch this
+	 field.  */

No useless period: "debug info"

+      /* Debugging information will refer to the CFA as the frame base
+	 address: we will do the same here.  */
+      const tree frame_addr_fndecl
+        = builtin_decl_explicit (BUILT_IN_DWARF_CFA);
+
+      /* Create a field in the FRAME record to hold the frame base address 
for
+	 this stack frame.  Since it will be used only by the debugger 
(through
+	 the debugging information), put it at the end of the record not to
+	 shift all other offsets.  */

"(through the debugging information)" sounds superfluous. "in order not to..."

+      fb_decl = make_node (FIELD_DECL);
+      name = concat ("FRAME_BASE.",
+		     IDENTIFIER_POINTER (DECL_NAME (root->context)),
+		     NULL);
+      DECL_NAME (fb_decl) = get_identifier (name);
+      free (name);

Let's avoid this concat/free business and use a simpler name.

+      TREE_TYPE (fb_decl) = ptr_type_node;
+      TREE_ADDRESSABLE (fb_decl) = 1;
+      DECL_CONTEXT (fb_decl) = root->frame_type;
+      field_decl_p = &TYPE_FIELDS (root->frame_type);
+      while (*field_decl_p != NULL_TREE)
+	field_decl_p = &DECL_CHAIN (*field_decl_p);
+      *field_decl_p = fb_decl;

TYPE_FIELDS (root->frame_type)
  = chainon (TYPE_FIELDS (root->frame_type), fb_decl);

-- 
Eric Botcazou

Re: [PATCH, PING*4] PR debug/53927: fix value for DW_AT_static_link

[Pierre-Marie de Rodat]

[-- Attachment #1: Type: text/plain, Size: 1105 bytes --]

Thank you Jason!

On 11/25/2015 09:35 PM, Eric Botcazou wrote:
> We try to declare variables only at the first use point now I think.

Fixed, thanks!

> +      /* Debugging information needs to compute the frame base address of the
> +	 nestee frame out of the static chain from the nested frame.
>
> "parent frame"

Just to make sure: instead of “nestee”: fixed.

> No useless period: "debug info"

Fixed.

> "(through the debugging information)" sounds superfluous. "in order not to..."

Done.

> +      fb_decl = make_node (FIELD_DECL);
> +      name = concat ("FRAME_BASE.",
> +		     IDENTIFIER_POINTER (DECL_NAME (root->context)),
> +		     NULL);
> +      DECL_NAME (fb_decl) = get_identifier (name);
> +      free (name);
>
> Let's avoid this concat/free business and use a simpler name.

Right, I put instead:

     DECL_NAME (fb_decl) = get_identifier ("FRAME_BASE.PARENT");

> TYPE_FIELDS (root->frame_type)
>    = chainon (TYPE_FIELDS (root->frame_type), fb_decl);

Much better, thanks. :-)

Here’s the updated patch. Regtested again on x86_64-linux.

-- 
Pierre-Marie de Rodat

[-- Attachment #2: 0001-DWARF-fix-loc.-descr.-generation-for-DW_AT_static_li.patch --]
[-- Type: text/x-diff, Size: 5784 bytes --]

From 3f067709d86fea108ef4debcc7f9b39cde91e644 Mon Sep 17 00:00:00 2001
From: Pierre-Marie de Rodat <derodat@adacore.com>
Date: Wed, 25 Feb 2015 14:48:24 +0100
Subject: [PATCH] DWARF: fix loc. descr. generation for DW_AT_static_link

gcc/ChangeLog:

	PR debug/53927
	* tree-nested.c (finalize_nesting_tree_1): Append a field to
	hold the frame base address.
	* dwarf2out.c (gen_subprogram_die): Generate for
	DW_AT_static_link a location description that computes the value
	of this field.
---
 gcc/dwarf2out.c   | 20 +++++++++++++++---
 gcc/tree-nested.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++------
 2 files changed, 73 insertions(+), 9 deletions(-)

diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index f184750..5249fca 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -19113,9 +19113,23 @@ gen_subprogram_die (tree decl, dw_die_ref context_die)
       compute_frame_pointer_to_fb_displacement (cfa_fb_offset);
 
       if (fun->static_chain_decl)
-	add_AT_location_description
-	  (subr_die, DW_AT_static_link,
-	   loc_list_from_tree (fun->static_chain_decl, 2, NULL));
+	{
+	  /* DWARF requires here a location expression that computes the
+	     address of the enclosing subprogram's frame base.  The machinery
+	     in tree-nested.c is supposed to store this specific address in the
+	     last field of the FRAME record.  */
+	  const tree frame_type
+	    = TREE_TYPE (TREE_TYPE (fun->static_chain_decl));
+	  const tree fb_decl = tree_last (TYPE_FIELDS (frame_type));
+
+	  tree fb_expr
+	    = build1 (INDIRECT_REF, frame_type, fun->static_chain_decl);
+	  fb_expr = build3 (COMPONENT_REF, TREE_TYPE (fb_decl),
+			    fb_expr, fb_decl, NULL_TREE);
+
+	  add_AT_location_description (subr_die, DW_AT_static_link,
+				       loc_list_from_tree (fb_expr, 0, NULL));
+	}
     }
 
   /* Generate child dies for template paramaters.  */
diff --git a/gcc/tree-nested.c b/gcc/tree-nested.c
index 1f6311c..cf6b0d8 100644
--- a/gcc/tree-nested.c
+++ b/gcc/tree-nested.c
@@ -2718,10 +2718,10 @@ fold_mem_refs (tree *const &e, void *data ATTRIBUTE_UNUSED)
   return true;
 }
 
-/* Do "everything else" to clean up or complete state collected by the
-   various walking passes -- lay out the types and decls, generate code
-   to initialize the frame decl, store critical expressions in the
-   struct function for rtl to find.  */
+/* Do "everything else" to clean up or complete state collected by the various
+   walking passes -- create a field to hold the frame base address, lay out the
+   types and decls, generate code to initialize the frame decl, store critical
+   expressions in the struct function for rtl to find.  */
 
 static void
 finalize_nesting_tree_1 (struct nesting_info *root)
@@ -2737,20 +2737,70 @@ finalize_nesting_tree_1 (struct nesting_info *root)
      out at this time.  */
   if (root->frame_type)
     {
+      /* Debugging information needs to compute the frame base address of the
+	 parent frame out of the static chain from the nested frame.
+
+	 The static chain is the address of the FRAME record, so one could
+	 imagine it would be possible to compute the frame base address just
+	 adding a constant offset to this address.  Unfortunately, this is not
+	 possible: if the FRAME object has alignment constraints that are
+	 stronger than the stack, then the offset between the frame base and
+	 the FRAME object will be dynamic.
+
+	 What we do instead is to append a field to the FRAME object that holds
+	 the frame base address: then debug info just has to fetch this
+	 field.  */
+
+      /* Debugging information will refer to the CFA as the frame base
+	 address: we will do the same here.  */
+      const tree frame_addr_fndecl
+        = builtin_decl_explicit (BUILT_IN_DWARF_CFA);
+
+      /* Create a field in the FRAME record to hold the frame base address for
+	 this stack frame.  Since it will be used only by the debugger, put it
+	 at the end of the record not to shift all other offsets.  */
+      tree fb_decl = make_node (FIELD_DECL);
+
+      DECL_NAME (fb_decl) = get_identifier ("FRAME_BASE.PARENT");
+      TREE_TYPE (fb_decl) = ptr_type_node;
+      TREE_ADDRESSABLE (fb_decl) = 1;
+      DECL_CONTEXT (fb_decl) = root->frame_type;
+      TYPE_FIELDS (root->frame_type) = chainon (TYPE_FIELDS (root->frame_type),
+						fb_decl);
+
       /* In some cases the frame type will trigger the -Wpadded warning.
 	 This is not helpful; suppress it. */
       int save_warn_padded = warn_padded;
-      tree *adjust;
-
       warn_padded = 0;
       layout_type (root->frame_type);
       warn_padded = save_warn_padded;
       layout_decl (root->frame_decl, 0);
 
+      /* Initialize the frame base address field.  If the builtin we need is
+	 not available, set it to NULL so that debugging information does not
+	 reference junk.  */
+      tree fb_ref = build3 (COMPONENT_REF, TREE_TYPE (fb_decl),
+			    root->frame_decl, fb_decl, NULL_TREE);
+      tree fb_tmp;
+
+      if (frame_addr_fndecl != NULL_TREE)
+	{
+	  gcall *fb_gimple = gimple_build_call (frame_addr_fndecl, 1,
+						integer_zero_node);
+	  gimple_stmt_iterator gsi = gsi_last (stmt_list);
+
+	  fb_tmp = init_tmp_var_with_call (root, &gsi, fb_gimple);
+	}
+      else
+	fb_tmp = build_int_cst (TREE_TYPE (fb_ref), 0);
+      gimple_seq_add_stmt (&stmt_list,
+			   gimple_build_assign (fb_ref, fb_tmp));
+
       /* Remove root->frame_decl from root->new_local_var_chain, so
 	 that we can declare it also in the lexical blocks, which
 	 helps ensure virtual regs that end up appearing in its RTL
 	 expression get substituted in instantiate_virtual_regs().  */
+      tree *adjust;
       for (adjust = &root->new_local_var_chain;
 	   *adjust != root->frame_decl;
 	   adjust = &DECL_CHAIN (*adjust))
-- 
2.3.3.199.g52cae64

Re: [PATCH, PING*4] PR debug/53927: fix value for DW_AT_static_link

[Eric Botcazou]

> Here’s the updated patch. Regtested again on x86_64-linux.

Thanks, it looks good as far as I'm concerned (modulo the missing "in order" 
before "not to shift all other offsets" in a comment).

-- 
Eric Botcazou

Re: [PATCH, PING*4] PR debug/53927: fix value for DW_AT_static_link

[Pierre-Marie de Rodat]

On 11/26/2015 03:50 PM, Eric Botcazou wrote:
>> Here’s the updated patch. Regtested again on x86_64-linux.
>
> Thanks, it looks good as far as I'm concerned (modulo the missing "in order"
> before "not to shift all other offsets" in a comment).

Fixed and pushed. Thank you again for reviewing!

-- 
Pierre-Marie de Rodat