diff mbox

compile-loc2c: Fix uninitialized variable error

Message ID 1497124148-11187-1-git-send-email-simon.marchi@ericsson.com
State New, archived
Headers

Commit Message

Simon Marchi June 10, 2017, 7:49 p.m. UTC 
  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(+)

Comments

Simon Marchi June 25, 2017, 5:24 p.m. UTC | #1 
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
Tom Tromey July 9, 2017, 4:44 p.m. UTC | #2 
>>>>> "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
Simon Marchi July 9, 2017, 6:27 p.m. UTC | #3 
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
diff mbox

Patch

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