Closed Bug 818816 Opened 7 years ago Closed 7 years ago

IonMonkey: (ARM) Add in conditional breakpoints


(Core :: JavaScript Engine, defect)

Not set





(Reporter: mjrosenb, Assigned: nrc)



(Whiteboard: [mentor=mjrosenb][lang=c++])


(2 files, 2 obsolete files)

Currently, masm.breakpoint() emits a bkpt instruction which is nice because it allows me to encode 16 bits of data in it (that I use to determine when thbreakpoint was generated)
However, it is frequently useful to have a breakpoint instruction that is conditional.
It turns out it is easy to do this using a load or store instruction:
ldr r12, [r12, -r12] will load from memory address r12 - r12 == 0 int r12.  Since load instructions aren't special, they can easily be made conditional using the standard methods of making instructions conditional.

Bonus points for re-implementing the current alignment checking code using this instruction

negative offsets to loads are supported somewhat, but the actual bits need to be propagated through a couple of layers of function calls.
Whiteboard: [mentor=mjrosenb][lang=c++]
Assignee: general → ncameron
Attached patch patch (obsolete) — Splinter Review
Attached patch alignment checking (obsolete) — Splinter Review
I'm not really sure how to test this (so its untested and so I haven't asked for review yet). I thought the alignment checking stuff would exercise the code, but pushing to Try is no joy because there is no debug tests for Android. I couldn't find any info on JS tests with some quick Googling.

So, should I write some sort of test for the conditional breakpoint, if so how? And/or can you point me towards some tests which exercise alignment checking so I can run them locally? Thanks!
Sadly, even if try were to run tests on debug builds, this wouldn't guarantee that they work, since we don't have any mis-alignments on tests in our testsuite.  I frequently use them to track down bailouts or other pool loads, so for example, putting one in:
MacroAssemblerARM::ma_b(void *target, Relocation::Kind reloc, Assembler::Condition c)
right before we branch.  This would probably be easiest to test out with a debugger, running a some code manually.
Attachment #690174 - Flags: review?(mrosenberg)
Attachment #690175 - Flags: review?(mrosenberg)
I got my testing environment sorted out and tested a bit. I played around with some JS in the shell and with a debug build of FF on Android and didn't get any crashes (well, no more than I did without the patches). And when I deliberately screwed up the stack alignment, I got a seg fault. But I can't really be sure that the seg fault came from my breakpoint stuff and not some horror due to the mis-aligned stack.

Is the desired result of hitting the breakpoint just a seg fault? I couldn't see how to copy the behaviour of the regular breakpoint.
(In reply to Marty Rosenberg [:mjrosenb] from comment #4)

> right before we branch.  This would probably be easiest to test out with a
> debugger, running a some code manually.

What do you mean manually? Do you mean a JS debugger, or GDB? And run it inside the ARM VM I guess?
the desired behavior is a segfault, and when I said debugger, I meant with gdb.  The ARM vm should be enough to verify that the "breakpoint" instruction is causing the segfault.
Comment on attachment 690174 [details] [diff] [review]

Review of attachment 690174 [details] [diff] [review]:

::: js/src/ion/arm/Assembler-arm.h
@@ +713,1 @@
>        : DtrOff(datastore::Reg(rn.code(), type, 0, shiftImm.encode()))

should probably also pass iu through here.
Attachment #690174 - Flags: review?(mrosenberg) → review+
Attachment #690175 - Flags: review?(mrosenberg) → review+
Attached patch patchSplinter Review
with reviewer's comment addressed. Carrying r=mjrosenb
Attachment #690174 - Attachment is obsolete: true
Attachment #693286 - Flags: review+
rebased, carrying r=mjrosenb
Attachment #690175 - Attachment is obsolete: true
Attachment #693288 - Flags: review+
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Depends on: 824463
You need to log in before you can comment on or make changes to this bug.