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

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

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