[v2,5/7] Fortran: Enable setting breakpoint on nested functions.

Message ID 1542663530-140490-5-git-send-email-pwodkowski@pl.sii.eu
State New
Headers show
Series
  • [v2,1/7] DWARF: Don't add nameless modules to partial symbol table.
Related show

Commit Message

Paweł Wódkowski Nov. 19, 2018, 9:38 p.m.
From: Bernhard Heckel <bernhard.heckel@intel.com>


Like in Ada, we want to be able to set a breakpoint on
nested functions, called "contained routines" in Fortran.

2016-06-30  Bernhard Heckel  <bernhard.heckel@intel.com>

gdb/Changelog:
	* dwarf2read.c (add_partial_symbol): store fortran subprograms
	in global scope.
	(add_partial_subprogram): Save fortran DW_TAG_subprogram.

gdb/Changelog:
	* gdb.fortran/nested-funcs.exp: Set breakpoint on contained routines.

Changes from V1 to V2:
	Fill commit message gaps.
---
 gdb/dwarf2read.c                           | 9 +++++++--
 gdb/testsuite/gdb.fortran/nested-funcs.exp | 7 +++++++
 2 files changed, 14 insertions(+), 2 deletions(-)

-- 
2.7.4

Comments

Richard Bunt Nov. 28, 2018, 10:31 a.m. | #1
Hi Pawel,

Thanks for this fix, I have a local patch for this so I'd be pleased to see an upstream variant.

Note: I cannot approve GDB patches but I thought I'd contribute a review since I was in this part
of the code recently.

On 11/19/18 9:38 PM, Pawel Wodkowski wrote:
> +# Test if we can set a breakpoint in a nested function

> +gdb_breakpoint "sub_nested_outer"

> +gdb_continue_to_breakpoint "sub_nested_outer" ".*local_int = 19"

>  

>  # Test if we can access local and

>  # non-local variables defined one level up.

> @@ -43,6 +46,10 @@ gdb_test "print local_int" "= 19" "print local_int in outer function"

>  gdb_test "up"

>  gdb_test "print index" "= 42" "print index at BP1, one frame up"

>  

> +# Test if we can set a breakpoint in a nested function

> +gdb_breakpoint "sub_nested_inner"

> +gdb_continue_to_breakpoint "sub_nested_inner" ".*local_int = 17"

> +

>  # Test if we can access local and

>  # non-local variables defined two level up.

>  gdb_breakpoint [gdb_get_line_number "! BP_inner"]

> 


This patch passed my local test case for this fix, so it looks good to me. The only test case that
I have locally that I think would be of value here, would be to test that breakpoints can be set on 
multiple functions in the same contains block (i.e. both sub_nested_outer and sub_nested_inner),
prior to program start. I found that such a test gives coverage to the changes in 
add_partial_subprogram as it tests the logic for subprograms which are linked as siblings.

What do you think?

