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

[Kai Tietz <ktietz70 at googlemail dot com>]

2010/3/23 Dave Korn <dave.korn.cygwin@googlemail.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!)

Agreed.

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

Thanks.

>> 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_")))
>

Well, I agree that those patterns are repeating. The reason why I
avoid to make helper-macros here was to show logic explicit and the
arguments are varying a bit, so that in fact at least two macros are
necessary.
But I can rework it to use a macro version for it, if you prefer.

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

Yes, in general ?: operators and boolean-expressions are frail for typos.

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

Ok, before I commit, I will give it a whirl for it. This must by my editor :(

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

Well, this issue isn't nice looking but IMHO necessary. The point here
is that __ImageBase is the name of the symbol-name used in C. So an
additional underscore is necessary for targets prefixing C-symbols.

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

Hope it doesn't show side-effects there. I assume none, but well ...

Cheers,
Kai


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