Closed Bug 965762 Opened 6 years ago Closed 6 years ago

ARM Simulator: support an environment variable and shell argument to drop into the debugger after a given number of instructions

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: dougc, Assigned: dougc)

References

Details

Attachments

(1 file, 1 obsolete file)

A convenient way to drop into the debugger helps debugging.  Breaking after a given number of instructions is also useful when debugging.

This patch adds the shell argument --arm-sim-stop-at and the environment variable ARM_SIM_STOP_AT.
Comment on attachment 8367883 [details] [diff] [review]
Support an environment variable and shell argument to drop into the debugger after a given number of instructions

Review of attachment 8367883 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/arm/Simulator-arm.cpp
@@ +4013,5 @@
>          }
> +    } else {
> +        while (program_counter != end_sim_pc) {
> +            SimInstruction *instr = reinterpret_cast<SimInstruction *>(program_counter);
> +            if (icount_ == Simulator::StopSimAt) {

This looks good but I think we could avoid the duplication without hurting performance in the common no-limit case if we templatized execute, something like this:

template <bool HasInstructionLimit>
void
Simulator::execute()
{
    ...
    if (HasInstructionLimit && icount_ == Simulator::StopSimAt) {
        ...
    }
}

This way the compiler generates two copies of execute and will optimize the extra check away if HasInstructionLimit is false. The caller can check Simulator::StopSimAt and call execute with either HasInstructionLimit true or false.

What do you think?
Attachment #8367883 - Flags: review?(jdemooij)
Rework to use a template.  Also account for instruction counts in the debugger.
Attachment #8367883 - Attachment is obsolete: true
Attachment #8368995 - Flags: review?(jdemooij)
Attachment #8368995 - Flags: review?(jdemooij) → review+
Note this patch assumes that the patch in bug 966570 has been applied.  Both are ready to checkin.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/52f8f3e943d6
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.