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

[Kai Tietz]

Hello all,

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.

2009-10-28  Kai Tietz  <kai.tietz@...>

        * emulparams/arm_epoc_pe.sh: Remove ENTRY.
        * emulparams/arm_wince_pe.sh: Likewise.
        * emulparams/i386pe.sh: Likewise.
        * emulparams/i386pe_posix.sh: Likewise.
        * emulparams/mcorepe.sh: Likewise.
        * emulparams/mipspe.sh: Likewise.
        * emulparams/ppcpe.sh: Likewise.
        * emulparams/armpe.sh: Likewise.
        * emulparams/i386pep.sh: Likewise.
        * emulparams/shpe.sh: Likewise.
        Additional cleaned up with double defined
        variables for SUBSYSTEM and INITIAL_SYMBOL_CHAR.
        * 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.
        (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.
        (gld_XXX_after_parse):Use here set_entry_point.
        * emultempl/pep.em: Likewise.

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. Is this patch ok for apply?

Cheers,
Kai

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

[Dave Korn-6]

Kai Tietz wrote:
> Hello all,
>
> 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.

  I'll need 24 hours to study this one, will get back to you tomorrow.

    cheers,
      DaveK

[Dave Korn-6]

Kai Tietz wrote:

  Hi Kai.  Thanks for bearing with me.

> 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:

> @@ -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?

> 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?

> 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.

    cheers,
      DaveK

[Kai Tietz-2]

Hi Dave,

2009/11/4 Dave Korn <dave.korn.cygwin@...>:
> 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

[Dave Korn-6]

Kai Tietz wrote:

>>  Hi Kai.  Thanks for bearing with me.
>
> No problem. This were pretty long 24 hours ;)

  Yes, and for 48 of those 24 hours I was even off the air owing to a blown-up
PSU!

>>> +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.

  I see what you're getting at; conceptually these are two different things,
one is a command-line switch, the other the value of a symbol.  However, the
value of the symbol is what the command-line option is there to specify, so
it's still a very direct test of the command-line option's presence or
absence.  And also, I wonder if one day we'll want to let people define those
symbols in input .o files to a final link and cause it to have the same affect
as specifying those corresponding command-line options.  So I'd suggest just
using the state of the 'dll' variable to decide about the entrypoint and
eliminate 'is_linked_dll'.

> Initial I did it mechanically, and then verified the x64, arm, and
> i686 cases explicit.

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

  Yep, your testing seems adequate to have covered all the popular platforms;
patch is OK once the redundant variable has been removed.

    cheers,
      DaveK

[Kai Tietz-2]

Adjusted to use just the dll variable here. And committed with
ChangeLog corrections.

Cheers,
Kai