RE: bfd/peigen.c problems and fix

[Donn Terry <donnte at microsoft dot com>]

Hmmm...  I don't see run into problem.  However, it may be due to a
compatabilty
issue.  I don't use any of the look-alike variants, and hew VERY closely
to matching the official PE results.  (I compare my results with the same
thing generated with MSVC, and expect the results to be "the same" (whatever
that means).)  However the cygwin and (as far as I can tell) mingw variants
end up making other valid (but different) choices.  I don't remember my
reasoning (from close to 3 years ago now) in the work I did in this area
(so I'm recreating it on the fly).

There some versionitis between my current copy (which I can't *yet* submit)
and
the CVS copy.  However, in the case of add_data_entry, I've gone further
and made 'base' ATTRIBUTE_UNUSED.  (Ian has earlier copies of this that
are unencumbered.)

In looking at an objdump -p of a MSVC-generated a.out (yes... on Interix
that statement makes sense) the data offset column for the Data Directory
contains values which do NOT contain the value of ImageBase (that is they
are RVAs).  The change you propose would change them into absolute VAs
(unless theres additional magic I'm not seeing.  Here's a cut of what I get
(from MSVC/LINK.EXE generated code):

Entry 0 00000000 00000000 Export Directory [.edata (or where ever we found
it)]
Entry 1 00012000 00000028 Import Directory [parts of .idata]
Entry 2 00000000 00000000 Resource Directory [.rsrc]
Entry 3 00000000 00000000 Exception Directory [.pdata]
Entry 4 00000000 00000000 Security Directory
Entry 5 00013000 00000594 Base Relocation Directory [.reloc]
Entry 6 0000f020 00000054 Debug Directory
Entry 7 00000000 00000000 Description Directory
Entry 8 00000000 00000000 Special Directory
Entry 9 00000000 00000000 Thread Storage Directory [.tls]
Entry a 00000000 00000000 Load Configuration Directory
Entry b 00000000 00000000 Bound Import Directory
Entry c 000120a4 0000007c Import Address Table Directory
Entry d 00000000 00000000 Reserved
Entry e 00000000 00000000 Reserved
Entry f 00000000 00000000 Reserved
           ^
           This column

If I understand the "add_data_entry" part of your proposed change
correctly, it would have the effect of adding 0x400000 (in effect) to every
value in "This column" above. For maximal MS compatability, that's wrong.
(I
believe things "work" with ImageBase added in, in most instances, however
I'm pretty sure that it would break at least one thing: the non-intel
architectures
that use .pdata for stack unwind.  That's an experiment I'd have to
run that I don't have time to.)

It looks as if the other changes are either similar or the complement
(backing out the value of ImageBase).   My inclination would be to look
for an alternative (possibly something in the set of things that Ian
hasn't yet applied from before the Softway aquisition) that keeps the
values in the executable "the same" as what MSVC generates.

(If I'm wrong in my supposition about what the effect on the
Data Directory is, please let me know and I'll look more closely.)

Donn

P.S.  Speaking (most assuredly!) only for myself.


> -----Original Message-----
> From: Alan Modra [mailto:alan@linuxcare.com.au]
> Sent: Wednesday, May 03, 2000 7:00 AM
> To: Donn Terry
> Cc: binutils@sourceware.cygnus.com
> Subject: bfd/peigen.c problems and fix
> 
> 
> Hi Donn,
>    Martin Kahlert pointed out that mingw32 is broken, and has been for
> quite a while.  I also managed to get some segv's from objdump -p on
> pei files.  Since this problem seems to be in some of your 
> code, would you
> mind looking over my fix?
> 
> Regards, Alan Modra
> 
> 
> On Wed, 3 May 2000, Martin Kahlert 
> <martin.kahlert@mchp.siemens.de> wrote:
> 
> > I compiled a new patched binutils and integrated that into 
> the cross compiler
> > toolchain as i did with my previous tests.
> > The toy progs i tested worked fine.
> > I will try to build a complete cross compiler toolchain
> > from ground and make a full compiler bootstrap - just to 
> become sure.
> > But at first glance, your patch works.
> 
> -- 
> Linuxcare.  Support for the Revolution.
> 
> 
> bfd/ChangeLog
> 	* peigen.c (_bfd_pei_swap_aouthdr_out): Pass ImageBase to
> 	add_data_entry.  DataDirectory virtual address is relative.
> 	(pe_print_idata): Account for relative DataDirectory virtual
> 	addresses.
> 	(pe_print_edata): Similarly.
> 
> Index: bfd/peigen.c
> ===================================================================
> RCS file: /cvs/src/src/bfd/peigen.c,v
> retrieving revision 1.8
> diff -u -p -r1.8 peigen.c
> --- peigen.c	2000/04/18 04:03:16	1.8
> +++ peigen.c	2000/05/03 09:12:29
> @@ -594,7 +594,7 @@ _bfd_pei_swap_aouthdr_out (abfd, in, out
>    /* first null out all data directory entries .. */
>    memset (extra->DataDirectory, sizeof (extra->DataDirectory), 0);
>  
> -  add_data_entry (abfd, extra, 0, ".edata", 0);
> +  add_data_entry (abfd, extra, 0, ".edata", ib);
>  
>    /* Don't call add_data_entry for .idata$2 or .idata$5.  
> It's done in
>       bfd_coff_final_link where all the required information is
> @@ -602,11 +602,11 @@ _bfd_pei_swap_aouthdr_out (abfd, in, out
>  
>    /* However, until other .idata fixes are made (pending patch), the
>       entry for .idata is needed for backwards compatability. 
>  FIXME.  */
> -  add_data_entry (abfd, extra, 1, ".idata" ,0);
> +  add_data_entry (abfd, extra, 1, ".idata" , ib);
>  
> -  add_data_entry (abfd, extra, 2, ".rsrc" ,0);
> +  add_data_entry (abfd, extra, 2, ".rsrc" , ib);
>  
> -  add_data_entry (abfd, extra, 3, ".pdata", 0);
> +  add_data_entry (abfd, extra, 3, ".pdata", ib);
>  
>    /* For some reason, the virtual size (which is what's set by
>       add_data_entry) for .reloc is not the same as the size recorded
> @@ -614,7 +614,7 @@ _bfd_pei_swap_aouthdr_out (abfd, in, out
>       but since it's the best we've got, use it.  It does do the right
>       thing for .pdata.  */
>    if (pe_data (abfd)->has_reloc_section)
> -    add_data_entry (abfd, extra, 5, ".reloc", 0);
> +    add_data_entry (abfd, extra, 5, ".reloc", ib);
>  
>    {
>      asection *sec;
> @@ -1062,6 +1062,7 @@ pe_print_idata (abfd, vfile)
>        if (addr == 0 || size == 0)
>  	return true;
>  
> +      addr += extra->ImageBase;
>        for (section = abfd->sections; section != NULL; 
> section = section->next)
>  	{
>  	   if (addr >= section->vma
> @@ -1076,7 +1077,7 @@ pe_print_idata (abfd, vfile)
>  	}
>  
>        fprintf (file, _("\nThere is an import table in %s at 
> 0x%lx\n"),
> -	       section->name, (unsigned long)addr);
> +	       section->name, (unsigned long) addr);
>  
>        dataoff = addr - section->vma;
>        datasize = size;
> @@ -1144,7 +1145,7 @@ pe_print_idata (abfd, vfile)
>    if (! bfd_get_section_contents (abfd, section, (PTR) data, 
> 0, secsize))
>      return false;
>  
> -  adj = - section->vma;
> +  adj = - (section->vma - extra->ImageBase);
>  
>    for (i = 0; i < datasize; i += onaline)
>      {
> @@ -1157,10 +1158,9 @@ pe_print_idata (abfd, vfile)
>        bfd_size_type j;
>        char *dll;
>  
> -      fprintf (file,
> -	       " %08lx\t",
> -	       (unsigned long int) (i + section->vma + dataoff));
> -      
> +      fprintf (file, " %08lx\t",
> +	       (unsigned long) (i + section->vma - 
> extra->ImageBase + dataoff));
> +
>        if (i + 20 > datasize)
>  	{
>  	  /* check stuff */
> @@ -1183,7 +1183,7 @@ pe_print_idata (abfd, vfile)
>        if (hint_addr == 0 && first_thunk == 0)
>  	break;
>  
> -      dll = (char *) data + dll_name - section->vma + dataoff;
> +      dll = (char *) data + dll_name + adj;
>        fprintf(file, _("\n\tDLL Name: %s\n"), dll);
>  
>        if (hint_addr != 0)
> @@ -1337,6 +1337,7 @@ pe_print_edata (abfd, vfile)
>        if (addr == 0 || size == 0)
>  	return true;
>  
> +      addr += extra->ImageBase;
>        for (section = abfd->sections; section != NULL; 
> section = section->next)
>  	{
>  	   if (addr >= section->vma
> @@ -1378,7 +1379,7 @@ pe_print_edata (abfd, vfile)
>    edt.npt_addr       = bfd_get_32(abfd, data+32);
>    edt.ot_addr        = bfd_get_32(abfd, data+36);
>  
> -  adj = - (section->vma + dataoff);
> +  adj = - (section->vma - extra->ImageBase + dataoff);
>  
>    /* Dump the EDT first first */
>    fprintf(file,
>