Re: [PATCH] Add comments to i386 assembly source

[Rich Felker <dalias@libc.org>]

The context of your reply was a bit confusing. Markus's patch is about
adding comments to code which he did not write but is trying to make
it easier to understand. Changes to the asm being commented are mostly
off-topic. If there are clear errors or inefficiencies found as part
of the commenting process, they may require additional patches, but
including any such changes (things that alter the machine code) in a
cosmetic patch would be reason for rejection. Cosmetic patches always
need to be machine-verifiable as cosmetic-only.

Now on to the actual review:

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

The style in Markus's patch is what's preferred in musl. At first I
thought you were suggesting preprocessed asm, but now I see you're
using asm symbols/labels, which I suppose works but needs to be
considered for how it affects symbol tables (local only, I think, but
does affect output .o files) and whether it makes a compatibility
difference for existing or hypothetical new assemblers. In general I
don't like to enlarge the set of features we're relying on
unnecessarily.

The main time when I think symbolic constants have a strong benefit
over comments is when their value can change, which is not the case
here.

> 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).
> So _that_ deserves a comment; else for safety all of %eax should be set:
> +	push $__NR_clone; pop %eax   /* 3 bytes; __NR_clone < 128 */
> +	int $128            /* clone(flags, stack, TID pointer, {.index = current gs index, .base = thread pointer, .limit=0xfffff, .seg32_bit, .limit_in_pages, .usable}, td pointer) */

I don't follow your reasoning here. Where are you getting the possible
range of %gs from? If __clone is called with flags relevant to thread
creation, %gs is necessarily a GDT entry. The LDT stuff in i386's
__set_thread_area is only used to provide a working %gs for
single-threaded processes on ancient kernels that lack thread support;
in this case pthread_create always fails without calling __clone.

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

While there's an argument to be made that 3*4 and 4*4 should be used
here (I believe there are some files written that way) it's not done
consistently that way now, and it's readable either way.

> 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 */

On a quick re-reading, the allocation seems to be to make space for
the argument to the start function in the new thread/process. It's
loaded from 20(%ebp) and stored at (%ecx) (the new stack) so that it
will get passed when the new thread/process executes the call insn
below.

Thank you and Markus for reminding me why some comments would help
here. :-)

> 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 */

I think this kind of change is just confusing and contrary to the
intent to make the code easier to understand. Saving a few bytes/insns
(bytes probably only if it reduced cache lines) in memset or memcpy
might be worthwhile but in a syscall it's pointless.

Rich