Closed Bug 1531479 Opened 6 years ago Closed 4 years ago

Use BytecodeLocation and BytecodeIterator in GetThisValueForDebuggerMaybeOptimizedOut

Categories

(Core :: JavaScript Engine, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
85 Branch
Tracking Status
firefox70 --- wontfix
firefox71 --- wontfix
firefox72 --- wontfix
firefox73 --- wontfix
firefox85 --- fixed

People

(Reporter: mgaudet, Assigned: kellykc72, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: good-first-bug, Whiteboard: [lang=c++])

Attachments

(1 file, 1 obsolete file)

Background

Historically access to bytecode in SpiderMonkey has been fairly freeform and common. This means that common idioms are repeated without encapsulation, and it's difficult to audit the codebase for patterns which makes changes around bytecode much more fragile than we'd like.

We'd like to encapsulate manipulation of bytecode within the engine to a set of accessor classes where possible.

This Bug

By using the BytecodeLocation and BytecodeIterator, we'd like to replace uses of jsbytecode* and pcOffset inside GetThisValueForDebuggerMaybeOptimizedOut.

This bug is only for changing the implementation of this function: It's OK that when values escape this function the return to jsbytecode* and pcOffsets. We can't change everything at once!

Note: It is expected that you may have to expand the interface of BytecodeLocation or BytecodeIterator. The interfaces as they exist now are driven by clients, and are comparatively thin.

Prior Art

  • In Bug 1499544, Part 2 used these interfaces to modify JSScript::assertValidJumpTargets. You can use that patch as inspiration for what we're looking for.

Prerequisites

Before getting started, you'll want to

  • You Have a checkout of the Firefox source code
  • Make sure you can build SpiderMonkey
  • Read this walkthrough about how development works in Firefox

Getting Help

Feel free to leave comments on this bug for questions, or, if you have more synchronous questions about this bug, feel free to drop into #jsapi on irc.mozilla.org.

Tips:

  • Not sure if the code you've been editing is getting run? Insert a call to MOZ_CRASH, a macro which will crash when executed, and run the entire test suite with an optimized build (for speed). If you see crashes, you can then use a debug build to make sure it's crashing in your code!
Blocks: 1531480

I'm interested in working on this. I would take a more in-depth look towards the weekend

I am interested in this. Can this be assigned to be?

Hi Shubhi; I think kellykc72 is looking at this; I'd recommend taking a look at Bug 1531480 -- it's similar.

We don't typically assign bugs until people are pretty far along with them at the start, but feel free to leave a comment that you're looking at it.

UPDATE: just started going through the linked "Prior Art", EnvironmentObject.cpp, BytecodeIterator.h, and BytecodeLocation.h.

How Matthew, please how do I know what tests to run? For this and in general

Flags: needinfo?(mgaudet)

So for this one, I'd run the whole JIT test suite. I use this command to do that, from my build directory ../jit-test/jit_test.py --jitflags=all ./dist/bin/js. You will find builds with --disable-optimize take quite a long time. On a build with --enable-optimize --enable-debug it shouldn't take longer than ~10-15 minutes or so (depending of course how fast your machine is). If even that's too slow you can try --disable-debug, however that disables some important assertions.

Note the tip at the end of the summary about MOZ_CRASH, which you can use to make sure you're exercising this code. Because this is debugger code, I suspect you might find you hit this code just running the debug tests directory: ../jit-test/jit_test.py --jitflags=all ./dist/bin/js debug

Flags: needinfo?(mgaudet)

Thank you!

TEST EXECUTION

../jit-test/jit_test.py --jitflags=all ./dist/bin/js debug

BEFORE FIX

FAILURES:
    debug\Frame-this-02.js
    --baseline-eager debug\Frame-this-02.js
    --ion-eager --ion-offthread-compile=off --ion-check-range-analysis --ion-extra-checks --no-sse3 --no-threads debug\Frame-this-02.js
    --ion-eager --ion-offthread-compile=off --more-compartments debug\Frame-this-02.js
    --no-baseline --no-ion --more-compartments debug\Frame-this-02.js
    debug\Frame-this-08.js
    --ion-eager --ion-offthread-compile=off --more-compartments debug\Frame-this-08.js
    --ion-eager --ion-offthread-compile=off --ion-check-range-analysis --ion-extra-checks --no-sse3 --no-threads debug\Frame-this-08.js
    --baseline-eager debug\Frame-this-08.js
    --no-baseline --no-ion --more-compartments debug\Frame-this-08.js
    debug\bug1300528.js
    --baseline-eager debug\bug1300528.js
    --ion-eager --ion-offthread-compile=off --more-compartments debug\bug1300528.js
    --no-baseline --no-ion --more-compartments debug\bug1300528.js

AFTER FIX

FAILURES:
    debug\Frame-this-02.js
    --ion-eager --ion-offthread-compile=off --ion-check-range-analysis --ion-extra-checks --no-sse3 --no-threads debug\Frame-this-02.js
    --ion-eager --ion-offthread-compile=off --more-compartments debug\Frame-this-02.js
    --baseline-eager debug\Frame-this-02.js
    --no-baseline --no-ion --more-compartments debug\Frame-this-02.js
    debug\Frame-this-08.js
    --ion-eager --ion-offthread-compile=off --more-compartments debug\Frame-this-08.js
    --ion-eager --ion-offthread-compile=off --ion-check-range-analysis --ion-extra-checks --no-sse3 --no-threads debug\Frame-this-08.js
    --no-baseline --no-ion --more-compartments debug\Frame-this-08.js
    --baseline-eager debug\Frame-this-08.js
    debug\bug1300528.js
    --ion-eager --ion-offthread-compile=off --more-compartments debug\bug1300528.js
    --baseline-eager debug\bug1300528.js
    --no-baseline --no-ion --more-compartments debug\bug1300528.js

All tests in both runs failed with this:

Assertion failure: isValid(), at d:/firefox/js/src\vm/BytecodeLocation.h:57
Exit code: -2147483645

Seems same failures to but this isn't satisfactory to me. I'm trying the MOZ_CRASH next but it wouldn't prove i haven't introduced new bugs. I will also try to read those tests and see.

Please feel free to share any advice or hints or suggestions

Flags: needinfo?(mgaudet)

hmm wait.. i may not have run mozmake lol. One sec

Flags: needinfo?(mgaudet)

Yup I forgot to run mozmakebefore taking the BEFORE reuslts. (I had changed the file and undone my change. Confirmed that the before state is clean

$ ../jit-test/jit_test.py --jitflags=all ./dist/bin/js debug
[4537|   0|   0|   0] 100% ==========================================>| 231.0ss
PASSED ALL

Sorry for the false alarm

But that's because the old code doesn't touch BytecodeLocation.h

So I have this line

          executedInitThisOp = BytecodeLocation(script, pc) > loc.next();

to replace

          executedInitThisOp = pc > GetNextPc(it);

The BytecodeLocation(script, pc) is what triggers the MOZ_ASSERT Failure

So first, as an aside, just let me say that I appreciate you coming back and working on this, and showing your work so I can help you. That's great.

Second, you've definitely stumbled on something interesting. One of the reasons we created the BytecodeLocation and BytecodeIterators was to catch situations just like this, where our invariants have been violated.

I've taken a quick look at this, reproduced the assertion you pointed out. It's possible you've identified a bug. I'll get back to you (hopefully before end of day) about how we want to proceed here.

So it seems that you have identified a bug with this. Which is great! It was a hoped for side-effect of this process, but it means we're going to be stalled here a bit while we wait to figure this out. This work is really valuable, even if your patch isn't able to make progress, because you've helped us highlight somewhere we've done something wrong.

I'm going to be opening a new bug that blocks this one to indicate there's something blocking this one from proceeding. If you'd like to follow along, you can hit the follow button on the bug (to be opened shortly) that blocks this one.

If you'd like, in the mean time you can start using the background you already have from this bug to look into Bug 1531480 (or, there are a large number of other places in the tree we'd like to start using BytecodeLocation and BytecodeIterator instead of jsbytecode* -- I can also point you at other places to try out and open new bugs for those.

Depends on: 1538301
No longer blocks: 1531480

Heyy, thanks for the words of encouragement!

Yes, I'll follow the new bug. and I'll check Bug 1531480 out!. I think you referred that other fellow to that bug, but I'll try to see fi they are already working on it. When this one is no longer stalled, I'll continue with it

Is that new bug something I can look into, or is it too advanced for me being a new contributor.

The thing with the new bug is that I think there will be some questions about the right way to proceed; once we have a clear picture of how to solve it, then it might be something to jump on. I saw you got CC'd on it, and I'll keep you in mind once we have a plan.

okay, awesome, thanks Matthew!

Hi Matthew! I'm still alive haha, just checking in on this. Has there been any updates? thanks!

Flags: needinfo?(mgaudet)

oops, nvm, I see the linked bug. No actions on it yet., but If I could be enlightened more on the situation I may be able to assist.

Flags: needinfo?(mgaudet)
Assignee: nobody → kellykc72
Status: NEW → ASSIGNED
Priority: P3 → P1

I don't find GetThisValueForDebuggerMaybeOptimizedOut in searchfox
Is this function removed from the codebase?

Flags: needinfo?(mgaudet)
Flags: needinfo?(mgaudet)

Thanks

Also; I suspect this is still blocked by Bug 1538301, as the core algorithm appears similar still.

oops I didn't read your comment before this patch

Attachment #9178734 - Attachment is obsolete: true
Pushed by mgaudet@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e48bcd3c4bb5 Using BytecodeIterator in GetThisValueForDebuggerEnvironmentIterMaybeOptimizedOut r=mgaudet
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 85 Branch

Whoops, extremely late to the party, thanks for taking care of this @yozaam

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: