Re: [PATCH/gdbtk] Update stackwin gdb api usage

[Keith Seitz <keiths@redhat.com>]

On 04/02/2012 08:21 AM, Roland Schwingel wrote:
>
> A general question:
> The direction of stack dump in insight is opposite to when issueing a
> backtrace inside gdb itself. Is this on purpose? Wouldn't it be nicer
> when insight's stack window would show the stack in the same order?

Yes, that was a design decision made many, many years ago. I don't have 
a preference to sort the stack "properly." If you find the time to write 
up a patch, I would consider/approve it.

> diff -ruNp gdbtk_orig/generic/gdbtk-stack.c gdbtk/generic/gdbtk-stack.c
> --- gdbtk_orig/generic/gdbtk-stack.c	2012-03-30 09:29:15.000000000 +0200
> +++ gdbtk/generic/gdbtk-stack.c	2012-04-02 16:13:11.351540500 +0200
> @@ -1,5 +1,6 @@
>   /* Tcl/Tk command definitions for Insight - Stack.
> -   Copyright (C) 2001-2012 Free Software Foundation, Inc.
> +   Copyright (C) 2001, 2002, 2003, 2008, 2011
> +   Free Software Foundation, Inc.
>
>      This file is part of GDB.
>

You already did this. :-)

> @@ -26,6 +27,10 @@
>   #include "dictionary.h"
>   #include "varobj.h"
>   #include "arch-utils.h"
> +#include "stack.h"
> +#ifndef PC_SOLIB
> +#include "solib.h"
> +#endif
>
>   #include<tcl.h>
>   #include "gdbtk.h"

I believe the proper etiquette in gdb is to simply include solib.h 
unconditionally. It should not hurt anything. I tested this on mingw32 
and x86_64-linux with no troubles.

> @@ -475,24 +480,24 @@ gdb_stack (ClientData clientData, Tcl_In
>         /* Find the outermost frame */
>         r  = GDB_get_current_frame (&fi);
>         if (r != GDB_OK)
> -	return TCL_ERROR;
> +        return TCL_ERROR;
>
>         while (fi != NULL)
>           {
>             top = fi;
> -	  r = GDB_get_prev_frame (fi,&fi);
> -	  if (r != GDB_OK)
> -	    fi = NULL;
> +          r = GDB_get_prev_frame (fi,&fi);
> +          if (r != GDB_OK)
> +            fi = NULL;
>           }
>
> +      result_ptr->obj_ptr = Tcl_NewListObj (0, NULL);
> +
>         /* top now points to the top (outermost frame) of the
>            stack, so point it to the requested start */
>         start = -start;
> -      r = GDB_find_relative_frame (top,&start,&top);
> -
> -      result_ptr->obj_ptr = Tcl_NewListObj (0, NULL);
> +      r = GDB_find_relative_frame (top,&start,&top);

Weird. My mail agent has removed the spaces after the commas. In the 
patch, all looks well. There does appear to be some whitespace trailing 
the "r = GDB_find_relative_frame (...);" line, though. Please 
double-check and remove if it is actually there. [and not another trick 
of my mail agent]

>         if (r != GDB_OK)
> -	return TCL_OK;
> +        return TCL_OK;
>
>         /* If start != 0, then we have asked to start outputting
>            frames beyond the innermost stack frame */
> @@ -503,8 +508,8 @@ gdb_stack (ClientData clientData, Tcl_In
>               {
>                 get_frame_name (interp, result_ptr->obj_ptr, fi);
>                 r = GDB_get_next_frame (fi,&fi);
> -	      if (r != GDB_OK)
> -		break;
> +              if (r != GDB_OK)
> +               break;

"break;" doesn't appear indented properly, though. Please double-check.

>               }
>           }
>       }

This rest of the whitespace change is okay, but please submit these 
sorts of patches separate from more substantial patches. It is a lot 
easier for me to review without that cluttering up the patch.

