Re: [patch] [i386] Put hlt at the ON_STACK breakpoint [Re: GDB 7.4.91 available for testing]

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

On Mon, 23 Jul 2012 17:59:51 +0200, Joel Brobecker wrote:
> It even seems to me that this should be done on all platforms, no?

Yes; just looking at the other archs it was not trivial to me so I wanted to
be sure it at least really helps valgrind.


> And instead of writing an instruction of the
> arch's choosing, why not write the breakpoint trap instruction?

It depends on the opinion.  I wanted to make clear there is a real GDB
breakpoint on top of it and in normal GDB control flow the 'hlt' instruction
is never executed by CPU.  There are other reasons why to put int3 there,
maybe it would be more clear to the users that way when I think about it now.

It also does not matter much, users do not probably disassemble
the <function called from gdb> frame.


> > +  /* This hlt instruction is never executed.  */
> > +  static const bfd_byte hlt = 0xf4;
> 
> Why make it static? Isn't that going to force the compiler to make
> that variable global (put into RO section)?

It is a nitpick but 'static const' is more effective than just 'const'.
In the latter case compiler has to create the storage for variable on stack
(probably to at least ensure its unique address), in the former case it just
takes an address of .rodata variable, which is generally cheaper.

const
   4:	48 8d 7c 24 08       	lea    0x8(%rsp),%rdi
   9:	c7 44 24 08 2a 00 00 	movl   $0x2a,0x8(%rsp)
  10:	00 
  [Nr] Name              Type            Address          Off    Size   ES Flg Lk Inf Al
  ---- no .rodata
vs.
static const
   4:	bf 00 00 00 00       	mov    $0x0,%edi
			5: R_X86_64_32	.rodata
  [Nr] Name              Type            Address          Off    Size   ES Flg Lk Inf Al
  [ 5] .rodata           PROGBITS        0000000000000000 00005c 000004 00   A  0   0  4


> I suggest merging the two comments into one at the point where the
> intruction is written.
> 
>      /* Write an legitimate instruction at the point where the infcall
>         breakpoint is going to be inserted.  While this instruction
>         is never going to be executed, a user investigating the memory
>         from GDB would see this instruction instead of random
>         uninitialized bytes.  */

OK, I will update the patch later for check-in.


Thanks,
Jan