1531480 - Use BytecodeLocation and BytecodeIterator in RemoveReferencedNames

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

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

+++ This bug was initially created as a clone of Bug #1531479 +++

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* inside RemoveReferencedNames.

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

[Vaishnavi Kaulagi]

Can I try this one?

Comment 2

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

Hey Vaishnavi; you can definitely look at this (though, I think we have someone else also looking at this) or there's the very similar Bug 1538557 which I just opened for new contributors :)

Comment 3

[Kc Udonsi]

Hi Vaishnavi, I'm the 'someone else' lol. If you don't mind, let me take care of this :). Should have commented earlier, my bad

Comment 4

[Vaishnavi Kaulagi]

@kellykc72 No problem I'll work on the other one Matthew linked.

Comment 5

[Kc Udonsi]

OOo I'm back.

Comment 6

[Kc Udonsi]

UPDATE: I've been battling with this error for some time now. It just started happening. I used hg diff to confirm I didn't change anything unexpectedly. Working on resolving it. It happens when I run mozmake

lld-link: error: undefined symbol: "public: class js::BytecodeIterator __cdecl js::AllBytecodesIterable::begin(void)" (?begin@AllBytecodesIterable@js@@QEAA?AVBytecodeIterator@2@XZ)
>>> referenced by d:\firefox\js\src\vm\EnvironmentObject.cpp:3741
>>>               ..\Unified_cpp_js_src14.obj:("bool __cdecl RemoveReferencedNames(struct JSContext *, class JS::Handle<class JSScript *>, class mozilla::HashSet<class js::PropertyName *, struct mozilla::DefaultHasher<class js::PropertyName *>, class js::TempAllocPolicy> &)" (?RemoveReferencedNames@@YA_NPEAUJSContext@@V?$Handle@PEAVJSScript@@@JS@@AEAV?$HashSet@PEAVPropertyName@js@@U?$DefaultHasher@PEAVPropertyName@js@@@mozilla@@VTempAllocPolicy@2@@mozilla@@@Z))

Associated line(s) of code

  AllBytecodesIterable iter(script);
  for (BytecodeLocation loc : iter) {

So far, I have:

1. re-run ../configure ..
2. checked for unintentional changes with hg diff
3. just thinking about the different things that can cause this error

Comment 7

[Kc Udonsi]

js/src/build/mozjs-67a1.dll
c:/Users/SecurityDeveloper/.mozbuild/clang/bin/lld-link.exe -NOLOGO -DLL -OUT:mozjs-67a1.dll -PDB:mozjs-67a1.pdb -SUBSYSTEM:WINDOWS,6.01 -MACHINE:X64 @d:/firefox/js/src/build_DBG.OBJ/js/src/build/mozjs-67a1_dll.list   -guard:cf,nolongjmp -LARGEADDRESSAWARE -DEBUG    d:\firefox\js\src\build_DBG.OBJ\x86_64-pc-windows-msvc\debug\jsrust.lib ..\..\..\mozglue\build\mozglue.lib ..\..\..\config\external\nspr\pr\nspr4.lib ..\..\..\config\external\nspr\libc\plc4.lib ..\..\..\config\external\nspr\ds\plds4.lib   kernel32.lib user32.lib gdi32.lib winmm.lib wsock32.lib advapi32.lib shell32.lib userenv.lib ws2_32.lib

Comment 8

[Kc Udonsi]

I don't seem to have this problem when I run configure with --disable-optimize.

Comment 9

[Kc Udonsi]

Heyy, I know I always ask this but if there's a guide on how to know, I would appreciate it. What tests do I need to run for this? I have tried ../jit-test/jit_test.py --jitflags=all ./dist/bin/js debug which didn't seem to touch the code (verified using MOZ_CRASH). Now, I'm just running all tests

Comment 10

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

Regarding the symbol errors:

So, I don't have a windows development environment setup, but what I'd bet is the issue is the inlines files.

If you use Searchfox (and I would highly recommend you do: It's an excellent tool!) to look for AllBytecodesIterable::begin you'll see that the function is declared in BytecodeIterator.h, but defined in BytecodeIterator-inl.h

Some of our headers are split into -inl.h files. This is to allow us to define inline functions, where the definition must be seen, separate from the actual header itself, to allow cleaner include chains (and in some cases this is the only sensible construction).

So, in EnvironmentObject.cpp, you need to include both BytecodeIterator.h, and in the inlines section below, BytecodeIterator-inl.h.

Regarding Testing

I don't have a prebuilt list of things to test for this one. When you build with --enable-optimize --enable-debug hopefully it's not too onerous to run the whole test suite with a MOZ_CRASH to try and find some cases. (Sounds like that's the path you were on anyhow, but just encouraging that).

Comment 11

[Kc Udonsi]

Oh man, I didn't mean to ghost this. Testing my fix wasn't going smoothly and I fell out of it due to other things. But I will finish this. My sincerest apologies Matt.

Comment 12

[Kc Udonsi]

Matt! Totally unrelated, but I was searching for Debugging SpiderMonkey with GDB and I stumbled across your blog! It was on the second page of the search results haha. I look forward to checking it out. That being said, in an unrelated project (to this bug) I need to debug with GDB but I can't seem to get it working appropriately. Since it's unrelated, I don't want to clutter this bug with details but how could I get in touch with you or someone who would be able to help?

Comment 13

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

No worries!

Drop me an email; [e-mail].

Comment 14

[Kc Udonsi]

Attachment: Bug 1531480 - Replace uses of jsbytecode* with BytecodeLocation and BytecodeIterator inside RemoveReferenceNames

Encapsulate manipulation of bytecode within RemoveReferenceNames method using accessor classes BytecodeLocation and BytecodeIterator

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1d3e2f937623
Replace uses of jsbytecode* with BytecodeLocation and BytecodeIterator inside RemoveReferenceNames r=nbp

Comment 16

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/1d3e2f937623