From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 8295 invoked by alias); 10 Oct 2014 18:13:36 -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 8279 invoked by uid 89); 10 Oct 2014 18:13:35 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.6 required=5.0 tests=AWL,BAYES_00,LIKELY_SPAM_BODY,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=no version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Fri, 10 Oct 2014 18:13:34 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s9AIDVr2030219 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Fri, 10 Oct 2014 14:13:31 -0400 Received: from localhost (dhcp-10-15-16-169.yyz.redhat.com [10.15.16.169]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s9AIDUKj017636 (version=TLSv1/SSLv3 cipher=AES128-GCM-SHA256 bits=128 verify=NO); Fri, 10 Oct 2014 14:13:31 -0400 From: Sergio Durigan Junior To: jose.marchesi@oracle.com (Jose E. Marchesi) Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 5/9] New probe type: DTrace USDT probes. References: <1411724905-31234-1-git-send-email-jose.marchesi@oracle.com> <1411724905-31234-6-git-send-email-jose.marchesi@oracle.com> <87y4syt5zn.fsf@redhat.com> <87iojrevbz.fsf@oracle.com> X-URL: http://blog.sergiodj.net Date: Fri, 10 Oct 2014 18:13:00 -0000 In-Reply-To: <87iojrevbz.fsf@oracle.com> (Jose E. Marchesi's message of "Fri, 10 Oct 2014 18:35:44 +0200") Message-ID: <87ppdz23p1.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2014-10/txt/msg00272.txt.bz2 On Friday, October 10 2014, Jose E. Marchesi wrote: > > +/* The type of the ELF sections where we will find the DOF programs > > + with information about probes. */ > > + > > +#ifndef SHT_SUNW_dof > > +# define SHT_SUNW_dof 0x6ffffff4 > > +#endif > > Can this macro exist in another header file that you are including? > > That macro is defined in elf.h in Solaris, Minix, and probably other > systems too. I would not be surprised if it is eventually added to the > elf headers in GNU/Linux, and also in binutils. I strongly recommend to > keep that sentinel in place to avoid potential problems with indirect > includes in the future. Sure thing :-). I was mostly curious. > > + > > + /* Number of arguments in the probe. */ > > + ret->probe_argc = DOF_UINT (dof, probe->dofpr_nargc); > > + > > + /* Store argument type descriptions. A description of the type > > + of the argument is in the (J+1)th null-terminated string > > + starting at `strtab' + `probe->dofpr_nargv'. */ > > We're not using `' anymore; instead, we're using '' (GNU Coding Style > has been updated). > > A hard-to-die habit after so many years... :) Haha, yeah, I can imagine :-). > > + ret->args = NULL; > > + p = strtab + DOF_UINT (dof, probe->dofpr_nargv); > > + for (j = 0; j < ret->probe_argc; j++) > > + { > > + struct dtrace_probe_arg arg; > > + struct expression *expr; > > + > > + arg.type_str = xstrdup (p); > > + while (((p - strtab) < strtab_size) /* sentinel. */ > > + && *p++); > > Again a matter of style, but for readability I prefer to write this loop > as: > > /* Use strtab_size as a sentinel. */ > while (*p != '\0' && p - strtab < strtab_size) > ++p; > > What you are suggesting is not exactly equivalent: it leaves `p' at the > blank character, while the idea is to leave `p' at the character next ot > the blank character. I changed the loop to: > > /* Use strtab_size as a sentinel. */ > while (*p++ != '\0' && p - strtab < strtab_size); > > Which makes the comparison explicit and thus may be more palatable for > you :) Ops, indeed, thanks for catching this :-). No wonder I proposed to make the loop clearer :-P. > > + VEC_safe_push (dtrace_probe_arg_s, ret->args, &arg); > > + } > > + > > + /* Add the vector of enablers to this probe, if any. */ > > + ret->enablers = VEC_copy (dtrace_probe_enabler_s, enablers); > > You should free the enablers VEC in the end of the function. You could > probably make a cleanup and call it later. > > Hmm, I don't see the need of doing a deep copy of the vector, nor I > remember why I felt it was necessary to do it when I wrote the original > code. > > I changed that to: > > /* Add the vector of enablers to this probe, if any. */ > ret->enablers = enablers; > > But maybe(probably) I am missing something? :? Hm, right. But if you do that, you will have to adjust dtrace_probe_destroy, because it will be freeing the same 'enablers' over and over... > > + } > > + > > + dtrace_process_dof (sect, objfile, probesp, dof); > > + xfree (dof); > > + } > > + } > > What about using bfd_map_over_sections instead of this for loop? I know > there is precedence of iterating over BFD sections by hand on GDB code, > but bfd_map_over_sections exists for this very purpose. > > I considered that, but the need to define a new structure type for > passing `objfile' and `probesp' to the handler (not to mention the > handler itself) makes it a bit overkill to use bfd_map_over_sections in > this specific case IMO... especially considering that > dtrace_process_dof is only called by this function. OK, fair enough. > > +/* Implementation of the clear_semaphore method. */ > > + > > +static void > > +dtrace_clear_semaphore (struct probe *probe_generic, struct objfile *objfile, > > + struct gdbarch *gdbarch) > > +{ > > + gdb_assert (probe_generic->pops == &dtrace_probe_ops); > > +} > > This shouldn't be needed, because USDT probes don't have the concept of > a semaphore, right? I will submit a patch soon to fix the fact that the > set/clear_semaphore functions are being called inconditionally. > > Correct, that should not be needed and can go away as soon as you do > that change. I should be able to post something today. Will put you on the loop. Cheers, -- Sergio GPG key ID: 0x65FC5E36 Please send encrypted e-mail if possible http://sergiodj.net/