public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH] Add some error checking to DWARF assembler
@ 2021-03-18 15:58 Tom Tromey
  2021-03-18 17:15 ` Andrew Burgess
  0 siblings, 1 reply; 6+ messages in thread
From: Tom Tromey @ 2021-03-18 15:58 UTC (permalink / raw)
  To: gdb-patches; +Cc: Tom Tromey

I had written a DWARF location expression like

    DW_OP_const1u
    DW_OP_stack_value

... and was surprised to see that the DW_OP_stack_value didn't appear
in the "readelf" output.

The problem here is that DW_OP_const1u requires an operand, but
neither the DWARF assembler nor gas diagnosed this problem.

This patch adds some checking to Dwarf::_location to try to avoid this
in the future.  I removed some existing checks in the name of
normalization; while the new messages are a bit worse (being
one-size-fits-all), I think this doesn't matter hugely, since this
should only be seen for test suite bugs.

gdb/testsuite/ChangeLog
2021-03-18  Tom Tromey  <tromey@adacore.com>

	* lib/dwarf.exp (Dwarf::_check_n): New proc.
	(Dwarf::_location): Use it.
---
 gdb/testsuite/ChangeLog     |  5 +++++
 gdb/testsuite/lib/dwarf.exp | 33 +++++++++++++++++++++++----------
 2 files changed, 28 insertions(+), 10 deletions(-)

diff --git a/gdb/testsuite/lib/dwarf.exp b/gdb/testsuite/lib/dwarf.exp
index f8fbd381810..64b9e263f10 100644
--- a/gdb/testsuite/lib/dwarf.exp
+++ b/gdb/testsuite/lib/dwarf.exp
@@ -890,6 +890,13 @@ namespace eval Dwarf {
 	}
     }
 
+    # Require N arguments for the opcode.
+    proc _check_n {line op n} {
+	if {[llength $line] != $n + 1} {
+	    error "$op requires $n arguments"
+	}
+    }
+
     # This is a miniature assembler for location expressions.  It is
     # suitable for use in the attributes to a DIE.  Its output is
     # prefixed with "=" to make it automatically use DW_FORM_block.
@@ -926,56 +933,68 @@ namespace eval Dwarf {
 
 	    switch -exact -- $opcode {
 		DW_OP_addr {
+		    _check_n $line $opcode 1
 		    _op .${addr_size}byte [lindex $line 1]
 		}
 
 		DW_OP_regx {
+		    _check_n $line $opcode 1
 		    _op .uleb128 [lindex $line 1]
 		}
 
 		DW_OP_pick -
 		DW_OP_const1u -
 		DW_OP_const1s {
+		    _check_n $line $opcode 1
 		    _op .byte [lindex $line 1]
 		}
 
 		DW_OP_const2u -
 		DW_OP_const2s {
+		    _check_n $line $opcode 1
 		    _op .2byte [lindex $line 1]
 		}
 
 		DW_OP_const4u -
 		DW_OP_const4s {
+		    _check_n $line $opcode 1
 		    _op .4byte [lindex $line 1]
 		}
 
 		DW_OP_const8u -
 		DW_OP_const8s {
+		    _check_n $line $opcode 1
 		    _op .8byte [lindex $line 1]
 		}
 
 		DW_OP_constu {
+		    _check_n $line $opcode 1
 		    _op .uleb128 [lindex $line 1]
 		}
 		DW_OP_consts {
+		    _check_n $line $opcode 1
 		    _op .sleb128 [lindex $line 1]
 		}
 
 		DW_OP_plus_uconst {
+		    _check_n $line $opcode 1
 		    _op .uleb128 [lindex $line 1]
 		}
 
 		DW_OP_piece {
+		    _check_n $line $opcode 1
 		    _op .uleb128 [lindex $line 1]
 		}
 
 		DW_OP_bit_piece {
+		    _check_n $line $opcode 2
 		    _op .uleb128 [lindex $line 1]
 		    _op .uleb128 [lindex $line 2]
 		}
 
 		DW_OP_skip -
 		DW_OP_bra {
+		    _check_n $line $opcode 1
 		    _op .2byte [lindex $line 1]
 		}
 
@@ -1000,9 +1019,7 @@ namespace eval Dwarf {
 
 		DW_OP_implicit_pointer -
 		DW_OP_GNU_implicit_pointer {
-		    if {[llength $line] != 3} {
-			error "usage: $opcode LABEL OFFSET"
-		    }
+		    _check_n $line $opcode 2
 
 		    # Here label is a section offset.
 		    set label [lindex $line 1]
@@ -1015,9 +1032,7 @@ namespace eval Dwarf {
 		}
 
 		DW_OP_GNU_variable_value {
-		    if {[llength $line] != 2} {
-			error "usage: $opcode LABEL"
-		    }
+		    _check_n $line $opcode 1
 
 		    # Here label is a section offset.
 		    set label [lindex $line 1]
@@ -1029,14 +1044,12 @@ namespace eval Dwarf {
 		}
 
 		DW_OP_deref_size {
-		    if {[llength $line] != 2} {
-			error "usage: DW_OP_deref_size SIZE"
-		    }
-
+		    _check_n $line $opcode 1
 		    _op .byte [lindex $line 1]
 		}
 
 		DW_OP_bregx {
+		    _check_n $line $opcode 2
 		    _op .uleb128 [lindex $line 1]
 		    _op .sleb128 [lindex $line 2]
 		}
-- 
2.26.2


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] Add some error checking to DWARF assembler
  2021-03-18 15:58 [PATCH] Add some error checking to DWARF assembler Tom Tromey
@ 2021-03-18 17:15 ` Andrew Burgess
  2021-03-18 18:22   ` Tom Tromey
  0 siblings, 1 reply; 6+ messages in thread
From: Andrew Burgess @ 2021-03-18 17:15 UTC (permalink / raw)
  To: Tom Tromey; +Cc: gdb-patches

* Tom Tromey <tromey@adacore.com> [2021-03-18 09:58:26 -0600]:

> I had written a DWARF location expression like
> 
>     DW_OP_const1u
>     DW_OP_stack_value
> 
> ... and was surprised to see that the DW_OP_stack_value didn't appear
> in the "readelf" output.
> 
> The problem here is that DW_OP_const1u requires an operand, but
> neither the DWARF assembler nor gas diagnosed this problem.
> 
> This patch adds some checking to Dwarf::_location to try to avoid this
> in the future.  I removed some existing checks in the name of
> normalization; while the new messages are a bit worse (being
> one-size-fits-all), I think this doesn't matter hugely, since this
> should only be seen for test suite bugs.
> 
> gdb/testsuite/ChangeLog
> 2021-03-18  Tom Tromey  <tromey@adacore.com>
> 
> 	* lib/dwarf.exp (Dwarf::_check_n): New proc.
> 	(Dwarf::_location): Use it.
> ---
>  gdb/testsuite/ChangeLog     |  5 +++++
>  gdb/testsuite/lib/dwarf.exp | 33 +++++++++++++++++++++++----------
>  2 files changed, 28 insertions(+), 10 deletions(-)
> 
> diff --git a/gdb/testsuite/lib/dwarf.exp b/gdb/testsuite/lib/dwarf.exp
> index f8fbd381810..64b9e263f10 100644
> --- a/gdb/testsuite/lib/dwarf.exp
> +++ b/gdb/testsuite/lib/dwarf.exp
> @@ -890,6 +890,13 @@ namespace eval Dwarf {
>  	}
>      }
>  
> +    # Require N arguments for the opcode.
> +    proc _check_n {line op n} {
> +	if {[llength $line] != $n + 1} {
> +	    error "$op requires $n arguments"
> +	}
> +    }
> +
>      # This is a miniature assembler for location expressions.  It is
>      # suitable for use in the attributes to a DIE.  Its output is
>      # prefixed with "=" to make it automatically use DW_FORM_block.
> @@ -926,56 +933,68 @@ namespace eval Dwarf {
>  
>  	    switch -exact -- $opcode {
>  		DW_OP_addr {
> +		    _check_n $line $opcode 1
>  		    _op .${addr_size}byte [lindex $line 1]
>  		}
>  
>  		DW_OP_regx {
> +		    _check_n $line $opcode 1
>  		    _op .uleb128 [lindex $line 1]
>  		}
>  
>  		DW_OP_pick -
>  		DW_OP_const1u -
>  		DW_OP_const1s {
> +		    _check_n $line $opcode 1
>  		    _op .byte [lindex $line 1]
>  		}
>  
>  		DW_OP_const2u -
>  		DW_OP_const2s {
> +		    _check_n $line $opcode 1
>  		    _op .2byte [lindex $line 1]
>  		}
>  
>  		DW_OP_const4u -
>  		DW_OP_const4s {
> +		    _check_n $line $opcode 1
>  		    _op .4byte [lindex $line 1]
>  		}
>  
>  		DW_OP_const8u -
>  		DW_OP_const8s {
> +		    _check_n $line $opcode 1
>  		    _op .8byte [lindex $line 1]
>  		}
>  
>  		DW_OP_constu {
> +		    _check_n $line $opcode 1
>  		    _op .uleb128 [lindex $line 1]
>  		}
>  		DW_OP_consts {
> +		    _check_n $line $opcode 1
>  		    _op .sleb128 [lindex $line 1]
>  		}
>  
>  		DW_OP_plus_uconst {
> +		    _check_n $line $opcode 1
>  		    _op .uleb128 [lindex $line 1]
>  		}
>  
>  		DW_OP_piece {
> +		    _check_n $line $opcode 1
>  		    _op .uleb128 [lindex $line 1]
>  		}
>  
>  		DW_OP_bit_piece {
> +		    _check_n $line $opcode 2
>  		    _op .uleb128 [lindex $line 1]
>  		    _op .uleb128 [lindex $line 2]
>  		}
>  
>  		DW_OP_skip -
>  		DW_OP_bra {
> +		    _check_n $line $opcode 1
>  		    _op .2byte [lindex $line 1]
>  		}
>  
> @@ -1000,9 +1019,7 @@ namespace eval Dwarf {
>  
>  		DW_OP_implicit_pointer -
>  		DW_OP_GNU_implicit_pointer {
> -		    if {[llength $line] != 3} {
> -			error "usage: $opcode LABEL OFFSET"
> -		    }
> +		    _check_n $line $opcode 2
>  
>  		    # Here label is a section offset.
>  		    set label [lindex $line 1]
> @@ -1015,9 +1032,7 @@ namespace eval Dwarf {
>  		}
>  
>  		DW_OP_GNU_variable_value {
> -		    if {[llength $line] != 2} {
> -			error "usage: $opcode LABEL"
> -		    }
> +		    _check_n $line $opcode 1
>  
>  		    # Here label is a section offset.
>  		    set label [lindex $line 1]
> @@ -1029,14 +1044,12 @@ namespace eval Dwarf {
>  		}
>  
>  		DW_OP_deref_size {
> -		    if {[llength $line] != 2} {
> -			error "usage: DW_OP_deref_size SIZE"
> -		    }
> -
> +		    _check_n $line $opcode 1

Maybe I'm over thinking this, but you could change this to:

	_check_n $line $opcode { SIZE }

And for DW_OP_implicit_pointer above you would do:

        _check_n $line $opcode { LABEL OFFSET }

Then in _check_n you can use the length of the final list to validate
the argument count, and if its wrong you can still print the helpful
text....

Just a thought.

Either way, more error checking is good.

Thanks,
Andrew

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] Add some error checking to DWARF assembler
  2021-03-18 17:15 ` Andrew Burgess
