|
Message-ID: <20171231154926.GG1627@brightrain.aerifal.cx> Date: Sun, 31 Dec 2017 10:49:26 -0500 From: Rich Felker <dalias@...c.org> To: musl@...ts.openwall.com Subject: Re: [PATCH] Add comments to i386 assembly source 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
Powered by blists - more mailing lists
Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.