[PATCH] compile-loc2c: Fix uninitialized variable error

[PATCH] compile-loc2c: Fix uninitialized variable error

[Simon Marchi]

Compiling with clang gives this warning/error:

  /home/emaisin/src/binutils-gdb/gdb/compile/compile-loc2c.c:731:6: error: variable 'uoffset' is uninitialized when used here [-Werror,-Wuninitialized]
              uoffset += dwarf2_per_cu_text_offset (per_cu);
              ^~~~~~~
  /home/emaisin/src/binutils-gdb/gdb/compile/compile-loc2c.c:669:23: note: initialize the variable 'uoffset' to silence this warning
        uint64_t uoffset, reg;
                        ^
                         = 0

I am really not sure if what this patch does is good, but it is my best
guess.  DW_OP_addr means that there's an constant address provided by
the DWARF bytecode that should be pushed on the stack.  That address is
considered skipped by the "op_ptr += addr_size", but it is never read.
uoffset is indeed read just after, without having been assigned first.

So I think the intent is to read the address, it was just omitted.

gdb/ChangeLog:

	* compile/compile-loc2c.c (do_compile_dwarf_expr_to_c): Read
	address when op is DW_OP_addr.
---
 gdb/compile/compile-loc2c.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/gdb/compile/compile-loc2c.c b/gdb/compile/compile-loc2c.c
index a53214f..ead1003 100644
--- a/gdb/compile/compile-loc2c.c
+++ b/gdb/compile/compile-loc2c.c
@@ -722,6 +722,7 @@ do_compile_dwarf_expr_to_c (int indent, string_file &stream,
 	  break;
 
 	case DW_OP_addr:
+	  uoffset = extract_unsigned_integer (op_ptr, addr_size, byte_order);
 	  op_ptr += addr_size;
 	  /* Some versions of GCC emit DW_OP_addr before
 	     DW_OP_GNU_push_tls_address.  In this case the value is an
-- 
2.7.4

Re: [PATCH] compile-loc2c: Fix uninitialized variable error

[Simon Marchi]

On 2017-06-10 21:49, Simon Marchi wrote:
> Compiling with clang gives this warning/error:
> 
>   /home/emaisin/src/binutils-gdb/gdb/compile/compile-loc2c.c:731:6:
> error: variable 'uoffset' is uninitialized when used here
> [-Werror,-Wuninitialized]
>               uoffset += dwarf2_per_cu_text_offset (per_cu);
>               ^~~~~~~
>   /home/emaisin/src/binutils-gdb/gdb/compile/compile-loc2c.c:669:23:
> note: initialize the variable 'uoffset' to silence this warning
>         uint64_t uoffset, reg;
>                         ^
>                          = 0
> 
> I am really not sure if what this patch does is good, but it is my best
> guess.  DW_OP_addr means that there's an constant address provided by
> the DWARF bytecode that should be pushed on the stack.  That address is
> considered skipped by the "op_ptr += addr_size", but it is never read.
> uoffset is indeed read just after, without having been assigned first.
> 
> So I think the intent is to read the address, it was just omitted.
> 
> gdb/ChangeLog:
> 
> 	* compile/compile-loc2c.c (do_compile_dwarf_expr_to_c): Read
> 	address when op is DW_OP_addr.
> ---
>  gdb/compile/compile-loc2c.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/gdb/compile/compile-loc2c.c b/gdb/compile/compile-loc2c.c
> index a53214f..ead1003 100644
> --- a/gdb/compile/compile-loc2c.c
> +++ b/gdb/compile/compile-loc2c.c
> @@ -722,6 +722,7 @@ do_compile_dwarf_expr_to_c (int indent, string_file 
> &stream,
>  	  break;
> 
>  	case DW_OP_addr:
> +	  uoffset = extract_unsigned_integer (op_ptr, addr_size, byte_order);
>  	  op_ptr += addr_size;
>  	  /* Some versions of GCC emit DW_OP_addr before
>  	     DW_OP_GNU_push_tls_address.  In this case the value is an

Hi Tom,

As you are the original author of that code, would it be possible for 
you to take a quick look, if you remember any of this :) ?

Thanks,

Simon

Re: [PATCH] compile-loc2c: Fix uninitialized variable error

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

Sorry about the delay on this.  I've been away.

>> * compile/compile-loc2c.c (do_compile_dwarf_expr_to_c): Read
>> address when op is DW_OP_addr.

Simon> As you are the original author of that code, would it be possible for
Simon> you to take a quick look, if you remember any of this :) ?

This patch looks correct to me.  I'm sure the earlier state was just an
oversight.

FWIW in gimli's DWARF expression code, I separated the format decoder
from the evaluator to avoid this sort of problem.  Each operation is
decoded to an object; and then evaluation is done using the object.
This isn't perfect, as different users of the decoded objects can still
disagree about the semantics of the operation, but an approach like this
would have caught this bug.

Tom

Re: [PATCH] compile-loc2c: Fix uninitialized variable error

[Simon Marchi]

On 2017-07-09 18:44, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:
> 
> Sorry about the delay on this.  I've been away.
> 
>>> * compile/compile-loc2c.c (do_compile_dwarf_expr_to_c): Read
>>> address when op is DW_OP_addr.
> 
> Simon> As you are the original author of that code, would it be 
> possible for
> Simon> you to take a quick look, if you remember any of this :) ?
> 
> This patch looks correct to me.  I'm sure the earlier state was just an
> oversight.

Thanks! Pushing it now.

> FWIW in gimli's DWARF expression code, I separated the format decoder
> from the evaluator to avoid this sort of problem.  Each operation is
> decoded to an object; and then evaluation is done using the object.
> This isn't perfect, as different users of the decoded objects can still
> disagree about the semantics of the operation, but an approach like 
> this
> would have caught this bug.

Indeed, that sounds like a good design.

Simon