@ 2021-03-18 18:22   ` Tom Tromey
  2021-03-18 21:15     ` Tom Tromey
  0 siblings, 1 reply; 6+ messages in thread
From: Tom Tromey @ 2021-03-18 18:22 UTC (permalink / raw)
  To: Andrew Burgess; +Cc: Tom Tromey, gdb-patches

Andrew> Maybe I'm over thinking this, but you could change this to:

Here's the definitive (IMNSHO) over-thinking approach.  It works by
having the helper proc set the necessary variables for the caller.

Tom

commit f223a278829665589f47cd39724148fa0a4ca5ce
Author: Tom Tromey <tromey@adacore.com>
Date:   Thu Mar 18 09:54:27 2021 -0600

    Add some error checking to DWARF assembler
    
    I had written a DWARF location expression like
    
        DW_OP_const1u
        DW_OP_stack_value
    
    ... and was surprised to see that the DW_OP_stack_value didn't appear
    in the "readelf" output.
    
    The problem here is that DW_OP_const1u requires an operand, but
    neither the DWARF assembler nor gas diagnosed this problem.
    
    This patch adds some checking to Dwarf::_location to try to avoid this
    in the future.  The checking is done via a helper proc that also
    dissects the argument list and sets local variables in the caller's
    frame.
    
    gdb/testsuite/ChangeLog
    2021-03-18  Tom Tromey  <tromey@adacore.com>
    
            * lib/dwarf.exp (Dwarf::_get_args): New proc.
            (Dwarf::_location): Use it.

diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index c1d7fec6cc4..f76bb167502 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2021-03-18  Tom Tromey  <tromey@adacore.com>
+
+	* lib/dwarf.exp (Dwarf::_get_args): New proc.
+	(Dwarf::_location): Use it.
+
 2021-03-17  Simon Marchi  <simon.marchi@polymtl.ca>
 	    Pedro Alves  <pedro@palves.net>
 
diff --git a/gdb/testsuite/lib/dwarf.exp b/gdb/testsuite/lib/dwarf.exp
index f8fbd381810..1c672351dd7 100644
--- a/gdb/testsuite/lib/dwarf.exp
+++ b/gdb/testsuite/lib/dwarf.exp
@@ -890,6 +890,19 @@ namespace eval Dwarf {
 	}
     }
 
+    # Assign elements from LINE to the variables named in ARGS.
+    # If the wrong number of elements appear in LINE, error.
+    proc _get_args {line op args} {
+	if {[llength $line] != [llength $args] + 1} {
+	    error "usage: $op [string toupper $args]"
+	}
+
+	foreach var $args value [lreplace $line 0 0] {
+	    upvar $var __$var
+	    set __$var $value
+	}
+    }
+
     # This is a miniature assembler for location expressions.  It is
     # suitable for use in the attributes to a DIE.  Its output is
     # prefixed with "=" to make it automatically use DW_FORM_block.
