Re: [PATCH] Add comments to i386 assembly source

[John Reiser <jreiser@bitwagon.com>]

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

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

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.

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

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

--