[Bug tdep/26280] New: [s390, Wmaybe-uninitialized] Warning in gdb/s390-tdep.c

[Bug tdep/26280] New: [s390, Wmaybe-uninitialized] Warning in gdb/s390-tdep.c

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=26280

            Bug ID: 26280
           Summary: [s390, Wmaybe-uninitialized] Warning in
                    gdb/s390-tdep.c
           Product: gdb
           Version: HEAD
            Status: NEW
          Severity: normal
          Priority: P2
         Component: tdep
          Assignee: unassigned at sourceware dot org
          Reporter: vries at gcc dot gnu.org
  Target Milestone: ---

Warning with -Wall:
...
src/gdb/s390-tdep.c: In function 'void s390_displaced_step_fixup(gdbarch*,
displaced_step_closure*, CORE_ADDR, CORE_ADDR, regcache*)':
src/gdb/s390-tdep.c:528:30: warning: 'r2' may be used uninitialized in this
function [-Wmaybe-uninitialized]
  528 |       if (insn[0] == op_basr && r2 == 0)
      |           ~~~~~~~~~~~~~~~~~~~^~~~~~~~~~
...

Code:
...
  unsigned int b2, r1, r2, x2, r3;

  ...

  /* Handle absolute branch and save instructions.  */
  if (is_rr (insn, op_basr, &r1, &r2)
      || is_rx (insn, op_bas, &r1, &d2, &x2, &b2))
    {
      /* Recompute saved return address in R1.  */
      regcache_cooked_write_unsigned (regs, S390_R0_REGNUM + r1,
                                      amode | (from + insnlen));
      /* Update PC iff the instruction doesn't actually branch.  */
      if (insn[0] == op_basr && r2 == 0)
        regcache_write_pc (regs, from + insnlen);
    }
...

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug tdep/26280] [s390, Wmaybe-uninitialized] Warning in gdb/s390-tdep.c

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=26280

Tom de Vries <vries at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |arnez at linux dot ibm.com,
                   |                            |uweigand at sourceware dot org
             Target|                            |s390

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug tdep/26280] [s390, Wmaybe-uninitialized] Warning in gdb/s390-tdep.c

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=26280

--- Comment #1 from Tom de Vries <vries at gcc dot gnu.org> ---
Tentative patch, fixes warning:
...
diff --git a/gdb/s390-tdep.c b/gdb/s390-tdep.c
index 0ee7a37256..ce110fc41e 100644
--- a/gdb/s390-tdep.c
+++ b/gdb/s390-tdep.c
@@ -518,14 +518,15 @@ s390_displaced_step_fixup (struct gdbarch *gdbarch,
                        paddress (gdbarch, pc), insnlen, (int) amode);

   /* Handle absolute branch and save instructions.  */
-  if (is_rr (insn, op_basr, &r1, &r2)
+  bool op_basr_p = is_rr (insn, op_basr, &r1, &r2);
+  if (op_basr_p
       || is_rx (insn, op_bas, &r1, &d2, &x2, &b2))
     {
       /* Recompute saved return address in R1.  */
       regcache_cooked_write_unsigned (regs, S390_R0_REGNUM + r1,
                                      amode | (from + insnlen));
       /* Update PC iff the instruction doesn't actually branch.  */
-      if (insn[0] == op_basr && r2 == 0)
+      if (op_basr_p && r2 == 0)
        regcache_write_pc (regs, from + insnlen);
     }

...

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug tdep/26280] [s390, Wmaybe-uninitialized] Warning in gdb/s390-tdep.c

[arnez at linux dot ibm.com]

https://sourceware.org/bugzilla/show_bug.cgi?id=26280

--- Comment #2 from Andreas Arnez <arnez at linux dot ibm.com> ---
(In reply to Tom de Vries from comment #1)
> Tentative patch, fixes warning:
> ...
The patch looks OK to me.  Note that is_rr() returns an int instead of a bool;
but I can live with the implicit conversion here.

So it seems that you want to get rid of all maybe-uninitialized warnings in
GDB?  Does this apply to all such warnings people encounter in their builds? 
If that's the goal, I should probably open bugs for any occurrences I see when
compiling for s390x, right?

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug tdep/26280] [s390, Wmaybe-uninitialized] Warning in gdb/s390-tdep.c

