Re: [PATCH] [gdb/testsuite] Use function_range in gdb.dwarf2/dw2-ref-missing-frame.exp

[Tom de Vries <tdevries@suse.de>]

[ In order to reply to this email, I needed to:
- press reply-to, remove unusable patch content
- open attachment in editor
- select all, copy
- paste as quoted

Could you check out
https://sourceware.org/gdb/wiki/ContributionChecklist#Submitting_patches
to see if you can improve your patch submission?

The inline version of the patch suffers from the issues mentioned there.

My guess is that not sending email containing html parts will allready
improve reviewer experience. ]

On 10/4/21 9:16 AM, Kumar N, Bhuvanendra wrote:


> From fc265d5b74c517c2781fb6cf0f67c28b37fecc60 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?=E2=80=9Cbhkumarn=E2=80=9D?= <Bhuvanendra.KumarN@amd.com>
> Date: Mon, 4 Oct 2021 12:21:19 +0530
> Subject: [PATCH] [gdb/testsuite] Use function_range in
>  gdb.dwarf2/dw2-ref-missing-frame.exp
> 
> Following 2 test points are failing with clang compiler
> 
> (gdb) FAIL: gdb.dwarf2/dw2-ref-missing-frame.exp: func_nofb print
> (gdb) FAIL: gdb.dwarf2/dw2-ref-missing-frame.exp: func_loopfb print
> 
> The problem is that the CU and functions have an empty address range:
> ...
> 0x0000000b: DW_TAG_compile_unit [1] *
>               DW_AT_high_pc [DW_FORM_addr]      (0x002016c0)
>               DW_AT_low_pc [DW_FORM_addr]       (0x002016c0)
>               DW_AT_name [DW_FORM_string]       ("file1.txt")
> 
> 0x00000032:   DW_TAG_subprogram [5] *
>                 DW_AT_name [DW_FORM_string]     ("func_nofb")
>                 DW_AT_low_pc [DW_FORM_addr]     (0x002016c0)
>                 DW_AT_high_pc [DW_FORM_addr]    (0x002016c0)
> 
> 0x0000005c:   DW_TAG_subprogram [6] *
>                 DW_AT_name [DW_FORM_string]     ("func_loopfb")
>                 DW_AT_low_pc [DW_FORM_addr]     (0x002016c0)
>                 DW_AT_high_pc [DW_FORM_addr]    (0x002016c0)
> ...
> 
> The address ranges are set like this in dw2-ref-missing-frame.S:
> ...
>         .4byte  cu_text_end                     /* DW_AT_high_pc */
>         .4byte  cu_text_start                   /* DW_AT_low_pc */
> 
>         .4byte          func_nofb_start         /* DW_AT_low_pc */
>         .4byte          func_nofb_end           /* DW_AT_high_pc */
> 
>         .4byte          func_loopfb_start       /* DW_AT_low_pc */
>         .4byte          func_loopfb_end         /* DW_AT_high_pc */
> ...
> 
> where the labels refer to dw2-ref-missing-frame-func.c:
> ...
> asm (".globl cu_text_start");
> asm ("cu_text_start:");
> 
> asm (".globl func_nofb_start");
> asm (".p2align 4");
> asm ("func_nofb_start:");
> 
> void
> func_nofb (void)
> {
>   /* int func_nofb_var; */
> }
> 
> asm (".globl func_nofb_end");
> asm ("func_nofb_end:");
> 
> asm (".globl func_loopfb_start");
> asm (".p2align 4");
> asm ("func_loopfb_start:");
> 
> void
> func_loopfb (void)
> {
>   /* int func_loopfb_var; */
> }
> 
> asm (".globl func_loopfb_end");
> asm ("func_loopfb_end:");
> 
> asm (".globl cu_text_end");
> asm ("cu_text_end:");
> ...
> 
> Using asm labels in global scope is a known source of problems, as explained
> in the comment of proc function_range in gdb/testsuite/lib/dwarf.exp.
> 

This analysis is good, thanks.