>            /* brobecker/2007-12-26: Normally, only "external" DIEs are part

>               of the global scope.  But in Ada, we want to be able to access

> @@ -9206,6 +9208,8 @@ add_partial_subprogram (struct partial_die_info *pdi,

>  	{

>  	  if (pdi->tag == DW_TAG_entry_point)

>  	    add_partial_entry_point (pdi, lowpc, highpc, set_addrmap, cu);

> +	  else if (pdi->tag == DW_TAG_subprogram)

> +	    add_partial_subprogram (pdi, lowpc, highpc, set_addrmap, cu);

>  	  pdi = pdi->die_sibling;

>  	}

>      }



Many thanks,

Rich
Paweł Wódkowski Dec. 2, 2018, 10:16 p.m. | #2
Hi Richard,

Please see my comments.

On 28.11.2018 11:30, Richard Bunt wrote:
> Hi Pawel,

> 

> Thanks for this fix, I have a local patch for this so I'd be pleased to see an upstream variant.

> 

> Note: I cannot approve GDB patches but I thought I'd contribute a review since I was in this part

> of the code recently.

> 


Any review is great, especially from someone else developping for 
fortran these days ;)

> On 11/19/18 9:38 PM, Pawel Wodkowski wrote:

>> +# Test if we can set a breakpoint in a nested function

>> +gdb_breakpoint "sub_nested_outer"

>> +gdb_continue_to_breakpoint "sub_nested_outer" ".*local_int = 19"

>>   

>>   # Test if we can access local and

>>   # non-local variables defined one level up.

>> @@ -43,6 +46,10 @@ gdb_test "print local_int" "= 19" "print local_int in outer function"

>>   gdb_test "up"

>>   gdb_test "print index" "= 42" "print index at BP1, one frame up"

>>   

>> +# Test if we can set a breakpoint in a nested function

>> +gdb_breakpoint "sub_nested_inner"

>> +gdb_continue_to_breakpoint "sub_nested_inner" ".*local_int = 17"

>> +

>>   # Test if we can access local and

>>   # non-local variables defined two level up.

>>   gdb_breakpoint [gdb_get_line_number "! BP_inner"]

>>

> 

> This patch passed my local test case for this fix, so it looks good to me. The only test case that

> I have locally that I think would be of value here, would be to test that breakpoints can be set on

> multiple functions in the same contains block (i.e. both sub_nested_outer and sub_nested_inner),

> prior to program start. I found that such a test gives coverage to the changes in

> add_partial_subprogram as it tests the logic for subprograms which are linked as siblings.

> 

> What do you think?

> 


Yes, setting breakpoint before starting program is something that should 
work for sure so I checked it. I'm happy to say that it is working for 
both functions when set before MAIN__ :)
I will change the tests.


>>             /* brobecker/2007-12-26: Normally, only "external" DIEs are part

>>                of the global scope.  But in Ada, we want to be able to access

>> @@ -9206,6 +9208,8 @@ add_partial_subprogram (struct partial_die_info *pdi,

>>   	{

>>   	  if (pdi->tag == DW_TAG_entry_point)

>>   	    add_partial_entry_point (pdi, lowpc, highpc, set_addrmap, cu);

>> +	  else if (pdi->tag == DW_TAG_subprogram)

>> +	    add_partial_subprogram (pdi, lowpc, highpc, set_addrmap, cu);

>>   	  pdi = pdi->die_sibling;

>>   	}

>>       }

> 

> 

> Many thanks,

> 

> Rich

> 


Pawel

Patch

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 88e57d7ab68e..dcf9da8372df 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -8910,7 +8910,9 @@  add_partial_symbol (struct partial_die_info *pdi, struct dwarf2_cu *cu)
     case DW_TAG_subprogram:
       addr = (gdbarch_adjust_dwarf2_addr (gdbarch, pdi->lowpc + baseaddr)
 	      - baseaddr);
-      if (pdi->is_external || cu->language == language_ada)
+      if (pdi->is_external
+	  || cu->language == language_ada
+	  || cu->language == language_fortran)
 	{
           /* brobecker/2007-12-26: Normally, only "external" DIEs are part
              of the global scope.  But in Ada, we want to be able to access
@@ -9206,6 +9208,8 @@  add_partial_subprogram (struct partial_die_info *pdi,
 	{
 	  if (pdi->tag == DW_TAG_entry_point)
 	    add_partial_entry_point (pdi, lowpc, highpc, set_addrmap, cu);
+	  else if (pdi->tag == DW_TAG_subprogram)
+	    add_partial_subprogram (pdi, lowpc, highpc, set_addrmap, cu);
 	  pdi = pdi->die_sibling;
 	}
     }
@@ -21543,7 +21547,8 @@  new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
 	  SYMBOL_ACLASS_INDEX (sym) = LOC_BLOCK;
 	  attr2 = dwarf2_attr (die, DW_AT_external, cu);
 	  if ((attr2 && (DW_UNSND (attr2) != 0))
-              || cu->language == language_ada)
+	      || cu->language == language_ada
+	      || cu->language == language_fortran)
 	    {
               /* Subprograms marked external are stored as a global symbol.
                  Ada subprograms, whether marked external or not, are always
diff --git a/gdb/testsuite/gdb.fortran/nested-funcs.exp b/gdb/testsuite/gdb.fortran/nested-funcs.exp
index 2481d256af69..5e28b0e1a3e2 100755
--- a/gdb/testsuite/gdb.fortran/nested-funcs.exp
+++ b/gdb/testsuite/gdb.fortran/nested-funcs.exp
@@ -29,6 +29,9 @@  if ![runto MAIN__] then {
     perror "couldn't run to breakpoint MAIN__"
     continue
 }
+# Test if we can set a breakpoint in a nested function
+gdb_breakpoint "sub_nested_outer"
+gdb_continue_to_breakpoint "sub_nested_outer" ".*local_int = 19"
 
 # Test if we can access local and
 # non-local variables defined one level up.
@@ -43,6 +46,10 @@  gdb_test "print local_int" "= 19" "print local_int in outer function"
 gdb_test "up"
 gdb_test "print index" "= 42" "print index at BP1, one frame up"
 
+# Test if we can set a breakpoint in a nested function
+gdb_breakpoint "sub_nested_inner"
+gdb_continue_to_breakpoint "sub_nested_inner" ".*local_int = 17"
+
 # Test if we can access local and
 # non-local variables defined two level up.
 gdb_breakpoint [gdb_get_line_number "! BP_inner"]