From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 26328 invoked by alias); 24 May 2012 01:12:54 -0000 Received: (qmail 26309 invoked by uid 22791); 24 May 2012 01:12:51 -0000 X-SWARE-Spam-Status: No, hits=-6.0 required=5.0 tests=AWL,BAYES_00,KAM_STOCKGEN,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,SPF_HELO_PASS,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 24 May 2012 01:12:32 +0000 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q4O1CKsX015864 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 23 May 2012 21:12:20 -0400 Received: from valrhona.uglyboxes.com (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id q4O1CGGt027924 (version=TLSv1/SSLv3 cipher=DHE-RSA-CAMELLIA256-SHA bits=256 verify=NO); Wed, 23 May 2012 21:12:18 -0400 Message-ID: <4FBD8AF0.2010009@redhat.com> Date: Thu, 24 May 2012 01:12:00 -0000 From: Keith Seitz User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:11.0) Gecko/20120329 Thunderbird/11.0.1 MIME-Version: 1.0 To: Roland Schwingel CC: insight@sourceware.org Subject: Re: [PATCH/gdbtk] Update stackwin gdb api usage References: <4F79C3F2.3020908@onevision.com> In-Reply-To: <4F79C3F2.3020908@onevision.com> Content-Type: text/plain; charset=ISO-8859-15; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes Mailing-List: contact insight-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: insight-owner@sourceware.org X-SW-Source: 2012-q2/txt/msg00026.txt.bz2 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 > #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 ("", -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 ("", -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