[PATCH] Start abbrevs at 1 in DWARF assembler

[PATCH] Start abbrevs at 1 in DWARF assembler

[Tom Tromey]

I noticed that the DWARF assembler starts abbrevs at 2.
I think 1 should be preferred.
---
 gdb/testsuite/lib/dwarf.exp | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/gdb/testsuite/lib/dwarf.exp b/gdb/testsuite/lib/dwarf.exp
index f09da0430ab..19c89c048fe 100644
--- a/gdb/testsuite/lib/dwarf.exp
+++ b/gdb/testsuite/lib/dwarf.exp
@@ -564,8 +564,8 @@ namespace eval Dwarf {
     # for Fission.
     variable _abbrev_section
 
-    # The next available abbrev number in the current CU's abbrev
-    # table.
+    # The most recently assigned abbrev number in the current CU's
+    # abbrev table.
     variable _abbrev_num
 
     # The string table for this assembly.  The key is the string; the
@@ -1485,7 +1485,7 @@ namespace eval Dwarf {
 
 	set cu_num [incr _cu_count]
 	set my_abbrevs [_compute_label "abbrev${cu_num}_begin"]
-	set _abbrev_num 1
+	set _abbrev_num 0
 
 	set _cu_label [_compute_label "cu${cu_num}_begin"]
 	set start_label [_compute_label "cu${cu_num}_start"]
@@ -1597,7 +1597,7 @@ namespace eval Dwarf {
 
 	set cu_num [incr _cu_count]
 	set my_abbrevs [_compute_label "abbrev${cu_num}_begin"]
-	set _abbrev_num 1
+	set _abbrev_num 0
 
 	set _cu_label [_compute_label "cu${cu_num}_begin"]
 	set start_label [_compute_label "cu${cu_num}_start"]
-- 
2.43.0

Re: [PATCH] Start abbrevs at 1 in DWARF assembler

[Tom de Vries]

On 12/5/23 22:23, Tom Tromey wrote:
> I noticed that the DWARF assembler starts abbrevs at 2.
> I think 1 should be preferred.

Agreed.

How about this approach instead:
...
diff --git a/gdb/testsuite/lib/dwarf.exp b/gdb/testsuite/lib/dwarf.exp
index f09da0430ab..0813bbbda09 100644
--- a/gdb/testsuite/lib/dwarf.exp
+++ b/gdb/testsuite/lib/dwarf.exp
@@ -958,13 +958,21 @@ namespace eval Dwarf {
  	_handle_macro_at_range $attr_value
      }

+    # Return the next available abbrev number in the current CU's abbrev
+    # table.
+    proc _get_abbrev_num {} {
+	variable _abbrev_num
+	set res $_abbrev_num
+	incr _abbrev_num
+	return $res
+    }
+
      proc _handle_DW_TAG {tag_name {attrs {}} {children {}}} {
  	variable _abbrev_section
-	variable _abbrev_num
  	variable _constants

  	set has_children [expr {[string length $children] > 0}]
-	set my_abbrev [incr _abbrev_num]
+	set my_abbrev [_get_abbrev_num]

  	# We somewhat wastefully emit a new abbrev entry for each tag.
  	# There's no reason for this other than laziness.
...

The nit I see with the patch you propose is that it sets _abbrev_num to 
0, and then claims that 0 is the most recently assigned abbrev number, 
while it was never assigned.

My feeling is that this solution is more intuitive.

Thanks,
- Tom

> ---
>   gdb/testsuite/lib/dwarf.exp | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/gdb/testsuite/lib/dwarf.exp b/gdb/testsuite/lib/dwarf.exp
> index f09da0430ab..19c89c048fe 100644
> --- a/gdb/testsuite/lib/dwarf.exp
> +++ b/gdb/testsuite/lib/dwarf.exp
> @@ -564,8 +564,8 @@ namespace eval Dwarf {
>       # for Fission.
>       variable _abbrev_section
>   
> -    # The next available abbrev number in the current CU's abbrev
> -    # table.
> +    # The most recently assigned abbrev number in the current CU's
> +    # abbrev table.
>       variable _abbrev_num
>   
>       # The string table for this assembly.  The key is the string; the
> @@ -1485,7 +1485,7 @@ namespace eval Dwarf {
>   
>   	set cu_num [incr _cu_count]
>   	set my_abbrevs [_compute_label "abbrev${cu_num}_begin"]
> -	set _abbrev_num 1
> +	set _abbrev_num 0
>   
>   	set _cu_label [_compute_label "cu${cu_num}_begin"]
>   	set start_label [_compute_label "cu${cu_num}_start"]
> @@ -1597,7 +1597,7 @@ namespace eval Dwarf {
>   
>   	set cu_num [incr _cu_count]
>   	set my_abbrevs [_compute_label "abbrev${cu_num}_begin"]
> -	set _abbrev_num 1
> +	set _abbrev_num 0
>   
>   	set _cu_label [_compute_label "cu${cu_num}_begin"]
>   	set start_label [_compute_label "cu${cu_num}_start"]

Re: [PATCH] Start abbrevs at 1 in DWARF assembler

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

>> I noticed that the DWARF assembler starts abbrevs at 2.
>> I think 1 should be preferred.

Tom> Agreed.

Tom> How about this approach instead:

It's fine by me, but FWIW I have also been toying with fixing the buglet
that abbrevs aren't cached, because I happened to notice that
mega-enum.exp makes an executable with 65000+ abbrevs where only 5 or so
are actually needed.  This patch would obsolete this counter entirely.

Tom

Re: [PATCH] Start abbrevs at 1 in DWARF assembler

[Tom de Vries]

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

On 12/6/23 16:41, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
>>> I noticed that the DWARF assembler starts abbrevs at 2.
>>> I think 1 should be preferred.
> 
> Tom> Agreed.
> 
> Tom> How about this approach instead:
> 
> It's fine by me,

Pushed as attached.

> but FWIW I have also been toying with fixing the buglet
> that abbrevs aren't cached, because I happened to notice that
> mega-enum.exp makes an executable with 65000+ abbrevs where only 5 or so
> are actually needed.  This patch would obsolete this counter entirely.
>

Ack.

Thanks,
- Tom


[-- Attachment #2: 0001-Start-abbrevs-at-1-in-DWARF-assembler.patch --]
[-- Type: text/x-patch, Size: 1359 bytes --]

From 1d72857c56141ef7971ccaea5aeae1d0053a5905 Mon Sep 17 00:00:00 2001
From: Tom Tromey <tom@tromey.com>
Date: Tue, 5 Dec 2023 14:23:06 -0700
Subject: [PATCH] Start abbrevs at 1 in DWARF assembler

I noticed that the DWARF assembler starts abbrevs at 2.
I think 1 should be preferred.

Co-Authored-By: Tom de Vries <tdevries@suse.de>
---
 gdb/testsuite/lib/dwarf.exp | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/gdb/testsuite/lib/dwarf.exp b/gdb/testsuite/lib/dwarf.exp
index f09da0430ab..a9b5be859a8 100644
--- a/gdb/testsuite/lib/dwarf.exp
+++ b/gdb/testsuite/lib/dwarf.exp
@@ -958,13 +958,22 @@ namespace eval Dwarf {
 	_handle_macro_at_range $attr_value
     }
 
+    # Return the next available abbrev number in the current CU's abbrev
+    # table.
+    proc _get_abbrev_num {} {
+	variable _abbrev_num
+	set res $_abbrev_num
+	incr _abbrev_num
+	return $res
+    }
+
     proc _handle_DW_TAG {tag_name {attrs {}} {children {}}} {
 	variable _abbrev_section
 	variable _abbrev_num
 	variable _constants
 
 	set has_children [expr {[string length $children] > 0}]
-	set my_abbrev [incr _abbrev_num]
+	set my_abbrev [_get_abbrev_num]
 
 	# We somewhat wastefully emit a new abbrev entry for each tag.
 	# There's no reason for this other than laziness.

base-commit: 288363c1737f93e2f7cb8c06026c11a5ff77fb58
-- 
2.35.3