[RFA/commit] Uninitialize variable "this_id" in frame.c:get_prev_frame_1.

[RFA/commit] Uninitialize variable "this_id" in frame.c:get_prev_frame_1.

[Joel Brobecker]

Hello,

With a simple Ada program where I have 3 functions, one just calling
the next, the backtrace is currently broken when GDB is compiled
at -O2:

   #0  hello.first () at hello.adb:5
   #1  0x0000000100001475 in hello.second () at hello.adb:10
   Backtrace stopped: previous frame inner to this frame (corrupt stack?)

It turns out that a recent patch deleted the assignment of variable
this_id, making it an unitialized variable:

        * frame-unwind.c (default_frame_unwind_stop_reason): Return
        UNWIND_OUTERMOST if the frame's ID is outer_frame_id.
        * frame.c (get_prev_frame_1): Remove outer_frame_id check.

The hunk in question starts with:

-  /* Check that this frame is not the outermost.  If it is, don't try
-     to unwind to the prev frame.  */
-  this_id = get_frame_id (this_frame);
-  if (frame_id_eq (this_id, outer_frame_id))

(the code was removed as redundant - but removing the assignment
may not have been intentional).

There is no other code in this function that sets the variable.
Instead of re-adding the statement in the lone section where it is
actually used, I inlined it, and then got rid of the variable
altogether.  This way, and until we start needing this frame ID
in another location within that function, we dont' have to worry
about the variable's validity/lifetime.

gdb/ChangeLog:

        * frame.c (get_prev_frame_1): Delete variable "this_id".
        Replace its use by a call to get_frame_id.

Tested on ia64-linux (where the problem is visible) and x86_64-linux
(where it, surprisingly, is not). In both cases, I did the testing
with a GDB compiled at -O2, because I could not reproduce the behavior
at lower optimization levels.

I want to call it obvious, but I have a slight hesitation, especially
since I did not take the time to figure out how it can possibly work
at all. Maybe just pure luck?

OK to commit?

Thanks,
-- 
Joel

---
 gdb/frame.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdb/frame.c b/gdb/frame.c
index db94d98..576c969 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -1719,7 +1719,6 @@ get_prev_frame_if_no_cycle (struct frame_info *this_frame)
 static struct frame_info *
 get_prev_frame_1 (struct frame_info *this_frame)
 {
-  struct frame_id this_id;
   struct gdbarch *gdbarch;
 
   gdb_assert (this_frame != NULL);
@@ -1791,7 +1790,8 @@ get_prev_frame_1 (struct frame_info *this_frame)
      See the comment at frame_id_inner for details.  */
   if (get_frame_type (this_frame) == NORMAL_FRAME
       && this_frame->next->unwind->type == NORMAL_FRAME
-      && frame_id_inner (get_frame_arch (this_frame->next), this_id,
+      && frame_id_inner (get_frame_arch (this_frame->next),
+			 get_frame_id (this_frame),
 			 get_frame_id (this_frame->next)))
     {
       CORE_ADDR this_pc_in_block;
-- 
1.8.1.2

Re: [RFA/commit] Uninitialize variable "this_id" in frame.c:get_prev_frame_1.

[Pedro Alves]

On 12/05/2013 04:45 PM, Joel Brobecker wrote:
> Hello,
> 
> With a simple Ada program where I have 3 functions, one just calling
> the next, the backtrace is currently broken when GDB is compiled
> at -O2:
> 
>    #0  hello.first () at hello.adb:5
>    #1  0x0000000100001475 in hello.second () at hello.adb:10
>    Backtrace stopped: previous frame inner to this frame (corrupt stack?)
> 
> It turns out that a recent patch deleted the assignment of variable
> this_id, making it an unitialized variable:
> 
>         * frame-unwind.c (default_frame_unwind_stop_reason): Return
>         UNWIND_OUTERMOST if the frame's ID is outer_frame_id.
>         * frame.c (get_prev_frame_1): Remove outer_frame_id check.

Whoops, sorry about that.

> 
> The hunk in question starts with:
> 
> -  /* Check that this frame is not the outermost.  If it is, don't try
> -     to unwind to the prev frame.  */
> -  this_id = get_frame_id (this_frame);
> -  if (frame_id_eq (this_id, outer_frame_id))
> 
> (the code was removed as redundant - but removing the assignment
> may not have been intentional).

Certainly not.

> 
> There is no other code in this function that sets the variable.
> Instead of re-adding the statement in the lone section where it is
> actually used, I inlined it, and then got rid of the variable
> altogether.  This way, and until we start needing this frame ID
> in another location within that function, we dont' have to worry
> about the variable's validity/lifetime.
> 
> gdb/ChangeLog:
> 
>         * frame.c (get_prev_frame_1): Delete variable "this_id".
>         Replace its use by a call to get_frame_id.

Thanks, this is definitely OK.

> Tested on ia64-linux (where the problem is visible) and x86_64-linux
> (where it, surprisingly, is not). In both cases, I did the testing
> with a GDB compiled at -O2, because I could not reproduce the behavior
> at lower optimization levels.
> 
> I want to call it obvious, but I have a slight hesitation, especially
> since I did not take the time to figure out how it can possibly work
> at all. Maybe just pure luck?

Certainly just luck.  In my case, Seem I'm always hitting:

static int
frame_id_inner (struct gdbarch *gdbarch, struct frame_id l, struct frame_id r)
{
  int inner;

  if (!l.stack_addr_p || !r.stack_addr_p)
    /* Like NaN, any operation involving an invalid ID always fails.  */
    inner = 0;      <<<<<<<<<<<<

-- 
Pedro Alves

pushed: [RFA/commit] Uninitialize variable "this_id" in frame.c:get_prev_frame_1.

[Joel Brobecker]

> > gdb/ChangeLog:
> > 
> >         * frame.c (get_prev_frame_1): Delete variable "this_id".
> >         Replace its use by a call to get_frame_id.
> 
> Thanks, this is definitely OK.

Thanks! The patch is now in. I fixed a missing 'd' in "Uninitialized",
and changed also the text to say that the removal was not intentional
(instead of "may not have been").

-- 
Joel