Closed Bug 815249 Opened 7 years ago Closed 7 years ago

IonMonkey: add masm.printf for debugging

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: h4writer, Assigned: h4writer)

Details

Attachments

(1 file, 1 obsolete file)

I find it handy to be able to print data during IM execution. Therefore I've added the function masm.printf that will call printf during executing. It is only for debugging, but I've also used it on --disable-debug builds. Therefore I put no #ifdef DEBUG around it.
Attached patch add printf (obsolete) — Splinter Review
Assignee: general → hv1989
Attachment #685261 - Flags: review?(jdemooij)
Attached patch add printfSplinter Review
Has a printf version that makes it possible to print information from registers
Attachment #685261 - Attachment is obsolete: true
Attachment #685261 - Flags: review?(jdemooij)
Attachment #685826 - Flags: review?(jdemooij)
Comment on attachment 685826 [details] [diff] [review]
add printf

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

Nice idea, seems useful. r=me with comments below addressed.

Can you make sure jit-tests pass if you add masm.printf(..) calls to some common LIR instructions?

::: js/src/ion/IonMacroAssembler.cpp
@@ +626,5 @@
>          handleException();
>      }
>  }
>  
> +void printf0_(char *test) {

Nit: "s/char */const char *" here and everywhere below.

@@ +638,5 @@
> +            ~Registers::CallMask);
> +    JS_ASSERT(!regs.empty());
> +    Register temp = regs.getAny();
> +    push(temp);
> +    push(ReturnReg);

We have to save all volatile registers since the printf call may overwrite them:

RegisterSet regs = RegisterSet::Volatile();
PushRegsInMask(regs);

Register temp = regs.getAny();

...CALL...

PopRegsInMask(regs);

@@ +659,5 @@
> +{
> +    GeneralRegisterSet regs(Registers::TempMask & ~Registers::JSCallMask &
> +            ~Registers::CallMask);
> +    if (regs.has(value))
> +        regs.take(value);

Same here. We have to make sure though that if we take registers from the set, the PushRegsInMask and PopRegsInMask still push/pop the same registers, so I'd just do something like:

RegisterSet regs = RegisterSet::Volatile();
PushRegsInMask(regs);

if (regs.has(value))
    regs.take(value);

Register temp = regs.getAny();

...CALL...

PopRegsInMask(RegisterSet::Volatile());

@@ +661,5 @@
> +            ~Registers::CallMask);
> +    if (regs.has(value))
> +        regs.take(value);
> +
> +    JS_ASSERT(!regs.empty());

Nit: don't need this assert, getAny also asserts !empty.
Attachment #685826 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/mozilla-central/rev/72c8d4dc1032
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.