diff mbox

Fix sometimes-uninitialized warning in gdbscm_value_address

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

Commit Message

Simon Marchi Sept. 21, 2017, 1:08 p.m. UTC 
  I am getting this warning with clang:

/home/emaisin/src/binutils-gdb/gdb/guile/scm-value.c:439:11: error: variable 'address' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
      if (res_val != NULL)
          ^~~~~~~~~~~~~~~
/home/emaisin/src/binutils-gdb/gdb/guile/scm-value.c:444:32: note: uninitialized use occurs here
      if (gdbscm_is_exception (address))
                               ^~~~~~~
/home/emaisin/src/binutils-gdb/gdb/guile/scm-value.c:439:7: note: remove the 'if' if its condition is always true
      if (res_val != NULL)
      ^~~~~~~~~~~~~~~~~~~~
/home/emaisin/src/binutils-gdb/gdb/guile/scm-value.c:427:18: note: initialize the variable 'address' to silence this warning
      SCM address;
                 ^
                  = nullptr

We can get rid of it with a small refactoring.  I think it's a bit
cleaner/safer to initialize address with a pessimistic value and assign
it on success.  Then there's no chance of using it uninitialized.  If I
understand correctly, the NULL check on res_val was to check whether
value_addr threw, and that if value_addr returns without throwing, the
result will never be NULL.  If that's true, we can skip the res_val
variable.

Tested by running gdb.guile/*.exp locally.

gdb/ChangeLog:

	* guile/scm-value.c (gdbscm_value_address): Initialize address,
	get rid of res_val.
---
 gdb/guile/scm-value.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

Comments

Pedro Alves Sept. 21, 2017, 2:30 p.m. UTC | #1 
On 09/21/2017 02:08 PM, Simon Marchi wrote:

> We can get rid of it with a small refactoring.  I think it's a bit
> cleaner/safer to initialize address with a pessimistic value and assign
> it on success.  Then there's no chance of using it uninitialized.  If I
> understand correctly, the NULL check on res_val was to check whether
> value_addr threw, and that if value_addr returns without throwing, the
> result will never be NULL.  If that's true, we can skip the res_val
> variable.

I think this is fine, though I'm curious about whether marking
value_addr with the returns_nonnull attribute would quiet
the warning.

Thanks,
Pedro Alves
Simon Marchi Sept. 21, 2017, 2:44 p.m. UTC | #2 
On 2017-09-21 16:30, Pedro Alves wrote:
> On 09/21/2017 02:08 PM, Simon Marchi wrote:
> 
>> We can get rid of it with a small refactoring.  I think it's a bit
>> cleaner/safer to initialize address with a pessimistic value and 
>> assign
>> it on success.  Then there's no chance of using it uninitialized.  If 
>> I
>> understand correctly, the NULL check on res_val was to check whether
>> value_addr threw, and that if value_addr returns without throwing, the
>> result will never be NULL.  If that's true, we can skip the res_val
>> variable.
> 
> I think this is fine, though I'm curious about whether marking
> value_addr with the returns_nonnull attribute would quiet
> the warning.

I tried, it didn't.

Thanks for reviewing, the patch is now pushed.

Simon
diff mbox

Patch

diff --git a/gdb/guile/scm-value.c b/gdb/guile/scm-value.c
index 0dc6630..3732666 100644
--- a/gdb/guile/scm-value.c
+++ b/gdb/guile/scm-value.c
@@ -421,24 +421,19 @@  gdbscm_value_address (SCM self)
 
   if (SCM_UNBNDP (v_smob->address))
     {
-      struct value *res_val = NULL;
       struct cleanup *cleanup
 	= make_cleanup_value_free_to_mark (value_mark ());
-      SCM address;
+      SCM address = SCM_BOOL_F;
 
       TRY
 	{
-	  res_val = value_addr (value);
+	  address = vlscm_scm_from_value (value_addr (value));
 	}
       CATCH (except, RETURN_MASK_ALL)
 	{
-	  address = SCM_BOOL_F;
 	}
       END_CATCH
 
-      if (res_val != NULL)
-	address = vlscm_scm_from_value (res_val);
-
       do_cleanups (cleanup);
 
       if (gdbscm_is_exception (address))