@@ -926,57 +939,69 @@ namespace eval Dwarf {
 
 	    switch -exact -- $opcode {
 		DW_OP_addr {
-		    _op .${addr_size}byte [lindex $line 1]
+		    _get_args $line $opcode size
+		    _op .${addr_size}byte $size
 		}
 
 		DW_OP_regx {
-		    _op .uleb128 [lindex $line 1]
+		    _get_args $line $opcode register
+		    _op .uleb128 $register
 		}
 
 		DW_OP_pick -
 		DW_OP_const1u -
 		DW_OP_const1s {
-		    _op .byte [lindex $line 1]
+		    _get_args $line $opcode const
+		    _op .byte $const
 		}
 
 		DW_OP_const2u -
 		DW_OP_const2s {
-		    _op .2byte [lindex $line 1]
+		    _get_args $line $opcode const
+		    _op .2byte $const
 		}
 
 		DW_OP_const4u -
 		DW_OP_const4s {
-		    _op .4byte [lindex $line 1]
+		    _get_args $line $opcode const
+		    _op .4byte $const
 		}
 
 		DW_OP_const8u -
 		DW_OP_const8s {
-		    _op .8byte [lindex $line 1]
+		    _get_args $line $opcode const
+		    _op .8byte $const
 		}
 
 		DW_OP_constu {
-		    _op .uleb128 [lindex $line 1]
+		    _get_args $line $opcode const
+		    _op .uleb128 $const
 		}
 		DW_OP_consts {
-		    _op .sleb128 [lindex $line 1]
+		    _get_args $line $opcode const
+		    _op .sleb128 $const
 		}
 
 		DW_OP_plus_uconst {
-		    _op .uleb128 [lindex $line 1]
+		    _get_args $line $opcode const
+		    _op .uleb128 $const
 		}
 
 		DW_OP_piece {
-		    _op .uleb128 [lindex $line 1]
+		    _get_args $line $opcode size
+		    _op .uleb128 $size
 		}
 
 		DW_OP_bit_piece {
-		    _op .uleb128 [lindex $line 1]
-		    _op .uleb128 [lindex $line 2]
+		    _get_args $line $opcode size offset
+		    _op .uleb128 $size
+		    _op .uleb128 $offset
 		}
 
 		DW_OP_skip -
 		DW_OP_bra {
-		    _op .2byte [lindex $line 1]
+		    _get_args $line $opcode label
+		    _op .2byte $label
 		}
 
 		DW_OP_implicit_value {
@@ -1000,27 +1025,21 @@ namespace eval Dwarf {
 
 		DW_OP_implicit_pointer -
 		DW_OP_GNU_implicit_pointer {
-		    if {[llength $line] != 3} {
-			error "usage: $opcode LABEL OFFSET"
-		    }
+		    _get_args $line $opcode label offset
 
 		    # Here label is a section offset.
-		    set label [lindex $line 1]
 		    if { $dwarf_version == 2 } {
 			_op .${addr_size}byte $label
 		    } else {
 			_op .${offset_size}byte $label
 		    }
-		    _op .sleb128 [lindex $line 2]
+		    _op .sleb128 $offset
 		}
 
 		DW_OP_GNU_variable_value {
-		    if {[llength $line] != 2} {
-			error "usage: $opcode LABEL"
-		    }
+		    _get_args $line $opcode label
 
 		    # Here label is a section offset.
-		    set label [lindex $line 1]
 		    if { $dwarf_version == 2 } {
 			_op .${addr_size}byte $label
 		    } else {
@@ -1029,16 +1048,14 @@ namespace eval Dwarf {
 		}
 
 		DW_OP_deref_size {
-		    if {[llength $line] != 2} {
-			error "usage: DW_OP_deref_size SIZE"
-		    }
-
-		    _op .byte [lindex $line 1]
+		    _get_args $line $opcode size
+		    _op .byte $size
 		}
 
 		DW_OP_bregx {
-		    _op .uleb128 [lindex $line 1]
-		    _op .sleb128 [lindex $line 2]
+		    _get_args $line $opcode register offset
+		    _op .uleb128 $register
+		    _op .sleb128 $offset
 		}
 
 		default {

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] Add some error checking to DWARF assembler
  2021-03-18 18:22   ` Tom Tromey
@ 2021-03-18 21:15     ` Tom Tromey
  2021-03-31 15:24       ` Tom Tromey
  0 siblings, 1 reply; 6+ messages in thread
From: Tom Tromey @ 2021-03-18 21:15 UTC (permalink / raw)
  To: Tom Tromey; +Cc: Andrew Burgess, gdb-patches

Andrew> Maybe I'm over thinking this, but you could change this to:
Tom> Here's the definitive (IMNSHO) over-thinking approach.  It works by
Tom> having the helper proc set the necessary variables for the caller.

One problem I thought of with that approach is that if you typo the code
in one case, you could pick up a local variable set from some previous
case.

Switching this to use an array that's cleared after every opcode avoids
this problem.

Let me know what you think.

Tom

commit b437283334e0fca000a8260f26af95b1538df7d9
Author: Tom Tromey <tromey@adacore.com>
Date:   Thu Mar 18 09:54:27 2021 -0600

    Add some error checking to DWARF assembler
    
    I had written a DWARF location expression like
    
        DW_OP_const1u
        DW_OP_stack_value
    
    ... and was surprised to see that the DW_OP_stack_value didn't appear
    in the "readelf" output.
    
    The problem here is that DW_OP_const1u requires an operand, but
    neither the DWARF assembler nor gas diagnosed this problem.
    
    This patch adds some checking to Dwarf::_location to try to avoid this
    in the future.  The checking is done via a helper proc that also
    dissects the argument list and sets an array in the caller's frame.
    
    gdb/testsuite/ChangeLog
    2021-03-18  Tom Tromey  <tromey@adacore.com>
    
            * lib/dwarf.exp (Dwarf::_get_args): New proc.
            (Dwarf::_location): Use it.

diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index c1d7fec6cc4..f76bb167502 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2021-03-18  Tom Tromey  <tromey@adacore.com>
+
+	* lib/dwarf.exp (Dwarf::_get_args): New proc.
+	(Dwarf::_location): Use it.
+
 2021-03-17  Simon Marchi  <simon.marchi@polymtl.ca>
 	    Pedro Alves  <pedro@palves.net>
 
diff --git a/gdb/testsuite/lib/dwarf.exp b/gdb/testsuite/lib/dwarf.exp
index f8fbd381810..5846a9dc862 100644
--- a/gdb/testsuite/lib/dwarf.exp
+++ b/gdb/testsuite/lib/dwarf.exp
@@ -890,6 +890,20 @@ namespace eval Dwarf {
 	}
     }
 
+    # Assign elements from LINE to the elements of an array named
+    # "argvec" in the caller scope.  The keys used are named in ARGS.
+    # If the wrong number of elements appear in LINE, error.
+    proc _get_args {line op args} {
+	if {[llength $line] != [llength $args] + 1} {
+	    error "usage: $op [string toupper $args]"
+	}
+
+	upvar argvec argvec
+	foreach var $args value [lreplace $line 0 0] {
+	    set argvec($var) $value
+	}
+    }
+
     # This is a miniature assembler for location expressions.  It is
     # suitable for use in the attributes to a DIE.  Its output is
     # prefixed with "=" to make it automatically use DW_FORM_block.
@@ -924,59 +938,72 @@ namespace eval Dwarf {
 	    set opcode [_map_name [lindex $line 0] _OP]
 	    _op .byte $_constants($opcode) $opcode
 
+	    array unset argvec *
 	    switch -exact -- $opcode {
 		DW_OP_addr {
-		    _op .${addr_size}byte [lindex $line 1]
+		    _get_args $line $opcode size
+		    _op .${addr_size}byte $argvec(size)
 		}
 
 		DW_OP_regx {
-		    _op .uleb128 [lindex $line 1]
+		    _get_args $line $opcode register
+		    _op .uleb128 $argvec(register)
 		}
 
 		DW_OP_pick -
 		DW_OP_const1u -
 		DW_OP_const1s {
-		    _op .byte [lindex $line 1]
+		    _get_args $line $opcode const
+		    _op .byte $argvec(const)
 		}
 
 		DW_OP_const2u -
 		DW_OP_const2s {
-		    _op .2byte [lindex $line 1]
+		    _get_args $line $opcode const
+		    _op .2byte $argvec(const)
 		}
 
 		DW_OP_const4u -
 		DW_OP_const4s {
-		    _op .4byte [lindex $line 1]
+		    _get_args $line $opcode const
+		    _op .4byte $argvec(const)
 		}
 
 		DW_OP_const8u -
 		DW_OP_const8s {
-		    _op .8byte [lindex $line 1]
+		    _get_args $line $opcode const
+		    _op .8byte $argvec(const)
 		}
 
 		DW_OP_constu {
-		    _op .uleb128 [lindex $line 1]
+		    _get_args $line $opcode const
+		    _op .uleb128 $argvec(const)
 		}
 		DW_OP_consts {
-		    _op .sleb128 [lindex $line 1]
+		    _get_args $line $opcode const
+		    _op .sleb128 $argvec(const)
 		}
 
 		DW_OP_plus_uconst {
-		    _op .uleb128 [lindex $line 1]
+		    _get_args $line $opcode const
+		    _op .uleb128 $argvec(const)
 		}
 
 		DW_OP_piece {
-		    _op .uleb128 [lindex $line 1]
+		    _get_args $line $opcode size
+		    _op .uleb128 $argvec(size)
 		}
 
 		DW_OP_bit_piece {
-		    _op .uleb128 [lindex $line 1]
-		    _op .uleb128 [lindex $line 2]
+		    _get_args $line $opcode size offset
+		    _op .uleb128 $argvec(size)
+		    _op .uleb128 $argvec(offset)
 		}
 
 		DW_OP_skip -
 		DW_OP_bra {
-		    _op .2byte [lindex $line 1]
+		    _get_args $line $opcode label
+		    _op .2byte $argvec(label)
 		}
 
 		DW_OP_implicit_value {
@@ -1000,45 +1027,37 @@ namespace eval Dwarf {
 
 		DW_OP_implicit_pointer -
 		DW_OP_GNU_implicit_pointer {
-		    if {[llength $line] != 3} {
-			error "usage: $opcode LABEL OFFSET"
-		    }
+		    _get_args $line $opcode label offset
 
 		    # Here label is a section offset.
-		    set label [lindex $line 1]
 		    if { $dwarf_version == 2 } {
-			_op .${addr_size}byte $label
+			_op .${addr_size}byte $argvec(label)
 		    } else {
-			_op .${offset_size}byte $label
+			_op .${offset_size}byte $argvec(label)
 		    }
-		    _op .sleb128 [lindex $line 2]
+		    _op .sleb128 $argvec(offset)
 		}
 
 		DW_OP_GNU_variable_value {
-		    if {[llength $line] != 2} {
-			error "usage: $opcode LABEL"
-		    }
+		    _get_args $line $opcode label
 
 		    # Here label is a section offset.
-		    set label [lindex $line 1]
 		    if { $dwarf_version == 2 } {
-			_op .${addr_size}byte $label
+			_op .${addr_size}byte $argvec(label)
 		    } else {
-			_op .${offset_size}byte $label
+			_op .${offset_size}byte $argvec(label)
 		    }
 		}
 
 		DW_OP_deref_size {
-		    if {[llength $line] != 2} {
-			error "usage: DW_OP_deref_size SIZE"
-		    }
-
-		    _op .byte [lindex $line 1]
+		    _get_args $line $opcode size
+		    _op .byte $argvec(size)
 		}
 
 		DW_OP_bregx {
-		    _op .uleb128 [lindex $line 1]
-		    _op .sleb128 [lindex $line 2]
+		    _get_args $line $opcode register offset
+		    _op .uleb128 $argvec(register)
+		    _op .sleb128 $argvec(offset)
 		}
 
 		default {

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] Add some error checking to DWARF assembler
  2021-03-18 21:15     ` Tom Tromey
@ 2021-03-31 15:24       ` Tom Tromey
  2021-03-31 20:07         ` Andrew Burgess
  0 siblings, 1 reply; 6+ messages in thread
From: Tom Tromey @ 2021-03-31 15:24 UTC (permalink / raw)
  To: Tom Tromey; +Cc: gdb-patches

Andrew> Maybe I'm over thinking this, but you could change this to:
Tom> Here's the definitive (IMNSHO) over-thinking approach.  It works by
Tom> having the helper proc set the necessary variables for the caller.

Tom> One problem I thought of with that approach is that if you typo the code
Tom> in one case, you could pick up a local variable set from some previous
Tom> case.

Tom> Switching this to use an array that's cleared after every opcode avoids
Tom> this problem.

I'm checking this in now.

Tom

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] Add some error checking to DWARF assembler
  2021-03-31 15:24       ` Tom Tromey
@ 2021-03-31 20:07         ` Andrew Burgess
  0 siblings, 0 replies; 6+ messages in thread
From: Andrew Burgess @ 2021-03-31 20:07 UTC (permalink / raw)
  To: Tom Tromey; +Cc: gdb-patches

* Tom Tromey <tromey@adacore.com> [2021-03-31 09:24:47 -0600]:

> Andrew> Maybe I'm over thinking this, but you could change this to:
> Tom> Here's the definitive (IMNSHO) over-thinking approach.  It works by
> Tom> having the helper proc set the necessary variables for the caller.
> 
> Tom> One problem I thought of with that approach is that if you typo the code
> Tom> in one case, you could pick up a local variable set from some previous
> Tom> case.
> 
> Tom> Switching this to use an array that's cleared after every opcode avoids
> Tom> this problem.
> 
> I'm checking this in now.

Sorry, I missed your updates.  The new patch looks great.

Thanks,
Andrew

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-03-31 20:07 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-18 15:58 [PATCH] Add some error checking to DWARF assembler Tom Tromey
2021-03-18 17:15 ` Andrew Burgess
2021-03-18 18:22   ` Tom Tromey
2021-03-18 21:15     ` Tom Tromey
2021-03-31 15:24       ` Tom Tromey
2021-03-31 20:07         ` Andrew Burgess

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).