[ If you would feel so inclined, it could be shortened by referring to
the other commit, along the lines of:
...
As in commit f677852bbda "[gdb/testsuite] Use function_range in
gdb.dwarf2/dw2-abs-hi-pc.exp", the problem is that the CU and functions
have an empty address range, due to using asm labels in global scope,
which is a known source of problems, as explained in the comment of proc
function_range in gdb/testsuite/lib/dwarf.exp.
... ]

> Fix this by using function_range instead.
> 
> Tested on x86_64-linux with gcc and clang.
> ---
>  .../gdb.dwarf2/dw2-ref-missing-frame-func.c   | 22 ++-----------
>  .../gdb.dwarf2/dw2-ref-missing-frame.S        | 12 +++----
>  .../gdb.dwarf2/dw2-ref-missing-frame.exp      | 33 +++++++++++++++----
>  3 files changed, 35 insertions(+), 32 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.dwarf2/dw2-ref-missing-frame-func.c b/gdb/testsuite/gdb.dwarf2/dw2-ref-missing-frame-func.c
> index 1ec1897b9e9..90772d65993 100644
> --- a/gdb/testsuite/gdb.dwarf2/dw2-ref-missing-frame-func.c
> +++ b/gdb/testsuite/gdb.dwarf2/dw2-ref-missing-frame-func.c
> @@ -15,34 +15,16 @@
>     You should have received a copy of the GNU General Public License
>     along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>  
> -asm (".globl cu_text_start");
> -asm ("cu_text_start:");
> -
> -asm (".globl func_nofb_start");
> -asm (".p2align 4");
> -asm ("func_nofb_start:");
> -
>  void
>  func_nofb (void)
>  {
> +asm ("func_nofb_label: .globl func_nofb_label\n");
>    /* int func_nofb_var; */
>  }
>  
> -asm (".globl func_nofb_end");
> -asm ("func_nofb_end:");
> -
> -asm (".globl func_loopfb_start");
> -asm (".p2align 4");
> -asm ("func_loopfb_start:");
> -
>  void
>  func_loopfb (void)
>  {
> +asm ("func_loopfb_label: .globl func_loopfb_label\n");
>    /* int func_loopfb_var; */
>  }
> -
> -asm (".globl func_loopfb_end");
> -asm ("func_loopfb_end:");
> -
> -asm (".globl cu_text_end");
> -asm ("cu_text_end:");
> diff --git a/gdb/testsuite/gdb.dwarf2/dw2-ref-missing-frame.S b/gdb/testsuite/gdb.dwarf2/dw2-ref-missing-frame.S
> index a64f03b0443..293eb0eca43 100644
> --- a/gdb/testsuite/gdb.dwarf2/dw2-ref-missing-frame.S
> +++ b/gdb/testsuite/gdb.dwarf2/dw2-ref-missing-frame.S
> @@ -28,8 +28,8 @@
>  
>  	/* CU die */
>  	.uleb128 1				/* Abbrev: DW_TAG_compile_unit */
> -	.4byte	cu_text_end			/* DW_AT_high_pc */
> -	.4byte	cu_text_start			/* DW_AT_low_pc */
> +	.4byte	FUNC_LOOPFB_END			/* DW_AT_high_pc */
> +	.4byte	FUNC_NOFB_START			/* DW_AT_low_pc */
>  	.ascii	"file1.txt\0"			/* DW_AT_name */
>  	.ascii	"GNU C 3.3.3\0"			/* DW_AT_producer */
>  	.byte	1				/* DW_AT_language (C) */
> @@ -43,8 +43,8 @@
>  	/* func_nofb */
>  	.uleb128	5			/* Abbrev: DW_TAG_subprogram (no fb) */
>  	.ascii		"func_nofb\0"		/* DW_AT_name */
> -	.4byte		func_nofb_start		/* DW_AT_low_pc */
> -	.4byte		func_nofb_end		/* DW_AT_high_pc */
> +	.4byte          FUNC_NOFB_START         /* DW_AT_low_pc */
> +	.4byte          FUNC_NOFB_END           /* DW_AT_high_pc */
>  
>  	.uleb128	7			/* Abbrev: DW_TAG_variable (location) */
>  	.ascii		"func_nofb_var\0"	/* DW_AT_name */
> @@ -58,8 +58,8 @@
>  	/* func_loopfb */
>  	.uleb128	6			/* Abbrev: DW_TAG_subprogram (loop fb) */
>  	.ascii		"func_loopfb\0"		/* DW_AT_name */
> -	.4byte		func_loopfb_start	/* DW_AT_low_pc */
> -	.4byte		func_loopfb_end		/* DW_AT_high_pc */
> +	.4byte		FUNC_LOOPFB_START	/* DW_AT_low_pc */
> +	.4byte		FUNC_LOOPFB_END		/* DW_AT_high_pc */
>  	.byte		2f - 1f			/* DW_AT_frame_base */
>  1:	.byte		0x91			/*   DW_OP_fbreg */
>  	.sleb128	0			/*   0 */
> diff --git a/gdb/testsuite/gdb.dwarf2/dw2-ref-missing-frame.exp b/gdb/testsuite/gdb.dwarf2/dw2-ref-missing-frame.exp
> index 1b871445cc3..c45a99b3efa 100644
> --- a/gdb/testsuite/gdb.dwarf2/dw2-ref-missing-frame.exp
> +++ b/gdb/testsuite/gdb.dwarf2/dw2-ref-missing-frame.exp
> @@ -19,17 +19,38 @@ if {![dwarf2_support]} {
>      return 0  
>  }
>  
> -standard_testfile .S
> -set srcfuncfile ${testfile}-func.c
> -set srcmainfile ${testfile}-main.c
> +standard_testfile