> @@ -515,14 +520,14 @@ gdb_stack (ClientData clientData, Tcl_In
>   /* A helper function for get_stack which adds information about
>    * the stack frame FI to the caller's LIST.
>    *
> - * This is stolen from print_frame_info in stack.c.
> + * This is stolen from print_frame_info/print_frame in stack.c.
>    */
> +
>   static void
>   get_frame_name (Tcl_Interp *interp, Tcl_Obj *list, struct frame_info *fi)
>   {
> -  struct symtab_and_line sal;
>     struct symbol *func = NULL;
> -  const char *funname = 0;
> +  const char *funname = NULL;
>     enum language funlang = language_unknown;
>     Tcl_Obj *objv[1];
>
> @@ -532,75 +537,39 @@ get_frame_name (Tcl_Interp *interp, Tcl_
>         Tcl_ListObjAppendElement (interp, list, objv[0]);
>         return;
>       }
> -  if ((get_frame_type (fi) == SIGTRAMP_FRAME))
> +  if (get_frame_type (fi) == SIGTRAMP_FRAME)
>       {
>         objv[0] = Tcl_NewStringObj ("<signal handler called>", -1);
>         Tcl_ListObjAppendElement (interp, list, objv[0]);
>         return;
>       }
> -
> -  sal =
> -    find_pc_line (get_frame_pc (fi),
> -		  get_next_frame (fi) != NULL
> -		&&  !(get_frame_type (fi) == SIGTRAMP_FRAME)
> -		&&  !(get_frame_type (fi) == DUMMY_FRAME));
> -
> -  func = find_pc_function (get_frame_pc (fi));
> -  if (func)
> -    {
> -      struct minimal_symbol *msymbol = lookup_minimal_symbol_by_pc (get_frame_pc (fi));
> -      if (msymbol != NULL
> -	&&  (SYMBOL_VALUE_ADDRESS (msymbol)
> -	>  BLOCK_START (SYMBOL_BLOCK_VALUE (func))))
> -	{
> -	  func = 0;
> -	  funname = GDBTK_SYMBOL_SOURCE_NAME (msymbol);
> -	  funlang = SYMBOL_LANGUAGE (msymbol);
> -	}
> -      else
> -	{
> -	  funname = GDBTK_SYMBOL_SOURCE_NAME (func);
> -	  funlang = SYMBOL_LANGUAGE (func);
> -	}
> -    }
> -  else
> +  if (get_frame_type (fi) == ARCH_FRAME)
>       {
> -      struct minimal_symbol *msymbol = lookup_minimal_symbol_by_pc (get_frame_pc (fi));
> -      if (msymbol != NULL)
> -	{
> -	  funname = GDBTK_SYMBOL_SOURCE_NAME (msymbol);
> -	  funlang = SYMBOL_LANGUAGE (msymbol);
> -	}
> +      objv[0] = Tcl_NewStringObj ("<cross-architecture call>", -1);
> +      Tcl_ListObjAppendElement (interp, list, objv[0]);
> +      return;
>       }
>
> -  if (sal.symtab)
> +  find_frame_funname (fi,&funname,&funlang,&func);

Argh! Darn mail agent! I apologize ahead of time if I ask you to watch 
out for this, and it turns out to be my mail agent and not you.

> +
> +  if (funname)
>       {
>         objv[0] = Tcl_NewStringObj (funname, -1);
>         Tcl_ListObjAppendElement (interp, list, objv[0]);
>       }
>     else
>       {
> -#if 0
> -      /* we have no convenient way to deal with this yet... */
> -      if (fi->pc != sal.pc || !sal.symtab)
> -	{
> -	  deprecated_print_address_numeric (fi->pc, 1, gdb_stdout);
> -	  printf_filtered (" in ");
> -	}
> -      printf_symbol_filtered (gdb_stdout, funname ? funname : "??", funlang,
> -			      DMGL_ANSI);
> -#endif
> -      objv[0] = Tcl_NewStringObj (funname != NULL ? funname : "??", -1);
> +    	char *lib = NULL;

Can you double-check the indent level on the above line? It doesn't look 
right. [Apologies again if my mail agent maligned it.]

> +      objv[0] = Tcl_NewStringObj (funname ? funname : "??", -1);
>   #ifdef PC_SOLIB
> -      if (!funname)
> -	{
> -	  char *lib = PC_SOLIB (get_frame_pc (fi));
> -	  if (lib)
> -	    {
> -	      Tcl_AppendStringsToObj (objv[0], " from ", lib, (char *) NULL);
> -	    }
> -	}
> +      lib = PC_SOLIB (get_frame_pc (fi));
> +#else
> +      lib = solib_name_from_address (get_frame_program_space (fi),
> +        get_frame_pc (fi));

GNU coding standards prefer that "get_frame_pc (fi));" be aligned 
underneath "get_frame_program_space (fi),".

>   #endif

This has nothing to do with your patch other than the fact that you are 
touching this, but could you check the indentation on "#endif" while 
you're touching this code? It looks a little off.

> +      if (lib)
> +        Tcl_AppendStringsToObj (objv[0], " from ", lib, (char *) NULL);
> +
>         Tcl_ListObjAppendElement (interp, list, objv[0]);
>       }
>   }
> diff -ruNp gdbtk_orig/library/stackwin.itb gdbtk/library/stackwin.itb
> --- gdbtk_orig/library/stackwin.itb	2008-02-09 02:23:42.000000000 +0100
> +++ gdbtk/library/stackwin.itb	2012-04-02 09:40:19.507691900 +0200
> @@ -1,5 +1,5 @@
>   # Stack window for Insight.
> -# Copyright (C) 1997, 1998, 1999, 2002, 2003, 2008 Red Hat
> +# Copyright (C) 1997-2012 Red Hat, Inc.
>   #
>   # This program is free software; you can redistribute it and/or modify it
>   # under the terms of the GNU General Public License (GPL) as published by
> @@ -68,20 +68,14 @@ itcl::body StackWin::update {event} {
>         set frames {}
>       }
>
> +    $itk_component(slb) delete 0 end
>       if {[llength $frames] == 0} {
> -      $itk_component(slb) delete 0 end
>         $itk_component(slb) insert end {NO STACK}
>         return
>       }
>
> -    $itk_component(slb) delete 0 end
>       set levels 0
>       foreach frame $frames {
> -      set len [string length $frame]
> -
> -      if {$len>  $maxwidth} {
> -	set maxwidth $len
> -      }
>         $itk_component(slb) insert end $frame
>         incr levels
>       }
> diff -ruNp gdbtk_orig/library/stackwin.ith gdbtk/library/stackwin.ith
> --- gdbtk_orig/library/stackwin.ith	2005-12-23 19:26:50.000000000 +0100
> +++ gdbtk/library/stackwin.ith	2012-04-02 09:40:23.623824000 +0200
> @@ -1,5 +1,5 @@
>   # Stack window class definition for GDBtk.
> -# Copyright (C) 1997, 1998, 1999 Cygnus Solutions
> +# Copyright (C) 1997-2012 Red Hat, Inc.
>   #
>   # This program is free software; you can redistribute it and/or modify it
>   # under the terms of the GNU General Public License (GPL) as published by
> @@ -20,7 +20,6 @@ itcl::class StackWin {
>     inherit EmbeddedWin GDBWin
>
>     private {
> -    variable maxwidth 40
>       variable Running 0
>       variable protect_me 0
>       method build_win {}
>

Ok with those minor changes.

Thanks!
Keith