From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 14258 invoked by alias); 24 May 2012 23:30:27 -0000 Received: (qmail 14245 invoked by uid 22791); 24 May 2012 23:30:26 -0000 X-SWARE-Spam-Status: No, hits=-6.7 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,SPF_HELO_PASS,TW_BT,TW_DB,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 23:30:08 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q4ONTuRs003849 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 24 May 2012 19:29:56 -0400 Received: from valrhona.uglyboxes.com (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id q4ONTr0s005344 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Thu, 24 May 2012 19:29:55 -0400 Message-ID: <4FBEC471.3000501@redhat.com> Date: Thu, 24 May 2012 23:30: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] Implement "Attach to process" for windows References: <4F859F3B.2090709@onevision.com> In-Reply-To: <4F859F3B.2090709@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/msg00031.txt.bz2 On 04/11/2012 08:11 AM, Roland Schwingel wrote: > One thing I was always missing in windows versions of insight was to > nicely attach to a running process. So I took the time now and > implemented it. The attached patch does the job. Very cool. Thank you! > One last thing. The "Attach to process" menu entry is IMHO in the wrong > menu. At present it is in the "Run" menu. To me it is more intuitive if > it would be in the "File" menu. If nobody objects I would generate a > seperate patch to move it in the "File" menu. Yeah, I agree. I don't know how it ended up there. Again, some minor nits below. > --- gdbtk_orig/library/attachdlg.itb 2008-02-09 02:23:42.000000000 +0100 > +++ gdbtk/library/attachdlg.itb 2012-04-11 16:51:23.387762900 +0200 > @@ -1,5 +1,5 @@ > # Attach Dialog for Insight. > -# Copyright (C) 1999, 2002, 2003, 2008 Red Hat, Inc. > +# Copyright (C) 1999-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 > @@ -152,51 +152,66 @@ itcl::body AttachDlg::choose_symbol_file > > > # ------------------------------------------------------------------ > -# METHOD: list_pids - List the available processes. Right now, > -# this just spawns ps, which means we have to deal with > -# all the different ps flags& output formats. At some > -# point we should steal some C code to do it by hand. > +# METHOD: list_pids - List the available processes. > +# Right now on *nix systems this just spawns ps, > +# which means we have to deal with all the different > +# ps flags& output formats. At some point we should > +# steal some C code to do it by hand. > # ------------------------------------------------------------------ There appear to be some superfluous whitespace at the end of several of the above lines. Please double-check. > > itcl::body AttachDlg::list_pids {{pattern *}} { > global gdbtk_platform > > - switch $gdbtk_platform(os) { > - Linux { > - set ps_cmd "ps axw" > + $itk_component(choose_pid) clear > + > + if {$gdbtk_platform(platform) == "windows"} { > + set processes [gdb_list_processes] > + foreach entry $processes { > + set executable [lindex $entry 1] > + if {[string match $pattern $executable]} { > + lappend pid_list $entry > + $itk_component(choose_pid) insert end $executable > + } > } > - default { > - set ps_cmd "ps w" In general, this is all good, but I would prefer that we call gdb_list_processes unconditionally first; if that fails/errors, then fall back to the "old" way (which doesn't even work on linux AFAICT). > + } else { > + switch $gdbtk_platform(os) { > + Linux { > + set ps_cmd "ps axw" > + } > + default { > + set ps_cmd "ps w" > + } > } > - } > - if {[catch {::open "|$ps_cmd" r} psH]} { > - set errTxt "Could not exec ps: $psH > + > + if {[catch {::open "|$ps_cmd" r} psH]} { > + set errTxt "Could not exec ps: $psH > You will have to enter the PID by hand." > - ManagedWin::open WarningDlg -message [list $errTxt] > - return > - } > - gets $psH header > + ManagedWin::open WarningDlg -message [list $errTxt] > + return > + } > + gets $psH header > > - set nfields [llength $header] > - set nfields_m_1 [expr {$nfields - 1}] > - set regexp {^ *([^ ]*) +} > - for {set i 1} {$i< $nfields_m_1} {incr i} { > - append regexp {[^ ]* +} > - } > - append regexp {(.*)$} > - > - $itk_component(choose_pid) clear > - set pid_list {} > + set nfields [llength $header] > + set nfields_m_1 [expr {$nfields - 1}] > + set regexp {^ *([^ ]*) +} > + for {set i 1} {$i< $nfields_m_1} {incr i} { > + append regexp {[^ ]* +} > + } > + append regexp {(.*)$} > + > + set pid_list {} > > - while {[gets $psH line]>= 0} { > - regexp $regexp $line dummy PID COMMAND > - if {[string match $pattern $COMMAND]} { > - lappend pid_list [list $PID $COMMAND] > - $itk_component(choose_pid) insert end $COMMAND > + while {[gets $psH line]>= 0} { > + regexp $regexp $line dummy PID COMMAND > + if {[string match $pattern $COMMAND]} { > + lappend pid_list [list $PID $COMMAND] > + $itk_component(choose_pid) insert end $COMMAND > + } > } > + > + close $psH > } > > - close $psH > $itk_component(choose_pid) selection set 0 > select_pid > } > --- gdbtk_orig/library/srcbar.itcl 2008-02-09 02:23:42.000000000 +0100 > +++ gdbtk/library/srcbar.itcl 2012-04-11 16:50:45.448496200 +0200 > @@ -1,5 +1,5 @@ > # SrcBar > -# Copyright (C) 2001, 2002, 2004, 2008 Red Hat, Inc. > +# Copyright (C) 2001-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 > @@ -230,16 +230,10 @@ itcl::class SrcBar { > > set is_native [TargetSelection::native_debugging] > > - # If we are on a Unix target, put in the attach options. "ps" doesn't > - # give me the Windows PID yet, and the attach also seems flakey, so > - # I will hold off on the Windows implementation for now. > - > if {$is_native} { > - if {[string compare $::gdbtk_platform(platform) windows] != 0} { > $Menu add command Attach "Attach to process" \ > [code $this do_attach $run_menu] \ > -underline 0 -accelerator "Ctrl+A" > - } > } else { > $Menu add command Other "Connect to target" \ > "$this do_connect $run_menu" -underline 0 > --- gdbtk_orig/generic/gdbtk-cmds.c 2012-03-30 09:14:33.000000000 +0200 > +++ gdbtk/generic/gdbtk-cmds.c 2012-04-11 16:51:59.423826200 +0200 > @@ -92,6 +92,11 @@ > #include /* for isprint() */ > #endif > > +#ifdef _WIN32 > +#include /* For gdb_list_processes() */ > +#include > +#endif > + > /* Various globals we reference. */ > extern char *source_path; > > @@ -225,6 +230,8 @@ static int perror_with_name_wrapper (PTR > static int wrapped_call (PTR opaque_args); > static int hex2bin (const char *hex, char *bin, int count); > static int fromhex (int a); > +static int gdb_list_processes (ClientData, Tcl_Interp *, int, Tcl_Obj * CONST[]); > + This line is too long and needs to be wrapped. > > > /* Gdbtk_Init > @@ -293,6 +300,8 @@ Gdbtk_Init (Tcl_Interp *interp) > gdb_get_inferior_args, NULL); > Tcl_CreateObjCommand (interp, "gdb_set_inferior_args", gdbtk_call_wrapper, > gdb_set_inferior_args, NULL); > + Tcl_CreateObjCommand (interp, "gdb_list_processes", gdbtk_call_wrapper, > + gdb_list_processes, NULL); > > /* gdb_context is used for debugging multiple threads or tasks */ > Tcl_LinkVar (interp, "gdb_context_id", > @@ -591,6 +600,60 @@ gdb_stop (ClientData clientData, Tcl_Int > > return TCL_OK; > } > + > +/* > + * This command lists all processes in a system. Yet only implemented > + * for windows as the *nix part is handled directly from tcl code. > + * > + * Arguments: > + * None > + * Tcl Result: > + * A list of 2 elemented lists containing all running processes and their pids. > + */ This line is also too long. > + > +static int > +gdb_list_processes (ClientData clientData, Tcl_Interp *interp, > + int objc, Tcl_Obj * CONST objv[]) This is not indented correctly. "int" should fall directly under "ClientData" (per GNU coding standards). > +{ > + if (objc != 1) > + { > + Tcl_WrongNumArgs (interp, 1, objv, NULL); > + return TCL_ERROR; > + } > + > + Tcl_SetListObj (result_ptr->obj_ptr, 0, NULL); > + > + #ifdef _WIN32 > + { > + HANDLE processSnap = CreateToolhelp32Snapshot (TH32CS_SNAPPROCESS, 0); > + if (processSnap != INVALID_HANDLE_VALUE) > + { > + PROCESSENTRY32 processEntry; > + > + processEntry.dwSize = sizeof(PROCESSENTRY32); > + > + if (Process32First (processSnap,&processEntry)) > + { > + do > + { > + Tcl_Obj *pidProc[2]; > + pidProc[0] = Tcl_NewIntObj (processEntry.th32ProcessID); > + pidProc[1] = Tcl_NewStringObj (processEntry.szExeFile, -1); > + > + Tcl_ListObjAppendElement (NULL, result_ptr->obj_ptr, > + Tcl_NewListObj (2, pidProc)); > + > + } while(Process32Next (processSnap,&processEntry)); > + } Something really wrong with that indent level! :-) > + > + CloseHandle (processSnap); > + } > + } > + #endif > + > + return TCL_OK; > +} > + You have two choices here... 1) Leave this as-is, change attachdlg so that it checks if the return from gdb_list_processes is empty and fallback to groking ps. 2) return TCL_OK inside _WIN32 and return TCL_ERROR to force attachdlg to fallback to groking ps. I'm ambivalent about which. The hope is that one day, I'll implement this properly for (some) unixen (or at least linux), and if I ever do that, I won't have to muck with attachdlg at all. Keith > > /* >