Re: [PATCH] Add comments to i386 assembly source

[Markus Wichmann <nullplan@gmx.net>]

On Sat, Dec 30, 2017 at 08:15:41PM -0800, John Reiser wrote:
> On 12/23/2017 09:45 UTC, Markus Wichmann wrote:
> 
> > But then there's i386. Without comments, and pulling off some very black
> > magic, I thought it would be worth commenting the files at least in the
> > threads directory.
> 
> > -	mov $120,%al
> > +	mov $120,%al    /* __NR_clone */
> 
> Using an actual symbol is clearer and easier to maintain or modify:
> +__NR_clone = 120
> +	mov $__NR_clone,%al
> 

Modify? Syscall numbers on i386? That's not happening while Linus still
draws breath.

> Constant arguments to system calls (including the system call number)
> should be loaded last in order to provide the least constraints for computing
> non-constant arguments.  Also, it is not obvious that as values (%eax == %al).
> The value in %eax was set by "xor %eax,%eax; ...; mov %gs,%ax; ...; shr $3,%eax";
> what guarantees that (%gs <= (255 << 3)) ?  %gs could be as high as (8191 << 3).

Ooh, boy, I hadn't considered that. GS is set in __set_thread_area. As
far as I know, it will usually be rather small, but it might not be. The
ABI certainly doesn't enforce it, set_thread_area() returns an int,
which is a GDT index. The platform also doesn't enforce it: The selector
has 13 bits of space for an index, and the GDTR limit portion can store
16 bits of space in bytes, where each GDT member is at least 8 bytes
large, so the GDT can have at most 2^13 entries.

I mean, alright, on Linux, a GDT with 20 elements is large, and musl
only uses a single TLS segment (Linux treats the GDT as thread-local),
but an implementation with one GDT to rule them all is certainly
thinkable. And then 255 segments will be gone in the blink of an eye, as
every process needs at least one TLS segment.

> So _that_ deserves a comment; else for safety all of %eax should be set:
> +	push $__NR_clone; pop %eax   /* 3 bytes; __NR_clone < 128 */

Wouldn't a register move avoid memory references and therefore be
faster? And clearer, since not everyone knows that "push byte" will
sign-extend the value to 32 bits ("push word" will not).

Unless you are at it with Fefe's mindset (not a single byte wasted!), I
don't see a reason for such a micro-op.

> +	int $128            /* clone(flags, stack, TID pointer, {.index = current gs index, .base = thread pointer, .limit=0xfffff, .seg32_bit, .limit_in_pages, .usable}, td pointer) */
> 
> Clarity can be improved by using a symbol:
> NBPW = 4  /* Number of Bytes Per Word */
> 	mov 3*NBPW(%ebp),%ecx  /* ecx = stack */
> 	mov 4*NBPW(%ebp),%ebx  /* ebx = flags */
> etc.

Most people can multiply and divide by four in their heads. At least
such small numbers. Don't know about the rest of the world, but in my
locality, we learn the small multiplication table in third grade.

My biggest problem with this stuff was actually to figure out where the
arguments started, and what they were.

> 
> Incorrect comment:
> > +	sub $16,%ecx        /* align stack */
> Perhaps you meant "/* allocate space for returned segment descriptor */"?
> The alignment is performed by:
>  	and $-4*NBPW,%ecx  /* align for stack */
> 

Ah, it's one line of difference. At the time I still thought that I was
right, though, because at the very start, ecx points to the first byte
behind the stack. Even after alignment, that might still be the case.

Of course, that isn't a problem, since any time the processor pushes
something on stack, it's

*--sp = argument;

So sp itself need not point to a stack, so long as "--sp" will point to
one.

Besides, the lines directly below the one you quoted already push the
thread descriptor pointer to the new thread's stack.

> If you are aiming for small space then
> +	mov %eax,%ebx       /* exit(rv from function) */
> can be implemented one byte smaller as:
> +	xchg %eax,%ebx  /* syscall arg %ebx = rv from function; %eax = do not care */
> 

Another micro-op. Personally, I don't see the need, but I do see the
confusion using an xchg in place of a mov might cause. But alright,
that's what comments are for.

And another thing: I merely commented the files as they were. I
explicitly did not want to change the source code itself, for fear of
getting something wrong.

That's why I only voiced my concern with using GS in the manner of this
function, but did not actually patch it. Would be easy enough, just look
if the L bit is set in GS, and if so, don't provide the CLONE_SETTLS bit
in flags. But then the child would have to call __set_thread_area again,
and I did not research if that happens. We might have had to
special-case it for i386, and that means, we'd have to call
__set_thread_area from the child in __clone. Possible, but ugly.

Ciao,
Markus