Re: [patch]: Unify and cleanup entry point handling for PE targets

[Kai Tietz <ktietz70 at googlemail dot com>]

Hi Dave,

2009/11/4 Dave Korn <dave.korn.cygwin@googlemail.com>:
> Kai Tietz wrote:
>
> ?Hi Kai. ?Thanks for bearing with me.

No problem. This were pretty long 24 hours ;)

>> this patch takes care that PE-coff targets are automatically using
>> DllMainCRTStartup entry point for --shared and/or --dll option. Also it
>> cleans up with the use of ENTRY in pe.em, pep.em, and its related
>> shell-scripts in emulparams.
>
> ?This is a nice tidy-up, thank you for taking the time :)
>
>> ? ? ? ? * emulparams/shpe.sh: Likewise.
>> ? ? ? ? Additional cleaned up with double defined
>> ? ? ? ? variables for SUBSYSTEM and INITIAL_SYMBOL_CHAR.
>
> ?"Additional" -> "Additionally" or "Also", "double defined" ->
> "double-defined", remove "with" and "for".
>
>> ? ? ? ? * emultempl/pe.em: Remove use of ENTRY.
>> ? ? ? ? (is_linked_dll): New local variable.
>> ? ? ? ? (pe_subsystem): New local variable.
>> ? ? ? ? (gld_XXX_before_parse): Don't set here default
>> ? ? ? ? entry point.
>
> ?"Don't set here X." -> "Don't set X here."
>
>> ? ? ? ? (set_entry_point): New function to set entry
>> ? ? ? ? point.
>> ? ? ? ? (set_pe_subsystem): Remove code for entry point.
>> ? ? ? ? (gld_XXX_handle_option): Set for OPTION_DLL value
>> ? ? ? ? is_linked_dll to true.
>
> ?"Set for X Y to true." -> "Set Y to true for X", or "For X, set Y to true".
>
>> ? ? ? ? (gld_XXX_after_parse):Use here set_entry_point.
>
> ?Missing space, and "Use here X." -> "Use X here."
>
>> ? ? ? ? * emultempl/pep.em: Likewise.
>
> ?In the code itself, just one thing:

Thanks for the spell-checking. I'll adjust ChangeLog as you suggest.

>> @@ -122,6 +121,8 @@
>>
>> ?static struct internal_extra_pe_aouthdr pe;
>> ?static int dll;
>> +static int is_linked_dll = 0;
>
> ?Is this new variable really necessary? ?I couldn't figure out any way that
> it could be set and the variable 'dll' above not also be set. ?When you give
> '-dll', the switch case OPTION_DLL sets 'is_linked_dll', and it calls
> 'set_pe_name ("__dll__", 1)', which sets the 'dll' variable also. ?I can't
> find any code that would notice a symbol called "__dll__" in one of the input
> BFDs and set the 'dll' variable based on it, so I think these two variables
> will always be in step, won't they?
Well, AFAICS those variable should be always in step. I just wanted to
avoid here to mix linker symbol values with internal state variables.
But if you think it is better to mix them, I can change this here.

>> This patch is tested for i686-pc-mingw32, and x86_64-pc-mingw32,
>> i686-pc-cygwin. For the bfd targets arm-epoc-pe, arm-wince-pe,
>> i686pe-posix, mips-pe, ppc-pe, arm-pe, sh-pe, and mcore-pe just cross
>> toolchain tests were done by me. All tests didn't produced any new
>> failures.
>
> ?One question: you've replaced a whole bunch of assignments by a table and
> some computation. ?How did you verify that the new code now produces *exactly*
> the same entry point symbols as the previous combination of defining ENTRY and
> having some #ifdefs in the emulation script? ?Did you do so mechanically, or
> manually? ?Or did you verify that there was a test in the testsuite that would
> fail if the entrypoint was wrong, and that was definitely run on all those
> platforms? ?If so, which one specifically?
Initial I did it mechanically, and then verified the x64, arm, and
i686 cases explicit.

>> Is this patch ok for apply?
>
> ?Pending only a resolution of whether the is_linked_dll variable is truly
> necessary, this is OK. ?I won't insist on testcases this time; it would be
> nice to have tests to compare the computed entrypoint from the linker under
> test to a hard-coded record of what the old linker used to use before the
> change, but I don't think this patch is likely to be a candidate for
> backporting to the release branch anyway, and you've done enough testing that
> it should be OK for head and we've got plenty of time between now and the next
> release to notice if anything has gone wrong.

Yes, I agree. If there are still some issues, which I don't expect
here, we have enough time to clean them up.

> ? ?cheers,
> ? ? ?DaveK
>
>


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