Re: [RFA] i386-tdep.c: Add i386_skip_noop function

[Joel Brobecker <brobecker at adacore dot com>]

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