From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/12288 Path: news.gmane.org!.POSTED!not-for-mail From: John Reiser Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH] Add comments to i386 assembly source Date: Sat, 30 Dec 2017 20:15:41 -0800 Message-ID: <72c68934-4445-c83d-7bbc-004953b2f9e9@bitwagon.com> References: <20171223094545.rmx6xtmucyz5xzap@voyager> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-Trace: blaine.gmane.org 1514693648 3862 195.159.176.226 (31 Dec 2017 04:14:08 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Sun, 31 Dec 2017 04:14:08 +0000 (UTC) User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 To: musl@lists.openwall.com Original-X-From: musl-return-12304-gllmg-musl=m.gmane.org@lists.openwall.com Sun Dec 31 05:14:04 2017 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by blaine.gmane.org with smtp (Exim 4.84_2) (envelope-from ) id 1eVV0b-0000Cr-2D for gllmg-musl@m.gmane.org; Sun, 31 Dec 2017 05:14:01 +0100 Original-Received: (qmail 9273 invoked by uid 550); 31 Dec 2017 04:15:56 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Original-Received: (qmail 9252 invoked from network); 31 Dec 2017 04:15:55 -0000 In-Reply-To: <20171223094545.rmx6xtmucyz5xzap@voyager> Content-Language: en-US Xref: news.gmane.org gmane.linux.lib.musl.general:12288 Archived-At: 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 */ --