Re: [PATCH] Add some error checking to DWARF assembler

[Andrew Burgess <andrew.burgess@embecosm.com>]

* 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