Use BytecodeLocation and BytecodeIterator in RemoveReferencedNames
Categories
(Core :: JavaScript Engine, enhancement, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox70 | --- | fixed |
People
(Reporter: mgaudet, Assigned: kellykc72, Mentored)
References
(Blocks 1 open bug)
Details
(Keywords: good-first-bug, Whiteboard: [lang=c++])
Attachments
(1 file)
+++ 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 pcOffset
s. 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!
Updated•5 years ago
|
Updated•5 years ago
|
Comment 1•5 years ago
|
||
Can I try this one?
Reporter | ||
Comment 2•5 years ago
|
||
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 :)
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•5 years ago
|
||
@kellykc72 No problem I'll work on the other one Matthew linked.
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:
- re-run
../configure ..
- checked for unintentional changes with
hg diff
- just thinking about the different things that can cause this error
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
I don't seem to have this problem when I run configure with --disable-optimize
.
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
Reporter | ||
Comment 10•5 years ago
|
||
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).
Assignee | ||
Comment 11•5 years ago
|
||
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.
Assignee | ||
Comment 12•5 years ago
|
||
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?
Reporter | ||
Comment 13•5 years ago
|
||
No worries!
Drop me an email; mgaudet@mozilla.com.
Assignee | ||
Comment 14•5 years ago
|
||
Encapsulate manipulation of bytecode within RemoveReferenceNames method using accessor classes BytecodeLocation and BytecodeIterator
Updated•5 years ago
|
Comment 15•5 years ago
|
||
Pushed by npierron@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1d3e2f937623 Replace uses of jsbytecode* with BytecodeLocation and BytecodeIterator inside RemoveReferenceNames r=nbp
Comment 16•5 years ago
|
||
bugherder |
Updated•5 years ago
|
Description
•