Re: initfini.c -> crt[in].S

[Roland McGrath <roland at hack dot frob dot com>]

> That seems fine as a deadline for this fix.

Ok.  I'll leave it to you to post/wikiate so somebody does the work for
each arch, and to follow up with the clean-ups in April (or as soon as
every arch has complied).

> Source line information ought to work; I've removed those options.  CFI is 
> riskier and I'm not sure it will work for these fragments in a useful way 
> given that they aren't complete functions (gas doesn't like .cfi_startproc 
> without matching .cfi_endproc).

startproc/endproc don't really need to coincide with functions as such.
They just drive the CFI resolution logic.  Anyway, CFI can be left for
later.  (But it might be good to do it soonish, so its presence here
sets the model for other arch code to follow.)

> Now just building crtn.S via the automatic rules (it gets built separately 
> in each directory, but the disassembly is the same).

We can probably just make the uses say $(csu-objpfx)crtn.o instead.
But it's fine to leave that until the final cleanup.

> I hope this version addresses all your comments including those I didn't 
> specifically remark on.

I think it's OK modulo the various small nits here.

> 	* csu/Makefile (generated): Do not append for crti.S in sysdirs.

The convention is to use "[conditional description] (thing): blah blah".
(Or "(thing) [...]:" if the conditionalization is inside the thing.)
It's fine to use an abstract description like yours when the condition
has hairy syntax like this one does.  But the [] convention makes it
clear you're talking about conditionalized code, which is not obvious
when you just write it out this way.

It's also fine to put a higher-level description at the top, so the log
entry is more explanatory.  That can either be:

	* csu/Makefile: Do blah blah to the frobnitz.
	(thing): detail blah.

if it's a high-level description of what's changing in this one file,
or:

	Blahrate the frob diddlers.
	* file1 (thing1): detail blah
	* file2 (thing2): detail blah

if it's a high-level description of what the whole log paragraph is about.

> 	* sysdeps/i386/crti.S, sysdeps/i386/crtn.S: New.  Based on
> 	compiler output for sysdeps/generic/initfini.c.
> 	* sysdeps/i386/elf/Makefile: Remove.
> 	* sysdeps/i386/Makefile (CFLAGS-initfini.s): Remove.

Personally I've always preferred "New file", "File removed", "Variable
removed", etc.  I find that easier to follow than simply relying on the
log syntax to indicate what granularity of object you're talking about.
(I have always used past tense, but I don't really object to using
imperative.)  I also favor "New file, based on ..." rather than even
less grammatical sentences.

> +ifeq (,$(wildcard $(sysdirs:%=%/crti.S)))

Please add a comment here about the reason for conditional kludgery, and
its temporariness.  It could include a link to a wiki page describing
the transition and deadline.

> +$(objpfx)crti.o: $(objpfx)pt-crti.o
> +	ln -f $< $@

In a later cleanup stage we should probably just change the uses
to name pt-crti.o directly.

> +#define PREINIT_FUNCTION __pthread_initialize_minimal_internal
> +#define PREINIT_FUNCTION_WEAK 0

Comment here about what the whole thing is for.

> +/* crti.S puts a function prologue at the beginning of the .init and
> +   .fini sections and defines global symbols for those addresses, so
> +   they can be called as functions.  */

Also mention here that the names _init and _fini are magical to the
linker, causing it to emit DT_INIT and DT_FINI.

> +	.hidden PREINIT_FUNCTION

What does .hidden even mean on an undefined symbol?
I don't think this has any practical effect, but I'm not sure.
(I'm being extra pedantic with this first file because it will
serve as the model other arch files will follow, so we should
make it as concise as possible.)

> +	.section	.init,"ax",@progbits
> +	.p2align	2
> +	.globl	_init
> +	.type	_init, @function
> +_init:
> +	pushl	%ebx
> +	subl	$8, %esp
> +	LOAD_PIC_REG (bx)
> +#if PREINIT_FUNCTION_WEAK
> +	movl PREINIT_FUNCTION@GOT(%ebx), %eax
> +	testl %eax, %eax
> +	je .Lno_weak_fn
> +	call PREINIT_FUNCTION@PLT

I don't have a strong opinion about whether to tab-align operands or
not, but please be consistent.  If you're going to use tabs, choose a
number of tabs that makes all the lines nearby line up exactly.  (My
tendency is mostly not to use tabs, roughly consistent with GNU style
for C et al where we don't usually tab-align assignments and such.)

Also, I think the SP adjustment here is to maintain 16-byte alignment.
A comment to that effect on the subl line seems appropriate.


Thanks,
Roland