[Lldb-commits] [lldb] [lldb] Step over non-lldb breakpoints (PR #174348)

[David Spickett via lldb-commits]

DavidSpickett wrote:

> I think I have also fixed the Arm 32 failure, as I'd just made mistakes in the masks inside the IsValidTrapInstruction implementation. I don't really have a good way to test that other than to say that the updated patterns are more correct.

It would be fine to reland the PR with these and just see what happens. We will have results from the ARM bot within a few hours and worst case we revert again. If it needs more intervention we can disable the feature for ARM and get Linaro to figure out the details on the real hardware.

> I haven't pushed updates to the branch yet though I can likely do that later on today, and I can open a new PR when I do.

I think GitHub will require you to do that, but regardless, relands should be a new PR. You can reference the original change and say what you've added to fix which problems.

> This is a different trap to the one that we saw originally, but there's not really any way to know that, so we step over that one as well, and the result is that two instructions are stepped over for the price of one.

Naive suggestion: we could look at the PC value. Unless the process branches back to the same PC and so generates a new trap. Hmm.

> This isn't a problem in other cases where there's only one trap, because then (most likely) the original fix from 5 years ago will handle it first, and the new code will see no trap to step over, so will do nothing, handing control back to the user.

I think the situation looks like this:
```
int main() {
  __builtin_debugtrap();
  __builtin_debugtrap();
  return 0;
}
```
I tried this on AArch64 Linux (which uses lldb-server):
```
$ ./bin/lldb /tmp/test.o
(lldb) target create "/tmp/test.o"
Current executable set to '/tmp/test.o' (aarch64).
(lldb) run
Process 7804 launched: '/tmp/test.o' (aarch64)
Process 7804 stopped
* thread #1, name = 'test.o', stop reason = signal SIGTRAP
    frame #0: 0x0000aaaaaaaa0724 test.o`main at test.c:3:3
   1   	int main() {
   2   	  __builtin_debugtrap();
-> 3   	  __builtin_debugtrap();
   4   	  return 0;
   5   	}
(lldb) dis
test.o`main:
    0xaaaaaaaa0714 <+0>:  sub    sp, sp, #0x10
    0xaaaaaaaa0718 <+4>:  mov    w0, wzr
    0xaaaaaaaa071c <+8>:  str    wzr, [sp, #0xc]
    0xaaaaaaaa0720 <+12>: brk    #0xf000
->  0xaaaaaaaa0724 <+16>: brk    #0xf000
    0xaaaaaaaa0728 <+20>: add    sp, sp, #0x10
    0xaaaaaaaa072c <+24>: ret    
(lldb) c
Process 7804 resuming
Process 7804 stopped
* thread #1, name = 'test.o', stop reason = signal SIGTRAP
    frame #0: 0x0000aaaaaaaa0728 test.o`main at test.c:4:3
   1   	int main() {
   2   	  __builtin_debugtrap();
   3   	  __builtin_debugtrap();
-> 4   	  return 0;
   5   	}
(lldb) dis
test.o`main:
    0xaaaaaaaa0714 <+0>:  sub    sp, sp, #0x10
    0xaaaaaaaa0718 <+4>:  mov    w0, wzr
    0xaaaaaaaa071c <+8>:  str    wzr, [sp, #0xc]
    0xaaaaaaaa0720 <+12>: brk    #0xf000
    0xaaaaaaaa0724 <+16>: brk    #0xf000
->  0xaaaaaaaa0728 <+20>: add    sp, sp, #0x10
    0xaaaaaaaa072c <+24>: ret
```
Note how the first stop location is in fact the second trap instruction. And here, lldb-server isn't helping lldb skip them.

Just in case, I checked gdb:
```
(gdb) run
Starting program: /tmp/test.o 
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/aarch64-linux-gnu/libthread_db.so.1".

Program received signal SIGTRAP, Trace/breakpoint trap.
main () at /tmp/test.c:2
2	  __builtin_debugtrap();
(gdb) disassemble 
Dump of assembler code for function main:
   0x0000aaaaaaaa0714 <+0>:	sub	sp, sp, #0x10
   0x0000aaaaaaaa0718 <+4>:	mov	w0, wzr
   0x0000aaaaaaaa071c <+8>:	str	wzr, [sp, #12]
=> 0x0000aaaaaaaa0720 <+12>:	brk	#0xf000
   0x0000aaaaaaaa0724 <+16>:	brk	#0xf000
   0x0000aaaaaaaa0728 <+20>:	add	sp, sp, #0x10
   0x0000aaaaaaaa072c <+24>:	ret
End of assembler dump.
```
Which proves that the kernel is delivering the signal as you'd expect.

Maybe that helps, maybe it makes it way more complicated :)

https://github.com/llvm/llvm-project/pull/174348