[PATCH 1/2] libdw: ensure read_encoded_value's value is set

[PATCH 1/2] libdw: ensure read_encoded_value's value is set

[Josh Stone]

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

With CFLAGS='-Og -g', F21 gcc hits -Werror=maybe-uninitialized in
read_encoded_value at "*result += value".  It's fine with -O2/-O0.

In particular it seems to care about the __libdw_cfi_read_address_inc
calls.  By my inspection, the only way those don't set value are for
error cases, which will then return immediately.  This patch just sets
value = 0 to begin with, so gcc is always convinced it's fine.

Signed-off-by: Josh Stone <jistone@redhat.com>
---
 libdw/ChangeLog       | 4 ++++
 libdw/encoded-value.h | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/libdw/ChangeLog b/libdw/ChangeLog
index d377937ff57c..b6ad425d8640 100644
--- a/libdw/ChangeLog
+++ b/libdw/ChangeLog
@@ -1,3 +1,7 @@
+2015-02-11  Josh Stone  <jistone@redhat.com>
+
+	* encoded-value.h (read_encoded_value): Initialize value.
+
 2014-12-24  Mark Wielaard  <mjw@redhat.com>
 
 	* dwarf_getsrc_die.c (dwarf_getsrc_die): Return the last line record
diff --git a/libdw/encoded-value.h b/libdw/encoded-value.h
index f953f5ef2a5f..0fa201835c5e 100644
--- a/libdw/encoded-value.h
+++ b/libdw/encoded-value.h
@@ -152,7 +152,7 @@ read_encoded_value (const Dwarf_CFI *cache, uint8_t encoding,
       return true;
     }
 
-  Dwarf_Addr value;
+  Dwarf_Addr value = 0;
   const unsigned char *endp = cache->data->d.d_buf + cache->data->d.d_size;
   switch (encoding & 0x0f)
     {
-- 
2.1.0

Re: [PATCH 1/2] libdw: ensure read_encoded_value's value is set

[Josh Stone]

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

On 02/12/2015 03:32 AM, Mark Wielaard wrote:
> On Thu, 2015-02-12 at 00:22 +0100, Petr Machata wrote:
>> Josh Stone <jistone@redhat.com> writes:
>>
>>> With CFLAGS='-Og -g', F21 gcc hits -Werror=maybe-uninitialized in
>>> read_encoded_value at "*result += value".  It's fine with -O2/-O0.
>>>
>>> By my inspection, the only way those don't set value are for error
>>> cases, which will then return immediately.
>>
>> Looks good to me.  I don't see how we could end up using value
>> uninitialized either, but oh well.
> 
> Agreed. In general I don't like working around limitations in the
> compiler not correctly deducing things like this. Especially since it
> does seem to figure things out fine with -O0/-O2. But in both this case
> and the test case the explicit initialization seems harmless/not on a
> critical path. And it is nice to have -Og work for those that want to
> use it.

Thanks, both are pushed now.

Re: [PATCH 1/2] libdw: ensure read_encoded_value's value is set

[Mark Wielaard]

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

On Thu, 2015-02-12 at 00:22 +0100, Petr Machata wrote:
> Josh Stone <jistone@redhat.com> writes:
> 
> > With CFLAGS='-Og -g', F21 gcc hits -Werror=maybe-uninitialized in
> > read_encoded_value at "*result += value".  It's fine with -O2/-O0.
> >
> > By my inspection, the only way those don't set value are for error
> > cases, which will then return immediately.
> 
> Looks good to me.  I don't see how we could end up using value
> uninitialized either, but oh well.

Agreed. In general I don't like working around limitations in the
compiler not correctly deducing things like this. Especially since it
does seem to figure things out fine with -O0/-O2. But in both this case
and the test case the explicit initialization seems harmless/not on a
critical path. And it is nice to have -Og work for those that want to
use it.

Thanks,

Mark

Re: [PATCH 1/2] libdw: ensure read_encoded_value's value is set

[Petr Machata]

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

Josh Stone <jistone@redhat.com> writes:

> With CFLAGS='-Og -g', F21 gcc hits -Werror=maybe-uninitialized in
> read_encoded_value at "*result += value".  It's fine with -O2/-O0.
>
> By my inspection, the only way those don't set value are for error
> cases, which will then return immediately.

Looks good to me.  I don't see how we could end up using value
uninitialized either, but oh well.

Thanks,
Petr