[PATCH][gdb/testsuite] Add label option to proc cu

[PATCH][gdb/testsuite] Add label option to proc cu

[Tom de Vries]

Hi,

We can use current dwarf assembly infrastructure to declare a label that marks
the start of the CU header:
...
  declare_labels header_start_cu_a
  _section ".debug_info"
  header_start_cu_a : cu {} {
  }
  _section ".debug_info"
  header_start_cu_b : cu {} {
  }
...
on the condition that we switch to the .debug_info section before, which makes
this style of use fragile.

Another way to achieve the same is to use the label as generated by the cu
proc itself:
...
  variable _cu_label
  cu {} {
  }
  set header_start_cu_a $_cu_label
  cu {} {
  }
  set header_start_cu_b $_cu_label
...
but again that seems fragile given that adding a new CU inbetween will
silently result in the wrong value for the label.

Add a label option to proc cu such that we can simply do:
...
  declare_labels header_start_cu_a
  cu { label header_start_cu_a } {
  }
  cu { label header_start_cu_b } {
  }
...

Tested on x86_64-linux.

Any comments?

Thanks,
- Tom

[gdb/testsuite] Add label option to proc cu

---
 gdb/testsuite/lib/dwarf.exp | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/gdb/testsuite/lib/dwarf.exp b/gdb/testsuite/lib/dwarf.exp