[cvs-commit at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=26280

--- Comment #3 from cvs-commit at gcc dot gnu.org <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Tom de Vries <vries@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=8ba83e9109a1a02f54f1c4793c8f231bb9256acd

commit 8ba83e9109a1a02f54f1c4793c8f231bb9256acd
Author: Tom de Vries <tdevries@suse.de>
Date:   Wed Jul 29 09:03:20 2020 +0200

    [tdep/s390] Fix Wmaybe-uninitialized in s390_displaced_step_fixup

    When building gdb with CFLAGS/CXXFLAGS="-O2 -g -Wall", I see:
    ...
    src/gdb/s390-tdep.c: In function 'void s390_displaced_step_fixup(gdbarch*,
\
      displaced_step_closure*, CORE_ADDR, CORE_ADDR, regcache*)':
    src/gdb/s390-tdep.c:528:30: warning: 'r2' may be used uninitialized in this
\
      function [-Wmaybe-uninitialized]
      528 |       if (insn[0] == op_basr && r2 == 0)
          |           ~~~~~~~~~~~~~~~~~~~^~~~~~~~~~
    ...

    The problem is that the compiler is unaware that
    'is_rr (insn, op_basr, &r1, &r2) == 1' ensures that 'insn[0] == op_basr':
    ...
      if (is_rr (insn, op_basr, &r1, &r2)
          || is_rx (insn, op_bas, &r1, &d2, &x2, &b2))
        {
          /* Recompute saved return address in R1.  */
          regcache_cooked_write_unsigned (regs, S390_R0_REGNUM + r1,
                                          amode | (from + insnlen));
          /* Update PC iff the instruction doesn't actually branch.  */
          if (insn[0] == op_basr && r2 == 0)
            regcache_write_pc (regs, from + insnlen);
        }
    ...

    Fix this by storing the result of the call, and using it instead of
    'insn[0] ==op_basr'.

    Build on x86_64-linux with
--enable-targets=s390-suse-linux,s390x-suse-linux.

    gdb/ChangeLog:

    2020-07-29  Tom de Vries  <tdevries@suse.de>

            PR tdep/26280
            * s390-tdep.c (s390_displaced_step_fixup): Fix
Wmaybe-uninitialized.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug tdep/26280] [s390, Wmaybe-uninitialized] Warning in gdb/s390-tdep.c

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=26280

--- Comment #4 from Tom de Vries <vries at gcc dot gnu.org> ---
(In reply to Andreas Arnez from comment #2)
> (In reply to Tom de Vries from comment #1)
> > Tentative patch, fixes warning:
> > ...
> The patch looks OK to me.  Note that is_rr() returns an int instead of a
> bool; but I can live with the implicit conversion here.
> 

Thanks for the review.  I've updated the var type to int, and committed.

> So it seems that you want to get rid of all maybe-uninitialized warnings in
> GDB?

It's just that this is one of the few warnings that I see with -Wall, so I
think it makes sense to fix it, given that it's easy to do:
...
$ grep -ic warning: build/MAKELOG 
10
...

> Does this apply to all such warnings people encounter in their builds?
> If that's the goal, I should probably open bugs for any occurrences I see
> when compiling for s390x, right?

I'm not sure if opening bugs for warnings that don't break the build is
required, it's just something I did because I was not able to fix it on the
spot and wanted to make sure I did not forget about it.

Ideally, builds should be warning-free, but it has been acknowledged that
-Wmaybe-uninitialized has issues (bugs and false positives) and therefore we
compile with -Wno-error=maybe-uninitialized.  But, if it's easy to fix the
warning, we probably should.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug tdep/26280] [s390, Wmaybe-uninitialized] Warning in gdb/s390-tdep.c

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=26280

Tom de Vries <vries at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED
   Target Milestone|---                         |10.1

--- Comment #5 from Tom de Vries <vries at gcc dot gnu.org> ---
Patch working around warning committed, marking resolved-fixed.

-- 
You are receiving this mail because:
You are on the CC list for the bug.