1531479 - Use BytecodeLocation and BytecodeIterator in GetThisValueForDebuggerMaybeOptimizedOut

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P1)

Description

[Matthew Gaudet (he/him) [:mgaudet]]

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!

Comment 1

[Kc Udonsi]

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

Comment 2

[Shubhi Mishra]

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

Comment 3

[Matthew Gaudet (he/him) [:mgaudet]]

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.

Comment 4

[Kc Udonsi]

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

Comment 5

[Kc Udonsi]

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

Comment 6

[Matthew Gaudet (he/him) [: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

Comment 7

[Kc Udonsi]

Thank you!

Comment 8

[Kc Udonsi]

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

Comment 9

[Kc Udonsi]

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

Comment 10

[Kc Udonsi]

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

Comment 11

[Kc Udonsi]

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

Comment 12

[Kc Udonsi]

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

Comment 13

[Matthew Gaudet (he/him) [:mgaudet]]

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.

Comment 14

[Matthew Gaudet (he/him) [:mgaudet]]

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.

Comment 15

[Matthew Gaudet (he/him) [:mgaudet]]

The aforementioned bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1538301

Comment 16

[Kc Udonsi]

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

Comment 17

[Kc Udonsi]

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

Comment 18

[Matthew Gaudet (he/him) [:mgaudet]]

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.

Comment 19

[Kc Udonsi]

okay, awesome, thanks Matthew!

Comment 20

[Kc Udonsi]

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

Comment 21

[Kc Udonsi]

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.

Comment 22

[yozaam]

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

Comment 23

[André Bargull [:anba]]

The function has been renamed to GetThisValueForDebuggerEnvironmentIterMaybeOptimizedOut.

Comment 24

[yozaam]

Thanks

Comment 25

[Matthew Gaudet (he/him) [:mgaudet]]

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

Comment 26

[yozaam]

Attachment: Bug 1531479 Using BytecodeIterator in GetThisValueForDebuggerEnvironmentIterMaybeOptimizedOut r=mgaudet

Comment 27

[yozaam]

oops I didn't read your comment before this patch

Comment 28

[yozaam]

Attachment: Bug 1531479 Using BytecodeIterator in GetThisValueForDebuggerEnvironmentIterMaybeOptimizedOut r=mgaudet

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e48bcd3c4bb5
Using BytecodeIterator in GetThisValueForDebuggerEnvironmentIterMaybeOptimizedOut r=mgaudet

Comment 30

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/e48bcd3c4bb5

Comment 31

[Kc Udonsi]

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