Re: [RFC PATCH] Link the bootwrapper as a position-independent executable

[Paul Mackerras <paulus@samba.org>]

Segher Boessenkool writes:

> Hurray!  Looks good, just a few nits...

Thanks for reviewing.

> > +	bl	.+4
> > +p_base:	mflr	r10		/* r10 now points to runtime addr of p_base */
> 
> bl p_base  instead?

I went back and forth on that.  I ended up with it that way to
emphasize that the bl does need to branch to the *next* instruction
for the idiom to work.

> > +10:	or.	r8,r0,r9	/* skip relocation if we don't have both */
> >  	beq	3f
> 
> Either the code or the comment is wrong -- the code says "skip  
> relocation
> if we don't have either".

Ah, operator precedence rules in English. :)  I (and I think most
native English speakers) would parse my comment as "don't (have both)"
whereas I think you parsed it as "(don't have) both".  Similarly what
you say the code says parses as "don't (have either)" for me rather
than "(don't have) either".   IOW, "don't have either" means "both are
missing".

Anyway I can change the comment to say "we need both to do
relocation".  OK?

> > +	cmpwi	r0,22		/* R_PPC_RELATIVE */
> > +	bne	3f
> 
> It would be nice if there was some way to complain (at build time?)
> if there are unhandled relocations present.  Prevents a lot of headaches
> when things go wrong (and they will ;-) )

Yes, that would be a good idea.  There is one extra relocation when I
build for pSeries, which was for _platform_stack_top (which is
undefined), which we're OK ignoring.

> >  4:	dcbf	r0,r9
> >  	icbi	r0,r9
> 
> Fix these while you're at it?  It's not r0, it's 0.

Yeah.

> > +  .dynsym : { *(.dynsym) }
> > +  .dynstr : { *(.dynstr) }
> > +  .dynamic :
> > +  {
> > +    __dynamic_start = .;
> > +    *(.dynamic)
> > +  }
> > +  .hash : { *(.hash) }
> > +  .interp : { *(.interp) }
> > +  .rela.dyn : { *(.rela*) }
> 
> Do some of these sections need alignment?

I suppose they should ideally be 4-byte aligned.  We don't actually
use .dynsym, .dynstr, .hash or .interp, but I couldn't find any way to
make the linker omit them.

Paul.