index ce7b5983ab9..8dda798ddf8 100644
--- a/gdb/testsuite/lib/dwarf.exp
+++ b/gdb/testsuite/lib/dwarf.exp
@@ -1258,6 +1258,10 @@ namespace eval Dwarf {
     #                default = default
     # fission 0|1  - boolean indicating if generating Fission debug info
     #                default = 0
+    # label <label>
+    #              - string indicating label to be defined at the start
+    #                of the CU header.
+    #                default = ""
     # BODY is Tcl code that emits the DIEs which make up the body of
     # the CU.  It is evaluated in the caller's context.
     proc cu {options body} {
@@ -1278,6 +1282,7 @@ namespace eval Dwarf {
 	set _cu_is_fission 0
 	set section ".debug_info"
 	set _abbrev_section ".debug_abbrev"
+	set label ""
 
 	foreach { name value } $options {
 	    set value [uplevel 1 "subst \"$value\""]
@@ -1286,6 +1291,7 @@ namespace eval Dwarf {
 		version { set _cu_version $value }
 		addr_size { set _cu_addr_size $value }
 		fission { set _cu_is_fission $value }
+		label { set label $value }
 		default { error "unknown option $name" }
 	    }
 	}
@@ -1309,6 +1315,9 @@ namespace eval Dwarf {
 	}
 
 	_section $section
+	if { $label != "" } {
+	    $label:
+	}
 
 	set cu_num [incr _cu_count]
 	set my_abbrevs [_compute_label "abbrev${cu_num}_begin"]

Re: [PATCH][gdb/testsuite] Add label option to proc cu

[Simon Marchi]

On 2021-08-26 7:56 a.m., Tom de Vries via Gdb-patches wrote:
> Hi,
> 
> We can use current dwarf assembly infrastructure to declare a label that marks
> the start of the CU header:
> ...
>   declare_labels header_start_cu_a
>   _section ".debug_info"
>   header_start_cu_a : cu {} {
>   }
>   _section ".debug_info"
>   header_start_cu_b : cu {} {
>   }
> ...
> on the condition that we switch to the .debug_info section before, which makes
> this style of use fragile.
> 
> Another way to achieve the same is to use the label as generated by the cu
> proc itself:
> ...
>   variable _cu_label
>   cu {} {
>   }
>   set header_start_cu_a $_cu_label
>   cu {} {
>   }
>   set header_start_cu_b $_cu_label
> ...
> but again that seems fragile given that adding a new CU inbetween will
> silently result in the wrong value for the label.

And this is done by poking in the internals of the framework, not good.

> Add a label option to proc cu such that we can simply do:
> ...
>   declare_labels header_start_cu_a
>   cu { label header_start_cu_a } {
>   }
>   cu { label header_start_cu_b } {
>   }

That looks like the right way to do it.  But can we make it so the
caller doesn't need to use declare_labels?  For example, with rnglists'
-post-header-label switch, it's not needed, so I presume it would be
possible here.

Simon

Re: [PATCH][gdb/testsuite] Add label option to proc cu

[Tom de Vries]

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

On 8/26/21 3:38 PM, Simon Marchi wrote:
> On 2021-08-26 7:56 a.m., Tom de Vries via Gdb-patches wrote:
>> Hi,
>>
>> We can use current dwarf assembly infrastructure to declare a label that marks
>> the start of the CU header:
>> ...
>>   declare_labels header_start_cu_a
>>   _section ".debug_info"
>>   header_start_cu_a : cu {} {
>>   }
>>   _section ".debug_info"
>>   header_start_cu_b : cu {} {
>>   }
>> ...
>> on the condition that we switch to the .debug_info section before, which makes
>> this style of use fragile.
>>
>> Another way to achieve the same is to use the label as generated by the cu
>> proc itself:
>> ...
>>   variable _cu_label
>>   cu {} {
>>   }
>>   set header_start_cu_a $_cu_label
>>   cu {} {
>>   }
>>   set header_start_cu_b $_cu_label
>> ...
>> but again that seems fragile given that adding a new CU inbetween will
>> silently result in the wrong value for the label.
> 
> And this is done by poking in the internals of the framework, not good.
> 
>> Add a label option to proc cu such that we can simply do:
>> ...
>>   declare_labels header_start_cu_a
>>   cu { label header_start_cu_a } {
>>   }
>>   cu { label header_start_cu_b } {
>>   }
> 
> That looks like the right way to do it.  But can we make it so the
> caller doesn't need to use declare_labels?  For example, with rnglists'
> -post-header-label switch, it's not needed, so I presume it would be
> possible here.

Done.

I looked at -post-header-label, but that puts the burden on the caller
to come up with a unique name.  That doesn't work well with proc
dummy_cu being defined like this:
...
    proc dummy_cu {} {
        # Generate a CU with default options and empty body.

        cu {label dummy_cu} {
        }

        # Generate an .debug_aranges entry for the dummy CU.

        aranges {} dummy_cu {
        }
    }
...
and then being called twice.

Instead, I opted to reuse _cu_label, but rather than poking it's now
exported by the proc.

WDYT?

Thanks,
- Tom



[-- Attachment #2: 0006-gdb-testsuite-Add-label-option-to-proc-cu.patch --]
[-- Type: text/x-patch, Size: 2466 bytes --]

[gdb/testsuite] Add label option to proc cu

We can use current dwarf assembly infrastructure to declare a label that marks
the start of the CU header:
...
  declare_labels header_start_cu_a
  _section ".debug_info"
  header_start_cu_a : cu {} {
  }
  _section ".debug_info"
  header_start_cu_b : cu {} {
  }
...
on the condition that we switch to the .debug_info section before, which makes
this style of use fragile.

Another way to achieve the same is to use the label as generated by the cu
proc itself:
...
  variable _cu_label
  cu {} {
  }
  set header_start_cu_a $_cu_label
  cu {} {
  }
  set header_start_cu_b $_cu_label
...
but again that seems fragile given that adding a new CU inbetween will
silently result in the wrong value for the label.

Add a label option to proc cu such that we can simply do:
...
  cu { label header_start_cu_a } {
  }
  cu { label header_start_cu_b } {
  }
...

Tested on x86_64-linux.

---
 gdb/testsuite/lib/dwarf.exp | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/gdb/testsuite/lib/dwarf.exp b/gdb/testsuite/lib/dwarf.exp
index ce7b5983ab9..4f66b91ccbe 100644
--- a/gdb/testsuite/lib/dwarf.exp
+++ b/gdb/testsuite/lib/dwarf.exp
@@ -1258,6 +1258,10 @@ namespace eval Dwarf {
     #                default = default
     # fission 0|1  - boolean indicating if generating Fission debug info
     #                default = 0
+    # label <label>
+    #              - string indicating label to be defined at the start
+    #                of the CU header.
+    #                default = ""
     # BODY is Tcl code that emits the DIEs which make up the body of
     # the CU.  It is evaluated in the caller's context.
     proc cu {options body} {
@@ -1278,6 +1282,7 @@ namespace eval Dwarf {
 	set _cu_is_fission 0
 	set section ".debug_info"
 	set _abbrev_section ".debug_abbrev"
+	set label ""
 
 	foreach { name value } $options {
 	    set value [uplevel 1 "subst \"$value\""]
@@ -1286,6 +1291,7 @@ namespace eval Dwarf {
 		version { set _cu_version $value }
 		addr_size { set _cu_addr_size $value }
 		fission { set _cu_is_fission $value }
+		label { set label $value }
 		default { error "unknown option $name" }
 	    }
 	}
@@ -1318,6 +1324,9 @@ namespace eval Dwarf {
 	set start_label [_compute_label "cu${cu_num}_start"]
 	set end_label [_compute_label "cu${cu_num}_end"]
 
+	upvar $label my_label
+	set my_label $_cu_label
+
 	define_label $_cu_label
 	if {$is_64} {
 	    _op .4byte 0xffffffff

Re: [PATCH][gdb/testsuite] Add label option to proc cu

[Tom Tromey]

>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:

Tom> +	upvar $label my_label
Tom> +	set my_label $_cu_label

Maybe this should be conditional on $label != ""

Tom

Re: [PATCH][gdb/testsuite] Add label option to proc cu

[Tom de Vries]

On 8/27/21 3:24 PM, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Tom> +	upvar $label my_label
> Tom> +	set my_label $_cu_label
> 
> Maybe this should be conditional on $label != ""

It should indeed.

I tried to understand why I didn't run into an error with this, and:
...
$ cat test.tcl
#!/usr/bin/tclsh

proc foo { } {
    set label ""
    upvar $label my_label
    set my_label "bla"
}

foo
puts ${}
$ ./test.tcl
bla
...
apparently it's possible to create a variable with the empty string as
variable name.

Thanks,
- Tom