From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/12289 Path: news.gmane.org!.POSTED!not-for-mail From: Markus Wichmann Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH] Add comments to i386 assembly source Date: Sun, 31 Dec 2017 07:54:33 +0100 Message-ID: <20171231065433.ewpdfi7so2xohqvb@voyager> References: <20171223094545.rmx6xtmucyz5xzap@voyager> <72c68934-4445-c83d-7bbc-004953b2f9e9@bitwagon.com> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Trace: blaine.gmane.org 1514703191 11184 195.159.176.226 (31 Dec 2017 06:53:11 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Sun, 31 Dec 2017 06:53:11 +0000 (UTC) User-Agent: NeoMutt/20170609 (1.8.3) To: musl@lists.openwall.com Original-X-From: musl-return-12305-gllmg-musl=m.gmane.org@lists.openwall.com Sun Dec 31 07:53:07 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 1eVXUV-0002Pm-4N for gllmg-musl@m.gmane.org; Sun, 31 Dec 2017 07:53:03 +0100 Original-Received: (qmail 28138 invoked by uid 550); 31 Dec 2017 06:55:04 -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 28120 invoked from network); 31 Dec 2017 06:55:03 -0000 Content-Disposition: inline In-Reply-To: <72c68934-4445-c83d-7bbc-004953b2f9e9@bitwagon.com> X-Provags-ID: V03:K0:pET+L0usJ2oiS4/jnpGu4w0kbPw5nOZbWyWAO1mGPpX3ghDpN/1 hRg+rJw2c0wMlQWSsub1tABNY0Z/G+ldBU5uLCZrRG6qAh35Rg44llFxKgPHAGTdoQpdPpR H0a+syCh6VjYKDL6aOaikHJgBHhKXNbIlnBF2yJGu5+5yPgQlLJUKQhjGaKsDGtAO9Kl3H3 2mJID8jneA6ae/OAFBqZw== X-UI-Out-Filterresults: notjunk:1;V01:K0:xqiVYVwnw7Y=:AZc+DrfCdDYn2596Mqtj91 cxD0U9oYBGJmtckewinK4rQmLxakkKTiY60sy9yjqe1/aGL2MqXMrzdpnxxr7UYu0aOLeNeXh nTi1NKM4N4+qxR/ANQgS/ZqrfpPSt45mLJbRruj0V7HtHNd2zCoFTJ+FLgQ9R0GvlCuKltOPX YgcdJmH+OBtBOG2Er78fjK18bzGYRNvFoRlz8VJywPz8+Olb8Bikg21WQVK+yhGDba2hL5sgU MWbPh0HFON04MRrQrRgD3LR8AgoTeXCKRZObcnBEDHVjNJd6WBpZ/3a6uY+wDVX1wVDCVyrOP V9L9t1n9vaCYl6Tt1PzFFqmXGzx9dNadhlOuQxeEj0BYanMBYu9afgSZ6iWf56izmt4lRN3CN H1nM5a2T3B3TQmyW9287V1fqSSXqPnn6txJGjt6A2stD50I4/kW0EObNNLgxBV6hF8tmBtmoB sHcV7WM93C3wYZToLFgTa5JR8araLk075aDb+IFJtnovp2gWk5zqOAUTfqvdTWrmnTA5d9HvJ bcFUIR1u5y0NBmCKnVteZ1uwYiAdVn3GSrPWERtpGjbMUGkbLYjG+ki6iTlyPplkbcYp5b6Bn WQ8hP68gCYk8CwTdfAelvI+pxPWmaM1b1iQY8uEHiAt/nMs6Qt/vbLlYJJ1q8WQUUCT4mzJ7L zqw+SImrCNZrjCFB7xZbSJU/ZG1aUu0Arqs2ZN7CRXY3ijrwD9xlYcaUPdwpJJvrwKKf27NoS FN9+cCyMcKLjSYPeDkDgYa0Qk4TCrcudan6UCj6XHPfDdPm4CneI0z2F0sGzQH/nuqbFJalY Xref: news.gmane.org gmane.linux.lib.musl.general:12289 Archived-At: 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