From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 26782 invoked by alias); 22 Nov 2012 17:30:43 -0000 Received: (qmail 26763 invoked by uid 22791); 22 Nov 2012 17:30:41 -0000 X-SWARE-Spam-Status: No, hits=-1.9 required=5.0 tests=AWL,BAYES_00,RCVD_IN_HOSTKARMA_NO,TW_BJ,TW_CP,TW_XZ X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 22 Nov 2012 17:30:29 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id D2F211C7151; Thu, 22 Nov 2012 12:30:28 -0500 (EST) 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 tRKv36QREeme; Thu, 22 Nov 2012 12:30:28 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 61CEB1C6550; Thu, 22 Nov 2012 12:30:28 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id 33616C2353; Thu, 22 Nov 2012 09:30:19 -0800 (PST) Date: Thu, 22 Nov 2012 17:30:00 -0000 From: Joel Brobecker To: Pierre Muller Cc: 'Pedro Alves' , 'Eli Zaretskii' , gdb-patches@sourceware.org Subject: Re: [RFC-v3] Fix .text section offset for windows DLL (was Calling __stdcall functions in the inferior) Message-ID: <20121122173019.GF9964@adacore.com> References: <834nm07z0s.fsf@gnu.org> <5077FEB9.4030304@redhat.com> <83y5jb7rfe.fsf@gnu.org> <006001cdaada$00c81f00$02585d00$@muller@ics-cnrs.unistra.fr> <20121024194517.GK3555@adacore.com> <011901cdb2ab$48076b90$d81642b0$@muller@ics-cnrs.unistra.fr> <20121105171121.GA2972@adacore.com> <50991f5f.8382440a.1100.ffff82abSMTPIN_ADDED@mx.google.com> <509ABA17.30507@redhat.com> <000301cdbd96$f5cd9f10$e168dd30$@muller@ics-cnrs.unistra.fr> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <000301cdbd96$f5cd9f10$e168dd30$@muller@ics-cnrs.unistra.fr> User-Agent: Mutt/1.5.21 (2010-09-15) 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 X-SW-Source: 2012-11/txt/msg00599.txt.bz2 Hello Pierre, > 2012-11-08 Pierre Muller > > * coff-pe-read.h (pe_text_section_offset): Declare new function. > * coff-pe-read.c (debug_coff_pe_read): New static variable. > (struct read_pe_section_data): Add section_name field. > (pe_as16): New function. > (IMAGE_SCN_CNT_CODE): New macro, if not already defined. > (IMAGE_SCN_CNT_INITIALIZED_DATA): Ditto. > (IMAGE_SCN_CNT_UNINITIALIZED_DATA): Ditto. > (add_pe_exported_sym): Handle unnamed exported function. > (add_pe_forwarded_sym): New function. > (read_pe_exported_syms): Use ordinal of function to > retrieve correct RVA address of function and handle > forwarded symbol. > (pe_text_section_offset): New function. > (show_debug_coff_pe_read): New function. > (_initialize_coff_pe_read): New function adding > 'set/show debug coff_pe_read' commands. > > * windows-tdep.c (windows_xfer_shared_library): Use > pe_text_section_offset function instead of possibly wrong > 0x1000 constant for .text sextion offset. Just a few minor comments... > +/* Extract for ABFD the offset of the .text section. ^^^^^ from? > + Returns default value 0x1000 if information is not found. */ > +extern CORE_ADDR pe_text_section_offset (struct bfd *abfd); > +#ifndef IMAGE_SCN_CNT_CODE > +# define IMAGE_SCN_CNT_CODE 0x20 > +#endif > +#ifndef IMAGE_SCN_CNT_INITIALIZED_DATA > +# define IMAGE_SCN_CNT_INITIALIZED_DATA 0x40 > +#endif > +#ifndef IMAGE_SCN_CNT_UNINITIALIZED_DATA > +# define IMAGE_SCN_CNT_UNINITIALIZED_DATA 0x80 > +#endif Do you have an idea of when these macros might not be defined? (and where they are normally coming from?). It'd be nice to add a comment providing the answer to those questions. > static void > add_pe_exported_sym (char *sym_name, > unsigned long func_rva, > + int ordinal, > const struct read_pe_section_data *section_data, > const char *dll_name, struct objfile *objfile) Can you update the funtion documentation to explain what each parameter is? > + if ((section_data->ms_type == mst_unknown) && debug_coff_pe_read) > + printf_filtered (_("Unknown section type for \"%s\" for entry \"%s\" \ > +in dll \"%s\"\n"), > + section_data->section_name, sym_name, dll_name); There was a discussion in the past about continuations of string messages, and we decided to avoid putting the continuation of the first column, as it affects the -p switch of the diff. I think you can simply do: printf_filtered (_("Unknown section type for \"%s\" for entry" " \"%s\" in dll \"%s\"\n"), section_data->section_name, sym_name, dll_name); > + if (debug_coff_pe_read > 1) > + printf_filtered (_("Adding exported symbol \"%s\" in dll \"%s\"\n"), > + sym_name, dll_name); Can you use printf_unfiltered for debug traces? There are several instances of this... > +/* Create a minimal symbol entry for an exported forward symbol. > + Returns 1 if the forwarded function was found 0 otherwise. */ "Return" (our style is more "do this, do that", rather than "does this, does that"). Can you also document what each parameter is? > + int dll_name_len = strlen (dll_name); > + char *forward_minimal_name = xmalloc (forward_dll_name_len + > + forward_func_name_len + 2); Can you use alloca, here? It avoids the need to xfree it later. If you need to xmalloc, then you need to xfree it, and I think that you'd be safer using a cleanup - some of the functions you use might throw an error. > + strncpy (forward_minimal_name, forward_dll_name, forward_dll_name_len); > + forward_minimal_name[forward_dll_name_len] = '!'; > + strcpy (forward_minimal_name + forward_dll_name_len + 1, forward_func_name); You also have the "concat" function, if that makes things a little simple for you. > + int i; > + for (i = 0; i < forward_dll_name_len; i++) Empty line between variable declaration and the rest of the code... > + if (debug_coff_pe_read > 1) > + printf_filtered (_("Adding forwarded exported symbol \"%s\" " > + "in dll \"%s\", pointing to \"%s\"\n"), > + sym_name, dll_name, forward_minimal_name); [l..] > + if (debug_coff_pe_read) > + printf_filtered (_("Unable to find function \"%s\" in dll \"%s\" " > + ", forward of \"%s\" in dll \"%s\"\n"), > + forward_func_name, forward_dll_name, sym_name, > + dll_name); printf_unfiltered. > + qualified_name = xmalloc (dll_name_len + strlen (sym_name) + 2); Same remarks as for "forward_minimal_name"... > + char * last_point = strrchr (dll_name, '.'); > + if (last_point != NULL) > + *last_point = '\0'; Empty line after var declarations... > - struct read_pe_section_data section_data[PE_SECTION_TABLE_SIZE] > - = { {0, 1, 0, mst_text}, > - {0, 1, 0, mst_data}, > - {0, 1, 0, mst_bss} > - }; > + struct read_pe_section_data *section_data; [...] > + section_data = xzalloc (PE_SECTION_TABLE_SIZE > + * sizeof (struct read_pe_section_data)); > + Are we missing a cleanup/xfree? > + for (i=0; i < PE_SECTION_TABLE_SIZE; i++) > + { > + section_data[i].vma_offset = 0; > + section_data[i].rva_start = 1; > + section_data[i].rva_end = 0; > + }; > + section_data[PE_SECTION_INDEX_TEXT].ms_type = mst_text; > + section_data[PE_SECTION_INDEX_TEXT].section_name = ".text"; > + section_data[PE_SECTION_INDEX_DATA].ms_type = mst_data; > + section_data[PE_SECTION_INDEX_DATA].section_name = ".data"; > + section_data[PE_SECTION_INDEX_BSS].ms_type = mst_bss; > + section_data[PE_SECTION_INDEX_BSS].section_name = ".bss"; Also, I think it makes it harder to determine what the contents of the table is. I suggest you go back to the static definition above, but updated with the extra field. > - if (vaddr <= export_rva && vaddr + vsize > export_rva) > + if ((strcmp (sname, ".edata") == 0) > + || ((vaddr <= export_opthdrrva) > + && (export_opthdrrva < vaddr + vsize))) The extra parentheses around the numerical comparison operators are unnecessary, I believe. I think the following is equivalent: if (strcmp (sname, ".edata") == 0 || (vaddr <= export_opthdrrva && export_opthdrrva < vaddr + vsize)) > + else if ((export_opthdrrva != vaddr) && debug_coff_pe_read) Same here... > + /* Retrieve ordinal value */ Missing period at end of comment. > + /* This is relatived to ordinal value. */ Missing second space after the period... > + if ((func_rva >= export_rva) > + && (func_rva < export_rva + export_size)) Unnecessary parentheses. > + if (sep) > + { > + int len = (int) (sep - forward_name); > + forward_dll_name = xmalloc (len + 1); Missing empty line after variable declaration. You might want to use a cleanup, just in case something in add_pe_forwarded_sym calls error. > + static char null_char = '\0'; > + > + add_pe_exported_sym (&null_char, func_rva, ordinal, > + section_data, dll_name, objfile); Why does this have to be static? Can you make prim_record_minimal_symbol sym_name parameter a "const", and then declare... const char *empty_name = ""; ... and pass that to add_pe_exported_sym? > + char sname[8]; [...] > + bfd_bread (sname, (bfd_size_type) 8, abfd); Use sizeof (sname) instead of litteral 8? > +static void > +show_debug_coff_pe_read (struct ui_file *file, int from_tty, > + struct cmd_list_element *c, const char *value) This function needs a short description ("implements ..." is good enough). -- Joel