From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17065 invoked by alias); 10 Jul 2013 12:57:43 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 17054 invoked by uid 89); 10 Jul 2013 12:57:43 -0000 X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL,BAYES_00,RCVD_IN_HOSTKARMA_NO,TW_CP autolearn=ham version=3.3.1 Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Wed, 10 Jul 2013 12:57:42 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 01E461C641D; Wed, 10 Jul 2013 08:57:41 -0400 (EDT) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id aUMZIxDa2m9X; Wed, 10 Jul 2013 08:57:40 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id AE1DB1C61A6; Wed, 10 Jul 2013 08:57:40 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id 146B8C1073; Wed, 10 Jul 2013 05:57:38 -0700 (PDT) Date: Wed, 10 Jul 2013 12:57:00 -0000 From: Joel Brobecker To: ali_anwar Cc: Pedro Alves , Tom Tromey , gdb-patches@sourceware.org Subject: Re: Updated patch for Bug 13217 - thread apply all detach throws a SEGFAULT Message-ID: <20130710125737.GF8063@adacore.com> References: <5062EA9B.1060505@codesourcery.com> <871uhnwkf3.fsf@fleche.redhat.com> <50C62BBB.6010404@codesourcery.com> <87hantek1p.fsf@fleche.redhat.com> <50C75319.9080903@codesourcery.com> <50C7628F.5080004@redhat.com> <51DD37FA.8010306@codesourcery.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <51DD37FA.8010306@codesourcery.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-SW-Source: 2013-07/txt/msg00273.txt.bz2 > I have tried to implement what you suggested in the attach patch. > Does it look reasonable? Putting my patch-champion hat on (reviewing the style without commenting on the actual code), I noticed: Missing ChangeLog entry. > +struct thread_array_cleanup { > + struct thread_info **tp_array; > + int count; > +}; Formatting of the opening curly brace: struct thread_array_cleanup { struct thread_info **tp_array; int count; }; There should be a comment documenting this struct and its fields. Typically, we have a global comment before the struct, and then one command before each field explaining the semantics of that field. A global comment is enough if it trivially documents the fields themselves. Here is an example: /* Our private data in struct so_list. */ struct lm_info { /* The name of the file mapped by the loader. Apart from the entry for the main executable, this is usually a shared library (which, on AIX, is an archive library file, created using the "ar" command). */ char *filename; /* The name of the shared object file with the actual dynamic loading dependency. This may be NULL (Eg. main executable). */ char *member_name; > +static void > +make_cleanup_thread_refcount (void *data) > +{ > + int k; > + struct thread_array_cleanup *ta_cleanup = data; > + for (k = 0; k != ta_cleanup->count; k++) > + ta_cleanup->tp_array[k]->refcount--; > +} Every new function should now be documented with a comment above the function. The GDB project also asks that an empty line between the documentation/comment and the start of the function definition be added. Example: /* Free the memory allocated for the given lm_info. */ static void solib_aix_xfree_lm_info (struct lm_info *info) { Another small nit: Empty line after local variable declarations. Thus: int k; struct thread_array_cleanup *ta_cleanup = data; for (k = 0; k != ta_cleanup->count; k++) > @@ -1176,13 +1191,13 @@ > thread apply 1 2 7 4 backtrace Apply backtrace cmd to threads 1,2,7,4 > thread apply 2-7 9 p foo(1) Apply p foo(1) cmd to threads 2->7 & 9 > thread apply all p x/i $pc Apply x/i $pc cmd to all threads. */ > - > static void Please do not delete that line. > thread_apply_all_command (char *cmd, int from_tty) > { > - struct thread_info *tp; > struct cleanup *old_chain; > char *saved_cmd; > + int tc; > + struct thread_array_cleanup ta_cleanup; > > if (cmd == NULL || *cmd == '\000') > error (_("Please specify a command following the thread ID list")); > @@ -1195,17 +1210,41 @@ > execute_command. */ > saved_cmd = xstrdup (cmd); > make_cleanup (xfree, saved_cmd); > - for (tp = thread_list; tp; tp = tp->next) > - if (thread_alive (tp)) > - { > - switch_to_thread (tp->ptid); > + tc = thread_count (); > > - printf_filtered (_("\nThread %d (%s):\n"), > - tp->num, target_pid_to_str (inferior_ptid)); > - execute_command (cmd, from_tty); > - strcpy (cmd, saved_cmd); /* Restore exact command used > - previously. */ > - } > + if (tc) > + { > + struct thread_info **tp_array; > + struct thread_info *tp; > + int i, k; > + i = 0; > + > + /* Save a copy of the thread_list in case we execute detach > + command. */ > + tp_array = xmalloc (sizeof (struct thread_info*) * tc); > + ta_cleanup.tp_array = tp_array; > + ta_cleanup.count = tc; > + > + ALL_THREADS (tp) > + { > + tp_array[i] = tp; > + tp->refcount++; > + i++; > + } > + for (k = 0; k != i; k++) > + if (thread_alive (tp_array[k])) > + { > + switch_to_thread (tp_array[k]->ptid); > + printf_filtered (_("\nThread %d (%s):\n"), > + tp_array[k]->num, target_pid_to_str (inferior_ptid)); This line is too long (max 70-74 characters, hard-limit at 80 characters). > + execute_command (cmd, from_tty); > + strcpy (cmd, saved_cmd); /* Restore exact command used > + previously. */ Please move the comment ahead of the strcpy statement. Comments on the side like this are ok when short enough to fit on the same line. Thank you, -- Joel