Using standard_testfile sets $srcfile to dw2-ref-missing-frame.c, which
is not there.

Please using instead:
...
standard_testfile .S -func.c -main.c
...
and refer to the files using $srcfile, $srcfile2, $srcfile3.

> +set sources \
> +    [list \
> +         ${testfile}-main.c \
> +         ${testfile}-func.c]
> +set sources [lmap i $sources { string cat "${srcdir}/${subdir}/" $i }]
> +lassign [function_range func_nofb $sources] \
> +    func_nofb_start func_nofb_len
> +lassign [function_range func_loopfb $sources] \
> +    func_loopfb_start func_loopfb_len
> +
> +set sources \
> +    [list \
> +         ${testfile}-main.c \
> +         ${testfile}.S \
> +         ${testfile}-func.c]
> +set flags \
> +    [list \
> +         "debug" \
> +         "additional_flags=\"-DFUNC_NOFB_START=$func_nofb_start\"" \
> +         "additional_flags=\"-DFUNC_NOFB_END=$func_nofb_start + $func_nofb_len\"" \
> +         "additional_flags=\"-DFUNC_LOOPFB_START=$func_loopfb_start\"" \
> +         "additional_flags=\"-DFUNC_LOOPFB_END=$func_loopfb_start + $func_loopfb_len\""]
>  set executable ${testfile}
>  
> -if {[prepare_for_testing_full "failed to prepare" \
> -	 [list $testfile {} $srcfile {} $srcfuncfile {} \
> -	      $srcmainfile debug]]} {
> +if {[build_executable ${testfile}.exp ${executable} $sources $flags] == -1} {
> +
>      return -1
>  }
>  
> +clean_restart $executable
> +

We prefer using prepare_for_testing instead of build_executable +
clean_restart.

This works for me:
...
if {[prepare_for_testing "failed to prepare" $testfile $sources \
         $flags] == -1} {
    return -1
}

With those nits fixed, LGTM.

Thanks,
- Tom

>  # First try referencing DW_AT_frame_base which is not defined.
>  if [runto func_nofb] {
>      gdb_test "p func_nofb_var" {Could not find the frame base for "func_nofb".} "func_nofb print"
> -- 
> 2.17.1
>