Re: [patch ld PE-COFF]: Use bfd target's underscoring default and add option --(no-)leading-underscore for ld

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

On 23/03/2010 11:13, Kai Tietz wrote:

> this patch uses the definition of underscoring prefix defined in bfd 
> target's structure and makes emulparam INITIAL_SYMBOL_CHAR deprecated 

  Nice, thank you!

> (In
> fact there is no target which is using this anymore AFAICS, but I keep it 
> for possible use).

  I think we should delete it altogether, there's no point keeping it hanging
around if it's unused.  It'll only get bit-rotted and/or someone will start
using it again and we'll be back in the situation of having two inconsistent
mechanisms for the same thing.  Far better to remove it out of temptation's
way; if anyone ever really _really_ needs it, they will just have to reinvent
it (or for preference something a bit better.  Or for even more preference,
they will use the BFD underscore prefix and we won't go round this loop again!)

  I won't ask you to do that though.  If nobody objects in the next day or
two, I'll check in a follow-up patch to remove the last traces of it.  It
should just be a mechanical clean-up.

> I tested this patch for i686-pc-mingw32, x86_64-pc-mingw32, and 
> i686-pc-cygwin without any regressions. Is this patch ok for apply?

  The patch is OK in principle, but there's a couple of suggestions I'd like
you to consider first:

> +      sy += ((!init[j].is_lead_underscore || pe_leading_underscore) ? 0 : 1);

> +		  if (! CONST_STRNEQ (symbols[i]->name,
> +		  		      (pe_leading_underscore == 0 ? "_head_"
> +		  		      				  : "__head_")))


  These two constructs appear repeatedly in the code: getting a pointer to a
string and skipping one leading char if no leading underscore, and doing a
string comparison against a constant string with or without a leading
underscore.  They should be wrapped in macros or inline functions if possible.
 (It would also let you generate the prefixed form of the string constant by
string concatenation, avoiding risk of typoing one of the two).

  Also, there are a bunch of white space nits where e.g. a line has spaces at
the start then TABs, or mixed TABs and spaces where it should all be TABs;
please have a quick look over for those before you commit it.

  This one's doing my head in.  Possibly because I'm on a really odd
sleep/wake cycle at the moment and I haven't been awake long, but ...

> (!init[j].is_lead_underscore || pe_leading_underscore) ? 0 : 1

  I keep looking at this phrase, and thinking, "Hang on?  Shouldn't the not be
outside the brackets?"  There's no comment explaining exactly what the
is_lead_underscore member signifies, and it looks like it's only there to
support the ...

> -  D(ImageBase, U ("__ImageBase"), NT_EXE_IMAGE_BASE),

> +  D(ImageBase, "___ImageBase", NT_EXE_IMAGE_BASE, TRUE),

... change in the prefixed/unprefixed-ness of the ImageBase name in the init
table.  Would you mind explaining it?  I don't yet get why not just keep the
unprefixed form of the name in the init table and not have is_lead_underscore
at all.  Semantically, it amounts more to a "this one is the one that gets the
special handling for ImageBase" flag than anything else, I think.

  (While you're thinking about that, I'll give it a bit of testing on cegcc
and pals.)

    cheers,
      DaveK