Re: [patch] Reading coff-pe-read files

Re: [patch] Reading coff-pe-read files

[Kai Tietz]

[-- Attachment #1: Type: text/plain, Size: 214 bytes --]

Hello,

I modified my patch, so that it doesn't need a #if clause anymore.


Cheers,
Kai



|  (\_/)  This is Bunny. Copy and paste Bunny
| (='.'=) into your signature to help him gain
| (")_(") world domination.


[-- Attachment #2: pe-coff-read.diff --]
[-- Type: application/octet-stream, Size: 2049 bytes --]

Index: src/gdb/coff-pe-read.c
===================================================================
--- src.orig/gdb/coff-pe-read.c
+++ src/gdb/coff-pe-read.c
@@ -191,6 +191,8 @@ read_pe_exported_syms (struct objfile *o
   unsigned char *expdata, *erva;
   unsigned long name_rvas, ordinals, nexp, ordbase;
   char *dll_name;
+  int be64 = 0;
+  int be32 = 0;
 
   /* Array elements are for text, data and bss in that order
      Initialization with start_rva > end_rva guarantees that
@@ -205,7 +207,11 @@ read_pe_exported_syms (struct objfile *o
 
   char const *target = bfd_get_target (objfile->obfd);
 
-  if ((strcmp (target, "pe-i386") != 0) && (strcmp (target, "pei-i386") != 0))
+  be64 = ((strcmp (target, "pe-x86-64") == 0)
+         || ((strcmp (target, "pei-x86-64") == 0));
+  be32 = ((strcmp (target, "pe-i386") == 0)
+          || (strcmp (target, "pei-i386") == 0));
+  if (!be32 && !be64)
     {
       /* This is not an i386 format file. Abort now, because the code
          is untested on anything else. *FIXME* test on further
@@ -216,15 +222,26 @@ read_pe_exported_syms (struct objfile *o
   /* Get pe_header, optional header and numbers of export entries.  */
   pe_header_offset = pe_get32 (dll, 0x3c);
   opthdr_ofs = pe_header_offset + 4 + 20;
-  num_entries = pe_get32 (dll, opthdr_ofs + 92);
+  if (be64)
+    num_entries = pe_get32 (dll, opthdr_ofs + 92 + 16);
+  else
+    num_entries = pe_get32 (dll, opthdr_ofs + 92);
 
   if (num_entries < 1)		/* No exports.  */
     {
       return;
     }
 
-  export_rva = pe_get32 (dll, opthdr_ofs + 96);
-  export_size = pe_get32 (dll, opthdr_ofs + 100);
+  if (be64)
+    {
+      export_rva = pe_get32 (dll, opthdr_ofs + 96 + 16);
+      export_size = pe_get32 (dll, opthdr_ofs + 100 + 16);
+    }
+  else
+    {
+      export_rva = pe_get32 (dll, opthdr_ofs + 96);
+      export_size = pe_get32 (dll, opthdr_ofs + 100);
+    }
   nsections = pe_get16 (dll, pe_header_offset + 4 + 2);
   secptr = (pe_header_offset + 4 + 20 +
 	    pe_get16 (dll, pe_header_offset + 4 + 16));

Re: [patch] Reading coff-pe-read files

[Joel Brobecker]

Hello Kai,

> I modified my patch, so that it doesn't need a #if clause anymore.

Thanks.  This was an excellent suggestion from Pierre and I'm much
happier with the new patch.

A few comments:

First, do you think you could use a different mime type when attaching
a patch?  I shows up as application/octet-stream which means that my
mailer doesn't treat it as text - This means that when I reply, your
patch isn't quoted automatically as part of the reply.  text/plain would
be great. Some people also use some mailers that do not handle base64
encodings. One of them is Mark Kettenis who's still one of our active
maintainers, particularly on x86/x86_64. You'll have more chances to get
his comments if you use a 7bit ASCII file...

Can you also provide a ChangeLog entry when submitting patches?

> Index: src/gdb/coff-pe-read.c
> ===================================================================
> --- src.orig/gdb/coff-pe-read.c
> +++ src/gdb/coff-pe-read.c
> @@ -191,6 +191,8 @@ read_pe_exported_syms (struct objfile *o
>    unsigned char *expdata, *erva;
>    unsigned long name_rvas, ordinals, nexp, ordbase;
>    char *dll_name;
> +  int be64 = 0;
> +  int be32 = 0;

Would you mind explaining what "be" stands for?

> +  if (be64)
> +    num_entries = pe_get32 (dll, opthdr_ofs + 92 + 16);
> +  else
> +    num_entries = pe_get32 (dll, opthdr_ofs + 92);

Not knowing the PE format all that well, could you explain
these numbers a little? 92 + 16 seems to suggest that the number
of entries is no longer at the beginning of some kind of "section"
but 16 bytes later.

(this is for my education - I tried to find some documentation
about the 64bit PE/COFF format, but the only one I found was from
MS and it's in a docx format that I can't seem to be able to open
with openoffice - I would be very grateful for a PDF)

> -  export_rva = pe_get32 (dll, opthdr_ofs + 96);
> -  export_size = pe_get32 (dll, opthdr_ofs + 100);
> +  if (be64)
> +    {
> +      export_rva = pe_get32 (dll, opthdr_ofs + 96 + 16);
> +      export_size = pe_get32 (dll, opthdr_ofs + 100 + 16);
> +    }
> +  else
> +    {
> +      export_rva = pe_get32 (dll, opthdr_ofs + 96);
> +      export_size = pe_get32 (dll, opthdr_ofs + 100);
> +    }

Same here.

I trust you that the changes are correct (and in any case only
affect x86_64-windows), but I want to understand them a little bit.

In the meantime, do you have write access to the GDB project?
If not, let's also work on that. I see that you have a copyright
assignment already in place, and an account on sourceware.org
as well.

-- 
Joel

Re: [patch] Reading coff-pe-read files

[Kai Tietz]

Hi Joel,

Joel Brobecker <brobecker@adacore.com> wrote on 08.01.2009 10:53:36:

> Hello Kai,
> 
> > I modified my patch, so that it doesn't need a #if clause anymore.
> 
> Thanks.  This was an excellent suggestion from Pierre and I'm much
> happier with the new patch.
> 
> A few comments:
> 
> First, do you think you could use a different mime type when attaching
> a patch?  I shows up as application/octet-stream which means that my
> mailer doesn't treat it as text - This means that when I reply, your
> patch isn't quoted automatically as part of the reply.  text/plain would
> be great. Some people also use some mailers that do not handle base64
> encodings. One of them is Mark Kettenis who's still one of our active
> maintainers, particularly on x86/x86_64. You'll have more chances to get
> his comments if you use a 7bit ASCII file...

Hmm, at home it isn't hard to do this, but at office I have to fight with 
Lotus. I'll see what can do.

> Can you also provide a ChangeLog entry when submitting patches?

I did in my first mail. I thought there is no new one necessary.

ChangeLog

2009-01-08  Kai Tietz  <kai.tietz@onevision.com>

        * coff-pe-read.c (read_pe_exported_syms): Enable read of PE+ 
export directory.


> > Index: src/gdb/coff-pe-read.c
> > ===================================================================
> > --- src.orig/gdb/coff-pe-read.c
> > +++ src/gdb/coff-pe-read.c
> > @@ -191,6 +191,8 @@ read_pe_exported_syms (struct objfile *o
> >    unsigned char *expdata, *erva;
> >    unsigned long name_rvas, ordinals, nexp, ordbase;
> >    char *dll_name;
> > +  int be64 = 0;
> > +  int be32 = 0;
> 
> Would you mind explaining what "be" stands for?

the "be" from "to be, or not to be" ;)

> > +  if (be64)
> > +    num_entries = pe_get32 (dll, opthdr_ofs + 92 + 16);
> > +  else
> > +    num_entries = pe_get32 (dll, opthdr_ofs + 92);
> 
> Not knowing the PE format all that well, could you explain
> these numbers a little? 92 + 16 seems to suggest that the number
> of entries is no longer at the beginning of some kind of "section"
> but 16 bytes later.

This offset is reasoned by the fact, that PE+ header (
IMAGE_OPTIONAL_HEADER) has a different size (and some differences in 
layout).
The 32-bit header looks like this:

typedef struct _IMAGE_OPTIONAL_HEADER {
  uint16_t Magic;
  uint8_t MajorLinkerVersion;
  uint8_t MinorLinkerVersion;
  uint32_t SizeOfCode;
  uint32_t SizeOfInitializedData;
  uint32_t SizeOfUninitializedData;
  uint32_t AddressOfEntryPoint;
  uint32_t BaseOfCode;
  uint32_t BaseOfData;
  uint32_t ImageBase;
  uint32_t SectionAlignment;
  uint32_t FileAlignment;
  uint16_t MajorOperatingSystemVersion;
  uint16_t MinorOperatingSystemVersion;
  uint16_t MajorImageVersion;
  uint16_t MinorImageVersion;
  uint16_t MajorSubsystemVersion;
  uint16_t MinorSubsystemVersion;
  uint32_t Win32VersionValue;
  uint32_t SizeOfImage;
  uint32_t SizeOfHeaders;
  uint32_t CheckSum;
  uint16_t Subsystem;
  uint16_t DllCharacteristics;
  uint32_t SizeOfStackReserve;
  uint32_t SizeOfStackCommit;
  uint32_t SizeOfHeapReserve;
  uint32_t SizeOfHeapCommit;
  uint32_t LoaderFlags;
  uint32_t NumberOfRvaAndSizes;
  IMAGE_DATA_DIRECTORY DataDirectory[IMAGE_NUMBEROF_DIRECTORY_ENTRIES];
} IMAGE_OPTIONAL_HEADER32,*PIMAGE_OPTIONAL_HEADER32;

the 64-bit one like that:

typedef struct _IMAGE_OPTIONAL_HEADER64 {
  uint16_t Magic;
  uint8_t MajorLinkerVersion;
  uint8_t MinorLinkerVersion;
  uint32_t SizeOfCode;
  uint32_t SizeOfInitializedData;
  uint32_t SizeOfUninitializedData;
  uint32_t AddressOfEntryPoint;
  uint32_t BaseOfCode;
  uint64_t ImageBase;
  uint32_t SectionAlignment;
  uint32_t FileAlignment;
  uint16_t MajorOperatingSystemVersion;
  uint16_t MinorOperatingSystemVersion;
  uint16_t MajorImageVersion;
  uint16_t MinorImageVersion;
  uint16_t MajorSubsystemVersion;
  uint16_t MinorSubsystemVersion;
  uint32_t Win32VersionValue;
  uint32_t SizeOfImage;
  uint32_t SizeOfHeaders;
  uint32_t CheckSum;
  uint16_t Subsystem;
  uint16_t DllCharacteristics;
  uint64_t SizeOfStackReserve;
  uint64_t SizeOfStackCommit;
  uint64_t SizeOfHeapReserve;
  uint64_t SizeOfHeapCommit;
  uint32_t LoaderFlags;
  uint32_t NumberOfRvaAndSizes;
  IMAGE_DATA_DIRECTORY DataDirectory[IMAGE_NUMBEROF_DIRECTORY_ENTRIES];
} IMAGE_OPTIONAL_HEADER64,*PIMAGE_OPTIONAL_HEADER64;

So as you can see, the entries for ImageBase, SizeOfStackReserve, 
SizeOfStackCommit,SizeOfHeapReserve.and SizeOfHeapCommit have 64-bit sizes 
in PE+.
For PE+ there is no member BaseOfData present anymore. So the delta is 5 * 
(8 - 4 /* The diff of sizes for those members */) - 4 /* Because there is 
no BaseOfData member anymore */. So I came to the diff of 16 bytes.


> (this is for my education - I tried to find some documentation
> about the 64bit PE/COFF format, but the only one I found was from
> MS and it's in a docx format that I can't seem to be able to open
> with openoffice - I would be very grateful for a PDF)
If you upgrade to OpenOffice 3.0 you can happily open it (as I do).

> > -  export_rva = pe_get32 (dll, opthdr_ofs + 96);
> > -  export_size = pe_get32 (dll, opthdr_ofs + 100);
> > +  if (be64)
> > +    {
> > +      export_rva = pe_get32 (dll, opthdr_ofs + 96 + 16);
> > +      export_size = pe_get32 (dll, opthdr_ofs + 100 + 16);
> > +    }
> > +  else
> > +    {
> > +      export_rva = pe_get32 (dll, opthdr_ofs + 96);
> > +      export_size = pe_get32 (dll, opthdr_ofs + 100);
> > +    }
> 
> Same here.

The same reason. Structure IMAGE_DATA_DIRECTORY hasn't changed in sizes, 
therefore just the delta is necessary.

> I trust you that the changes are correct (and in any case only
> affect x86_64-windows), but I want to understand them a little bit.
> 
> In the meantime, do you have write access to the GDB project?
> If not, let's also work on that. I see that you have a copyright
> assignment already in place, and an account on sourceware.org
> as well.

AFAIC I have no write access (or write permissions) to gbd tree. I have 
already an account and write permissions for binutils.


Cheers,
Kai

|  (\_/)  This is Bunny. Copy and paste Bunny
| (='.'=) into your signature to help him gain
| (")_(") world domination.

Re: [patch] Reading coff-pe-read files

[Joel Brobecker]

> Hmm, at home it isn't hard to do this, but at office I have to fight with 
> Lotus. I'll see what can do.

Thanks for doing that. Perhaps, if Lotus doesn't muck the contents
of your emails, one solution is to inline the patch inside the email
body rather than as an attachment. If, as I fear, Lotus does things
like breaking lines, etc, then the current approach at least allows
us to receive the patch intact, which is the most important. Anyway,
all this monologue just to say: Do your best :).

> > Can you also provide a ChangeLog entry when submitting patches?
> 
> I did in my first mail. I thought there is no new one necessary.

Hmmm, I thought I double-checked before mentioning it, sorry.

> 2009-01-08  Kai Tietz  <kai.tietz@onevision.com>
> 
>         * coff-pe-read.c (read_pe_exported_syms): Enable read of PE+ 
> export directory.

Does "PE+" mean PE for 64bit?

> > > +  int be64 = 0;
> > > +  int be32 = 0;
> > 
> > Would you mind explaining what "be" stands for?
> 
> the "be" from "to be, or not to be" ;)

In that case, I'd really like to use a more meaningful name.
How about pe32_p and pe64_p? Or is_pe32 and is_pe64?

> > > +  if (be64)
> > > +    num_entries = pe_get32 (dll, opthdr_ofs + 92 + 16);
> > > +  else
> > > +    num_entries = pe_get32 (dll, opthdr_ofs + 92);
> > 
> > Not knowing the PE format all that well, could you explain
> > these numbers a little? 92 + 16 seems to suggest that the number
> > of entries is no longer at the beginning of some kind of "section"
> > but 16 bytes later.
[...]
> So as you can see, the entries for ImageBase, SizeOfStackReserve, 
> SizeOfStackCommit,SizeOfHeapReserve.and SizeOfHeapCommit have 64-bit sizes 
> in PE+.
> For PE+ there is no member BaseOfData present anymore. So the delta is 5 * 
> (8 - 4 /* The diff of sizes for those members */) - 4 /* Because there is 
> no BaseOfData member anymore */. So I came to the diff of 16 bytes.

OK - I understand, now. Thanks!

I think that it would be clearer to use 108 in this case than
"92 + 16", because 92 in the PE+ case doesn't really mean anything,
does it?

> > Same here.
> 
> The same reason. Structure IMAGE_DATA_DIRECTORY hasn't changed in sizes, 
> therefore just the delta is necessary.

I also suggest the same as above, if you agree.

> AFAIC I have no write access (or write permissions) to gbd tree. I have 
> already an account and write permissions for binutils.

Given that this patch qualifies as a good patch (or will qualify as soon
as we agree on one version of it), you are eligible for receiving "Write
After Approval" priviledges. What you need to do next, in parallel to
this discussion, is ask overseers to adjust your priviledges.  This is
what the account request on sourceware says about your case:

    Note that if you already have an account on sourceware.org or
    gcc.gnu.org for CVS or Subversion write access, then do not use this
    form. Instead send an email to the overseers mail account at this site
    telling what project you want write access to and who approved that
    access.

You can list me as the approver.

-- 
Joel

Re: [patch] Reading coff-pe-read files

[Kai Tietz]

[-- Attachment #1: Type: text/plain, Size: 2722 bytes --]

Hi Joel,

Joel Brobecker <brobecker@adacore.com> wrote on 08.01.2009 12:10:02:

> > Hmm, at home it isn't hard to do this, but at office I have to fight 
with 
> > Lotus. I'll see what can do.
> 
> Thanks for doing that. Perhaps, if Lotus doesn't muck the contents
> of your emails, one solution is to inline the patch inside the email
> body rather than as an attachment. If, as I fear, Lotus does things
> like breaking lines, etc, then the current approach at least allows
> us to receive the patch intact, which is the most important. Anyway,
> all this monologue just to say: Do your best :).
> 
> > > Can you also provide a ChangeLog entry when submitting patches?
> > 
> > I did in my first mail. I thought there is no new one necessary.
> 
> Hmmm, I thought I double-checked before mentioning it, sorry.
> 
> > 2009-01-08  Kai Tietz  <kai.tietz@onevision.com>
> > 
> >         * coff-pe-read.c (read_pe_exported_syms): Enable read of PE+ 
> > export directory.
> 
> Does "PE+" mean PE for 64bit?
> 
> > > > +  int be64 = 0;
> > > > +  int be32 = 0;
> > > 
> > > Would you mind explaining what "be" stands for?
> > 
> > the "be" from "to be, or not to be" ;)
> 
> In that case, I'd really like to use a more meaningful name.
> How about pe32_p and pe64_p? Or is_pe32 and is_pe64?

Fine for me, I changed the attached patch.

[...]
> 
> I think that it would be clearer to use 108 in this case than
> "92 + 16", because 92 in the PE+ case doesn't really mean anything,
> does it?
> 
> > > Same here.
> > 
> > The same reason. Structure IMAGE_DATA_DIRECTORY hasn't changed in 
sizes, 
> > therefore just the delta is necessary.
> 
> I also suggest the same as above, if you agree.

Ok, done.

> > AFAIC I have no write access (or write permissions) to gbd tree. I 
have 
> > already an account and write permissions for binutils.
> 
> Given that this patch qualifies as a good patch (or will qualify as soon
> as we agree on one version of it), you are eligible for receiving "Write
> After Approval" priviledges. What you need to do next, in parallel to
> this discussion, is ask overseers to adjust your priviledges.  This is
> what the account request on sourceware says about your case:
> 
>     Note that if you already have an account on sourceware.org or
>     gcc.gnu.org for CVS or Subversion write access, then do not use this
>     form. Instead send an email to the overseers mail account at this 
site
>     telling what project you want write access to and who approved that
>     access.
> 
> You can list me as the approver.

Thanks, I sent mail to the overseers.

Cheers,
Kai



|  (\_/)  This is Bunny. Copy and paste Bunny
| (='.'=) into your signature to help him gain
| (")_(") world domination.

[-- Attachment #2: pe-coff-read.diff --]
[-- Type: application/octet-stream, Size: 2059 bytes --]

Index: src/gdb/coff-pe-read.c
===================================================================
--- src.orig/gdb/coff-pe-read.c
+++ src/gdb/coff-pe-read.c
@@ -191,6 +191,8 @@ read_pe_exported_syms (struct objfile *o
   unsigned char *expdata, *erva;
   unsigned long name_rvas, ordinals, nexp, ordbase;
   char *dll_name;
+  int is_pe64 = 0;
+  int is_pe32 = 0;
 
   /* Array elements are for text, data and bss in that order
      Initialization with start_rva > end_rva guarantees that
@@ -205,7 +207,11 @@ read_pe_exported_syms (struct objfile *o
 
   char const *target = bfd_get_target (objfile->obfd);
 
-  if ((strcmp (target, "pe-i386") != 0) && (strcmp (target, "pei-i386") != 0))
+  is_pe64 = ((strcmp (target, "pe-x86-64") == 0)
+         || ((strcmp (target, "pei-x86-64") == 0));
+  is_pe32 = ((strcmp (target, "pe-i386") == 0)
+          || (strcmp (target, "pei-i386") == 0));
+  if (!is_pe32 && !is_pe64)
     {
       /* This is not an i386 format file. Abort now, because the code
          is untested on anything else. *FIXME* test on further
@@ -216,15 +222,26 @@ read_pe_exported_syms (struct objfile *o
   /* Get pe_header, optional header and numbers of export entries.  */
   pe_header_offset = pe_get32 (dll, 0x3c);
   opthdr_ofs = pe_header_offset + 4 + 20;
-  num_entries = pe_get32 (dll, opthdr_ofs + 92);
+  if (is_pe64)
+    num_entries = pe_get32 (dll, opthdr_ofs + 108;
+  else
+    num_entries = pe_get32 (dll, opthdr_ofs + 92);
 
   if (num_entries < 1)		/* No exports.  */
     {
       return;
     }
 
-  export_rva = pe_get32 (dll, opthdr_ofs + 96);
-  export_size = pe_get32 (dll, opthdr_ofs + 100);
+  if (is_pe64)
+    {
+      export_rva = pe_get32 (dll, opthdr_ofs + 112);
+      export_size = pe_get32 (dll, opthdr_ofs + 116);
+    }
+  else
+    {
+      export_rva = pe_get32 (dll, opthdr_ofs + 96);
+      export_size = pe_get32 (dll, opthdr_ofs + 100);
+    }
   nsections = pe_get16 (dll, pe_header_offset + 4 + 2);
   secptr = (pe_header_offset + 4 + 20 +
 	    pe_get16 (dll, pe_header_offset + 4 + 16));

Re: [patch] Reading coff-pe-read files

[Joel Brobecker]

> > > 2009-01-08  Kai Tietz  <kai.tietz@onevision.com>
> > > 
> > >         * coff-pe-read.c (read_pe_exported_syms): Enable read of PE+ 
> > > export directory.

Approved.

Once your account gives you write access, please add yourself in
the MAINTAINERS file in the "WRITE AFTER APPROVAL" section (remember
to post a patch on this list as well :-). Once done, you can commit
this patch as well.

In the meantime, I will test this patch in my own tree and see if
it helps fixing the problem after thread switching...

Thank you,
-- 
Joel

Re: [patch] Reading coff-pe-read files

[Joel Brobecker]

> > > > 2009-01-08  Kai Tietz  <kai.tietz@onevision.com>
> > > > 
> > > >         * coff-pe-read.c (read_pe_exported_syms): Enable read of PE+ 
> > > > export directory.
> 
> Approved.

Actually, the compiler just spotted a couple of syntax errors.
GASP! We should have both been more careful.
Can you please fix them before checking in?

> -  if ((strcmp (target, "pe-i386") != 0) && (strcmp (target, "pei-i386") != 0))
> +  is_pe64 = ((strcmp (target, "pe-x86-64") == 0)
> +         || ((strcmp (target, "pei-x86-64") == 0));
               ^^ One '(' too many here.

Also, I can never determine in diffs whether the weird formatting
is because of tabs or because it is wrong.  But in this case,
the "||" is put at the wrong location. I know it looks nicer if
the two strcmp expressions are aligned, but any formatter will
destroy that, and we do use GNU indent once in a while.  So let's
be consistent and format the above as follow:

> +  is_pe64 = ((strcmp (target, "pe-x86-64") == 0)
> +             || ((strcmp (target, "pei-x86-64") == 0));

Same for is_pe32.

> +  if (is_pe64)
> +    num_entries = pe_get32 (dll, opthdr_ofs + 108;
                                                    ^^
                                          missing ')' here.

Thanks,
-- 
Joel

Re: [patch] Reading coff-pe-read files

[Kai Tietz]

Joel Brobecker <brobecker@adacore.com> wrote on 08.01.2009 14:09:04:

> > > > > 2009-01-08  Kai Tietz  <kai.tietz@onevision.com>
> > > > > 
> > > > >         * coff-pe-read.c (read_pe_exported_syms): Enable read of 
PE+ 
> > > > > export directory.
> > 
> > Approved.
> 
> Actually, the compiler just spotted a couple of syntax errors.
> GASP! We should have both been more careful.
> Can you please fix them before checking in?
> 
> > -  if ((strcmp (target, "pe-i386") != 0) && (strcmp (target, "pei-
> i386") != 0))
> > +  is_pe64 = ((strcmp (target, "pe-x86-64") == 0)
> > +         || ((strcmp (target, "pei-x86-64") == 0));
>                ^^ One '(' too many here.
> 
> Also, I can never determine in diffs whether the weird formatting
> is because of tabs or because it is wrong.  But in this case,
> the "||" is put at the wrong location. I know it looks nicer if
> the two strcmp expressions are aligned, but any formatter will
> destroy that, and we do use GNU indent once in a while.  So let's
> be consistent and format the above as follow:
> 
> > +  is_pe64 = ((strcmp (target, "pe-x86-64") == 0)
> > +             || ((strcmp (target, "pei-x86-64") == 0));
> 
> Same for is_pe32.
> 
> > +  if (is_pe64)
> > +    num_entries = pe_get32 (dll, opthdr_ofs + 108;
>                                                     ^^
>                                           missing ')' here.
> 
> Thanks,
> -- 
> Joel
> 

I corrected it in my patch.

Sorry,
Kai

|  (\_/)  This is Bunny. Copy and paste Bunny
| (='.'=) into your signature to help him gain
| (")_(") world domination.

Re: [patch] Reading coff-pe-read files

[Christopher Faylor]

On Thu, Jan 08, 2009 at 04:58:16PM +0400, Joel Brobecker wrote:
>> > > 2009-01-08  Kai Tietz  <kai.tietz@onevision.com>
>> > > 
>> > >         * coff-pe-read.c (read_pe_exported_syms): Enable read of PE+ 
>> > > export directory.
>
>Approved.
>
>Once your account gives you write access, please add yourself in
>the MAINTAINERS file in the "WRITE AFTER APPROVAL" section (remember
>to post a patch on this list as well :-). Once done, you can commit
>this patch as well.

Kai already has checkin privileges to binutils so he can already check
into gdb.

cgf

Re: [patch] Reading coff-pe-read files

[Kai Tietz]

2009/1/8 Christopher Faylor <cgf-use-the-mailinglist-please@sourceware.org>:
> On Thu, Jan 08, 2009 at 04:58:16PM +0400, Joel Brobecker wrote:
>>> > > 2009-01-08  Kai Tietz  <kai.tietz@onevision.com>
>>> > >
>>> > >         * coff-pe-read.c (read_pe_exported_syms): Enable read of PE+
>>> > > export directory.
>>
>>Approved.
>>
>>Once your account gives you write access, please add yourself in
>>the MAINTAINERS file in the "WRITE AFTER APPROVAL" section (remember
>>to post a patch on this list as well :-). Once done, you can commit
>>this patch as well.
>
> Kai already has checkin privileges to binutils so he can already check
> into gdb.
>
> cgf
>

Thank you all. Patch committed.

Cheers,
Kai

-- 
|  (\_/) This is Bunny. Copy and paste
| (='.'=) Bunny into your signature to help
| (")_(") him gain world domination

Re: [patch] Reading coff-pe-read files

[Pedro Alves]

On Thursday 08 January 2009 20:55:13, Kai Tietz wrote:

> Thank you all. Patch committed.

Looks like the wrong version got in? :

.../../src/gdb/coff-pe-read.c: In function 'read_pe_exported_syms':
../../src/gdb/coff-pe-read.c:226: error: expected ')' before ';' token
../../src/gdb/coff-pe-read.c:358: error: expected ';' before '}' token

-- 
Pedro Alves

Re: [patch] Reading coff-pe-read files

[Kai Tietz]

Pedro Alves <pedro@codesourcery.com> wrote on 09.01.2009 09:58:01:

> On Thursday 08 January 2009 20:55:13, Kai Tietz wrote:
> 
> > Thank you all. Patch committed.
> 
> Looks like the wrong version got in? :
> 
> .../../src/gdb/coff-pe-read.c: In function 'read_pe_exported_syms':
> ../../src/gdb/coff-pe-read.c:226: error: expected ')' before ';' token
> ../../src/gdb/coff-pe-read.c:358: error: expected ';' before '}' token
> 
> -- 
> Pedro Alves
> 
Hi Pedro,

Sorry, I missed one ending frame. Committed as obvious.

2009-01-08  Kai Tietz  <kai.tietz@onevision.com>

        * coff-pe-read.c (read_pe_exported_syms): Fix typo.

Thanks,
Kai

-----
Index: coff-pe-read.c
===================================================================
RCS file: /cvs/src/src/gdb/coff-pe-read.c,v
retrieving revision 1.9
diff -u -r1.9 coff-pe-read.c
--- coff-pe-read.c      8 Jan 2009 20:53:32 -0000       1.9
+++ coff-pe-read.c      9 Jan 2009 09:31:19 -0000
@@ -223,7 +223,7 @@
   pe_header_offset = pe_get32 (dll, 0x3c);
   opthdr_ofs = pe_header_offset + 4 + 20;
   if (is_pe64)
-    num_entries = pe_get32 (dll, opthdr_ofs + 108;
+    num_entries = pe_get32 (dll, opthdr_ofs + 108);
   else
     num_entries = pe_get32 (dll, opthdr_ofs + 92);

RE: [patch] Reading coff-pe-read files

[Pierre Muller]

> From Joel Brobecker
> Hello Kai,
> 
> > I modified my patch, so that it doesn't need a #if clause anymore.
> 
> Thanks.  This was an excellent suggestion from Pierre and I'm much
> happier with the new patch.

  Just to restore the truth,
I found the second version of the patch submitted by Kai Tietz,
just after sending my answer, so that my suggestion came too late...


Pierre Muller
Pascal language support maintainer for GDB

[RFA/commit] arch-utils.c: Use host_address_to_string when printing function addresses

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 451 bytes --]

Similar to what I did in gdbarch.sh/gdbarch.c, this patch uses
host_address_to_string to transform addresses into strings, rather
than printing them as longs...

2009-01-07  Joel Brobecker  <brobecker@adacore.com>

        * arch-utils.c (gdbarch_update_p): Use host_address_to_string
        to print the address of the gdbarch pointer.

Tested on x86-linux. Will commit at the same time as the gdbarch.sh
patch if there are no objections.

-- 
Joel

[-- Attachment #2: arch-utils.diff --]
[-- Type: text/plain, Size: 995 bytes --]

diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
index 41c4933..c1ea9da 100644
--- a/gdb/arch-utils.c
+++ b/gdb/arch-utils.c
@@ -495,8 +495,8 @@ gdbarch_update_p (struct gdbarch_info info)
     {
       if (gdbarch_debug)
 	fprintf_unfiltered (gdb_stdlog, "gdbarch_update_p: "
-			    "Architecture 0x%08lx (%s) unchanged\n",
-			    (long) new_gdbarch,
+			    "Architecture %s (%s) unchanged\n",
+			    host_address_to_string (new_gdbarch),
 			    gdbarch_bfd_arch_info (new_gdbarch)->printable_name);
       return 1;
     }
@@ -504,8 +504,8 @@ gdbarch_update_p (struct gdbarch_info info)
   /* It's a new architecture, swap it in.  */
   if (gdbarch_debug)
     fprintf_unfiltered (gdb_stdlog, "gdbarch_update_p: "
-			"New architecture 0x%08lx (%s) selected\n",
-			(long) new_gdbarch,
+			"New architecture %s (%s) selected\n",
+			host_address_to_string (new_gdbarch),
 			gdbarch_bfd_arch_info (new_gdbarch)->printable_name);
   deprecated_current_gdbarch_select_hack (new_gdbarch);
 

[patch] Reading coff-pe-read files

[Kai Tietz]

[-- Attachment #1: Type: text/plain, Size: 820 bytes --]

Hi,

One small piece for x86_64 windows support in coff-pe-read.c seems to be 
missing. In function read_pe_exported_syms wrong indexes are used to find 
exports of the image.
The source uses here the same pattern as in bfd/pe-dll.c.

ChangeLog

2009-01-07  Kai Tietz  <kai.tietz@onevision.com>

        * coff-pe-read.c (read_pe_exported_syms): Enable read of PE+ 
export directory.


I am uncertain, which macro I should use here (or if it would be better to 
make for PE+ a copy of this file), so I used _WIN64 as condition.

Cheers,
Kai

PS: I have already papers ready with FSF for gdb, so there shouldn't be 
any problem about taking parts of my donated  code, I've provided to Joel 
IMHO.



|  (\_/)  This is Bunny. Copy and paste Bunny
| (='.'=) into your signature to help him gain
| (")_(") world domination.

[-- Attachment #2: pe-coff-read.diff --]
[-- Type: application/octet-stream, Size: 1503 bytes --]

Index: src/gdb/coff-pe-read.c
===================================================================
--- src.orig/gdb/coff-pe-read.c
+++ src/gdb/coff-pe-read.c
@@ -205,7 +205,10 @@ read_pe_exported_syms (struct objfile *o
 
   char const *target = bfd_get_target (objfile->obfd);
 
-  if ((strcmp (target, "pe-i386") != 0) && (strcmp (target, "pei-i386") != 0))
+  if ((strcmp (target, "pe-i386") != 0)
+      && (strcmp (target, "pei-i386") != 0)
+      && (strcmp (target, "pe-x86-64") != 0)
+      && (strcmp (target, "pei-x86-64") != 0))
     {
       /* This is not an i386 format file. Abort now, because the code
          is untested on anything else. *FIXME* test on further
@@ -216,15 +219,24 @@ read_pe_exported_syms (struct objfile *o
   /* Get pe_header, optional header and numbers of export entries.  */
   pe_header_offset = pe_get32 (dll, 0x3c);
   opthdr_ofs = pe_header_offset + 4 + 20;
+#ifdef _WIN64
+  num_entries = pe_get32 (dll, opthdr_ofs + 92 + 16);
+#else
   num_entries = pe_get32 (dll, opthdr_ofs + 92);
+#endif
 
   if (num_entries < 1)		/* No exports.  */
     {
       return;
     }
 
+#ifdef _WIN64
+  export_rva = pe_get32 (dll, opthdr_ofs + 96 + 16);
+  export_size = pe_get32 (dll, opthdr_ofs + 100 + 16);
+#else
   export_rva = pe_get32 (dll, opthdr_ofs + 96);
   export_size = pe_get32 (dll, opthdr_ofs + 100);
+#endif
   nsections = pe_get16 (dll, pe_header_offset + 4 + 2);
   secptr = (pe_header_offset + 4 + 20 +
 	    pe_get16 (dll, pe_header_offset + 4 + 16));

RE: [patch] Reading coff-pe-read files

[Pierre Muller]

  Should this be transformed so that we can also
correctly cross-read 64-bit PE files
on 32-bit and vice-versa?

  Possibly by defining an extra offset
that would be set to 16 if target is "pe-x86-64" or "pei-x86-64"
and to zero otherwise?


Pierre Muller
Pascal language support maintainer for GDB



> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Kai Tietz
> Envoyé : Wednesday, January 07, 2009 2:15 PM
> À : Joel Brobecker
> Cc : gdb-patches@sourceware.org
> Objet : [patch] Reading coff-pe-read files
> 
> Hi,
> 
> One small piece for x86_64 windows support in coff-pe-read.c seems to
> be missing. In function read_pe_exported_syms wrong indexes are used to
> find exports of the image.
> The source uses here the same pattern as in bfd/pe-dll.c.
> 
> ChangeLog
> 
> 2009-01-07  Kai Tietz  <kai.tietz@onevision.com>
> 
>         * coff-pe-read.c (read_pe_exported_syms): Enable read of PE+
> export directory.
> 
> 
> I am uncertain, which macro I should use here (or if it would be better
> to
> make for PE+ a copy of this file), so I used _WIN64 as condition.
> 
> Cheers,
> Kai
> 
> PS: I have already papers ready with FSF for gdb, so there shouldn't be
> any problem about taking parts of my donated  code, I've provided to
> Joel
> IMHO.
> 
> 
> 
> |  (\_/)  This is Bunny. Copy and paste Bunny
> | (='.'=) into your signature to help him gain
> | (")_(") world domination.