This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFA] i386-tdep.c: Add i386_skip_noop function
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