Re: [BUG?] Re: [patch]: Fix auto-import for pe targets

["Dave Korn" <dave dot korn dot cygwin at googlemail dot com>]

Kai Tietz wrote:

> The IAT is broken, because it is part of the IMPORT directory and by the
> gap introduced by make_head. By comparing vs and binutils
> output and directory entries the following layout differences cab be
> shown:
>
> For Directory entries in pe(+) header:
>  - binutils places into PE_IMPORT_TABLE the address to idata and set the
> size to section size of (idata).
>  - binutils doesn't handle the the PE_IMPORT_ADDRESS_TABLE at all (and I
> assume the wrong builded size of idata$5)
>
>  - vc places into PE_IMPORT_TABLE the address of idata$2 and set the size
> to of it just to of idata$2 (plus the null entry).
>  - vc places into PE_IMPORT_ADDRESS_TABLE to address of idata$5 and sizeof
> merged idata$5.
>
> The layout for idata looks like this in general (beside that idata$5
> should be placed into writable section and for ppc idata$6/idata$7 are
> flipped in meaning)
>
> IMPORT_DIRECTORY_START: .idata$2*
> (binutils only) .idata$3 (IIUC this isn't used by any target anymore)
> (+zero-idata$2 entry)
> IMPORT_DIRECTORY_END:
> ILT_START:
> .idata$4*
> ILT_END:
> IAT_START:
> .idata$5*
> END_START:
> HINT_NAME_TABLE_START:
> .idata$6*
> HINT_NAME_TABLE_END:
> DLL_NAME_START:
> .idata$7*
> DLL_NAME_END:
>
> As I already said all these regions are placed by binutils into one
> section. And PE_IMPORT_TABLE is set as
> (va:IMPORT_DIRECTORY_START,size:DLL_NAME_END-IMPORT_DIRECTORY_START).
> IMPORT_ADDRESS_TABLE isn't set at all.
>
> As I read the 8.1 coff spec and observed by vs (without any section
> merging features enabled) The following places are used:
> PE_IMPORT_TABLE is set to
> (va:IMPORT_DIRECTORY_START,size:IMPORT_DIRECTORY_END-IMPORT_DIRECTORY_START)
> and IMPORT_DIRECTORY_START is placed normally in .rdata(.idata)
> ILT_START-ILT_END is put into (.rdata/.idata)
> DLL_NAME_START-DLL_NAME_END is put into (.rdata/.idata)
> HINT_NAME_TABLE_START-HINT_NAME_TABLE_END) is put into (.rdata/.idata) but
> only when used.

  Yes, I fully agree with your reading of the spec about the values that
should be in the data directory entries.

> IMPORT_ADDRESS_TABLE is set (va:IAT_START,size:IAT_END-IAT_START) is
> placed into .data

  I'm slightly less sure it actually /specifies/ this section placement.  The
diagram (fig. 3, sec. 6.4) doesn't show the IAT, true.  But sec. 6.4.4, about
the IAT, doesn't actually specify which section to place it in, and there is
nothing which specifies what /should/ go in the .data section.  So we have
only the observed fact that MSVC puts it there and it works, and that binutils
puts it in .idata and it also works.  Do you happen to know whether perhaps it
only works /because/ binutils fails to set the data directory entry pointing
to it?

> So if you want to export via IAT_DIRECTORY_ENTRY in PE(+) image header,
> the IAT begins with a leading zero and is therefore at least one element
> too big.

  And indeed it contains embedded zeros in between the arrays of entries,
each time another DLL or import library is linked.  Ugly.

> So I would elminate the writing of the leading zero element in make_one()
> and adjust dlltool for this, too. First there seems to be no need for
> this, because otherwise the fixup import directory element would need it,
> too. Secondly it is just a waste of memory.

  Thanks Kai, this explanation gave me the missing parts of the story.  I was
also helped off-list by a poster who goes by the handle of "mosfet" who showed
me his research findings.

  I agree that on the face of it, binutils is setting the wrong/no values in
the data directory table PE_IMPORT_TABLE/PE_IMPORT_ADDRESS_TABLE entries.  I
think that a future patch changing the code to set them as per MSVC would be
the right thing to do.

  I tested your patch.  It worked fine when I built cygdstdc++-6.dll and ran
the g++/libstdc++ testsuites.

  As to the leading zeros, I'm still a bit concerned, because they were
obviously put there deliberately and so there must have been a reason, at
least at the time, and neither of us know what that reason was.  But at the
same time, I really can't see what possible use they are, and MSVC doesn't
leave any such gaps.  So I can't see any reason not to just go ahead and
remove them; if Nick hadn't asked for a command-line options, I would have
been ready to just go ahead and try it and wait to see if any problems showed
up.  In general, I think we want GNU tools to try and emulate platform formats
accurately and in depth, don't we?

    cheers,
      DaveK