This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Patch: delay import for dlltool


Timo Kreuzer wrote:
> Dave Korn wrote:
>> Timo Kreuzer wrote:
>>
>>   
>>> Let me know if something is missing.
>>>     
>>   Documentation in binutils/doc/binutils.texi, and some testcases would be
>> nice.  Also a ChangeLog entry.
>>   
> I have attached an updated patch with the documentation.

  A few coding style comments arise, nothing major; you don't need to respin
the patch for these, I'll fix them.

> +  0xFF, 0x25, 0x00, 0x00, 0x00, 0x00, // jmp __imp__function
> +  0xB8, 0x00, 0x00, 0x00, 0x00,       // mov eax, offset __imp__function
> +  0xE9, 0x00, 0x00, 0x00, 0x00        // jmp __tailMerge__dllname

  We only use C-style comments in binutils for portability.

> +	  if (delay) break;

  We always put a line break after the condition.

> +  fprintf(f,"\t%s\t%s\n", ASM_GLOBAL,head_label);

  Missed a space there.

> +      fprintf (f, "\t.section	.idata$4\n");
> +      fprintf (f, "__INT_%s:\n", imp_name_lab);
> +    }
> +
> +  fprintf (f, "\t.section	.idata$2\n");

  These strings have real embedded TABs; they should be \t.

> -  ar_head = make_head ();
> +  if (delay)
> +    {
> +      ar_head = make_delay_head();
> +    }
> +  else
> +    {
> +      ar_head = make_head();
> +    }

  Shouldn't delete the space between the function name and the brackets, even
if it does match the (incorrect) example a couple of lines below.

  Other than those tiny trivia, this patch is spotless.  Nice work!

> I don't really know how to write a testcase. I have attatched a tiny C
> program. link it with delayimp.c and a delay-import library created from
> the shlwapi.def. it simply uses StrToIntA to convert the first parameter
> to an integer and return that. Maybe you can make something from that ;)

  I'll whip one up when I get some spare time.  It can run dlltool with the -y
option and use 'nm' or 'objdump' to ensure the generated file has an undefined
reference to __delayLoadHelper2.  (If you want to have a go, take a look at
binutils/testsuite/binutils-all/dlltool.exp, it's fairly obvious what's going
on and how to extend it.)

> changelog: "Add support for delay importing to dlltool. Use the
> --output-delaylib <file> switch to create a delay-import library. The
> resulting app will load the dll as soon as the first function is called.
> It will link to __delayLoadHelper2() from the static delayimp library,
> which will import LoadLibraryA and GetProcAddress from kernel32."

  That's not how a ChangeLog entry goes, although we could use the first
sentence or two for a NEWS item.  Please take a browse through the
binutils/ChangeLog file to see examples of how they're written, and read the
chapter about how to write a changelog entry in the GNU coding standards:

    http://www.gnu.org/prep/standards/standards.html#Change-Logs

  Once you've written a ChangeLog entry and your paperwork is confirmed to
have arrived back at the FSF, this patch is going to be OK, and I'll check it
in for you.  Thank you for contributing this new feature :)

    cheers,
      DaveK


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]