This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: Patch: delay import for dlltool
- From: Dave Korn <dave dot korn dot cygwin at googlemail dot com>
- To: Timo Kreuzer <timo dot kreuzer at web dot de>
- Cc: Dave Korn <dave dot korn dot cygwin at googlemail dot com>, binutils at sourceware dot org
- Date: Mon, 24 Aug 2009 03:18:18 +0100
- Subject: Re: Patch: delay import for dlltool
- References: <4A884E7A.40103@web.de> <4A8AE347.2050403@gmail.com> <4A8B76B6.8090606@web.de>
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