From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17669 invoked by alias); 25 Jan 2008 16:13:14 -0000 Received: (qmail 17659 invoked by uid 22791); 25 Jan 2008 16:13:14 -0000 X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.31) with ESMTP; Fri, 25 Jan 2008 16:12:52 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 2694B2A9678; Fri, 25 Jan 2008 11:12:50 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id cUDbfvgwrSpH; Fri, 25 Jan 2008 11:12:50 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id AF6CE2A9649; Fri, 25 Jan 2008 11:12:49 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id 63584E7ACB; Fri, 25 Jan 2008 08:12:47 -0800 (PST) Date: Fri, 25 Jan 2008 16:38:00 -0000 From: Joel Brobecker To: Pierre Muller Cc: 'Mark Kettenis' , gdb-patches@sourceware.org Subject: Re: [RFA] i386-tdep.c: Add i386_skip_noop function Message-ID: <20080125161247.GJ3979@adacore.com> References: <000001c83b4a$573b4560$05b1d020$@u-strasbg.fr> <200712101854.lBAIs91J031646@brahms.sibelius.xs4all.nl> <002701c83be2$ac2a9a60$047fcf20$@u-strasbg.fr> <003101c85696$6f4d9e20$4de8da60$@u-strasbg.fr> <200801241750.m0OHoD5k007103@brahms.sibelius.xs4all.nl> <004701c85f53$2ef82dc0$8ce88940$@u-strasbg.fr> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <004701c85f53$2ef82dc0$8ce88940$@u-strasbg.fr> User-Agent: Mutt/1.4.2.2i Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2008-01/txt/msg00613.txt.bz2 Hi Pierre, This is not a formal review of your code - Mark is our de-facto maintainer so unless he asks for some help, I prefer to defer to him. But I thought I'd put a "Patch Champion" hat on, and make some tiny comments. > +/* Some Microsoft's system dll functions start with a I'm not a native English speaker (originally I'm French, as I suspect you are :), but the above sounds a little funny to me. I suggest either: - Some of Microsoft's system dll functions ... - Some functions in Microsoft's system dlls ... Also, you inserted a line-break a bit early IMO. It's not consistent with the line-length of the rest of the comment. But that's really very very minor - you might have thought that you wanted `mov %edi,%edi' and the word "instruction" on the same line, which is also a good argument. > + `mov %edi,%edi' instruction, which is effectively a two byte `nop'. ^^^^^^^^ I suggest "2-byte", see below. > + This instruction is used for hot patching support, together with 5 > + bytes of slack before the function. It would be nicer, IMO, if "5" and "bytes" were on the same line. It's easier to read. > Later, when hot-patching, the 2 "2-byte" (no space, a dash). > + byte op can be replaced with a relative jump to 5 bytes back. The 5 ^^ Is the "to" correct, here? To me, I think it should be "a relative jump 5 bytes back". > + A two byte nop is used to be sure that no thread is executing ^^^^^^^^ I suggest you remain consistent and use "2-byte" everywhere. > + the instruction at byte 1 of the function, so the patching can be > + performed atomically. */ > + > +/* 0x8b,0xff matches `mov %edi,%edi' */ > + if (op[0] == 0x8b && op[1] == 0xff) The practice in that file (and many other tdep files that I have worked on) is to just specify the instruction. Like so: if (op[0] == 0x8b && op[1] == 0xff) /* mov %edi,%edi */ > +/* Here other patterns can be added if found. */ I think that this comment in unnecessary, but check with Mark. > +/* Quoted from Mark Kettenis: > + "I've heard of a couple of code generation tools that do something > similar > + as Microsoft and insert nop instructions at the start of a function to > be > + patched up later. So other targets could benefit from the same code. > + And calling this function unconditionally keeps the code simple." */ I suggest that this comment be moved up, inside/after the comment explaining what happens in some DLL functions. You don't need to quote him, I think that it's better if you write something that connects better with what you wrote. For instance: Mark Kettenis (or maybe just "we") have heard of a couple of code generation tools taht do something similer. Otherwise, the code itself looks good to me! -- Joel