From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 24575 invoked by alias); 10 Oct 2014 16:38:37 -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 24557 invoked by uid 89); 10 Oct 2014 16:38:36 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.2 required=5.0 tests=AWL,BAYES_00,LIKELY_SPAM_BODY,RP_MATCHES_RCVD,SPF_PASS,UNPARSEABLE_RELAY autolearn=no version=3.3.2 X-HELO: userp1040.oracle.com Received: from userp1040.oracle.com (HELO userp1040.oracle.com) (156.151.31.81) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Fri, 10 Oct 2014 16:38:35 +0000 Received: from ucsinet21.oracle.com (ucsinet21.oracle.com [156.151.31.93]) by userp1040.oracle.com (Sentrion-MTA-4.3.2/Sentrion-MTA-4.3.2) with ESMTP id s9AGcSpm012164 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 10 Oct 2014 16:38:28 GMT Received: from aserz7021.oracle.com (aserz7021.oracle.com [141.146.126.230]) by ucsinet21.oracle.com (8.14.4+Sun/8.14.4) with ESMTP id s9AGcRjd015747 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Fri, 10 Oct 2014 16:38:27 GMT Received: from abhmp0017.oracle.com (abhmp0017.oracle.com [141.146.116.23]) by aserz7021.oracle.com (8.14.4+Sun/8.14.4) with ESMTP id s9AGcQZC003880; Fri, 10 Oct 2014 16:38:26 GMT Received: from termi.oracle.com (/10.175.211.52) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Fri, 10 Oct 2014 09:38:25 -0700 From: jose.marchesi@oracle.com (Jose E. Marchesi) To: Sergio Durigan Junior 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> Date: Fri, 10 Oct 2014 16:38:00 -0000 In-Reply-To: <87y4syt5zn.fsf@redhat.com> (Sergio Durigan Junior's message of "Thu, 02 Oct 2014 19:19:08 -0400") Message-ID: <87iojrevbz.fsf@oracle.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3.50 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2014-10/txt/msg00251.txt.bz2 Hi Sergio. > +/* 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. > + > + /* 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... :) > + 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 :) > + 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? :? > +/* Implementation of the get_probes method. */ > + > +static void > +dtrace_get_probes (VEC (probe_p) **probesp, struct objfile *objfile) > +{ > + bfd *abfd = objfile->obfd; > + asection *sect = NULL; > + > + /* Do nothing in case this is a .debug file, instead of the objfile > + itself. */ > + if (objfile->separate_debug_objfile_backlink != NULL) > + return; > + > + /* Iterate over the sections in OBJFILE looking for DTrace > + information. */ > + for (sect = abfd->sections; sect != NULL; sect = sect->next) > + { > + if (elf_section_data (sect)->this_hdr.sh_type == SHT_SUNW_dof) > + { > + struct dtrace_dof_hdr *dof; > + > + /* Read the contents of the DOF section and then process it to > + extract the information of any probe defined into it. */ > + if (!bfd_malloc_and_get_section (abfd, sect, (bfd_byte **) &dof)) > + { > + complaint (&symfile_complaints, > + _("could not obtain the contents of" > + "section '%s' in objfile `%s'."), > + sect->name, abfd->filename); > + return; Why return here? Is there only one section whose type is SHT_SUNW_dof? If no, then I guess the loop should keep rolling. Otherwise, then besides calling return here you should call return after the "xfree" below. Am I getting it right? Yeah, in principle there can be more than one sections of type SHT_SUNW_dof. I changed the code as suggested. > + } > + > + 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. > +/* 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.