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

  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(-)
  

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
  

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