Odin: refactor AsmJSModule in preparation for wasm

RESOLVED FIXED in Firefox 46

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

unspecified
mozilla46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(7 attachments, 4 obsolete attachments)

211.72 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
4.27 KB, patch
Details | Diff | Splinter Review
520.94 KB, patch
Details | Diff | Splinter Review
58 bytes, text/x-review-board-request
jandem
: review+
Details
58 bytes, text/x-review-board-request
bbouvier
: review+
Details
58 bytes, text/x-review-board-request
bbouvier
: review+
Details
58 bytes, text/x-review-board-request
bbouvier
: review+
Details
Finishing what was started in bug 1224389 for AsmJSModule, such that it's possible to create a WasmModule w/o any JS source.
Posted patch hoist-stuff-to-wasm-h (obsolete) — Splinter Review
Simple renaming/motion/tidying patch.  The goal is that Wasm(|Module).h will contain everything that will be in the wasm::Module without pulling it in from 5 different places
 - CallSite* and HeapAccess stripped of AsmJS and moved to Wasm.h 
 - the whole AsmJSExit funny business changed into a plain class, renamed to wasm::ExitReason and moved to Wasm.h
 - AsmJSImmPtr/AsmJSAbsoluteAddress removed, replaced with the underlying enum AsmJSImmKind which is renamed to wasm::SymbolicAddress
Attachment #8694557 - Flags: review?(benj)
Better comments and fix compile error on osx.
Attachment #8694557 - Attachment is obsolete: true
Attachment #8694557 - Flags: review?(benj)
Attachment #8694563 - Flags: review?(benj)
Comment on attachment 8694563 [details] [diff] [review]
hoist-stuff-to-wasm-h

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

Thank you for merging the AsmJSAbsoluteAddress/ImmPtr/etc concepts into SymbolicAddress, the differences always have been a bit confusing and needed thinking and the SymbolicAddress name is much eloquent. Nice patch, a few nits / questions / suggestions below, but nothing that should block landing.

::: js/public/ProfilingFrameIterator.h
@@ +46,5 @@
>      // from it to use as the exit-frame pointer when the next caller jit
>      // activation (if any) comes around.
>      void* savedPrevJitTop_;
>  
> +    static const unsigned StorageSpace = 8 * sizeof(void*);

I am not sure to understand here: does the size of AsmJSProfilingFrameIter change because ExitReason now has two enum with undefined storage class (so they're ending up being unsigned ? and up to 2 bytes on 64 bits architecture)? If so, using enum classes could maybe allow to not change this StorageSpace?

::: js/src/asmjs/AsmJSModule.cpp
@@ +1450,5 @@
>  size_t
>  AsmJSModule::AbsoluteLinkArray::serializedSize() const
>  {
>      size_t size = 0;
> +    for (unsigned i = 0; i < unsigned(SymbolicAddress::Limit); i++)

(If you're using EnumeratedArray as suggested, I think there is also something like EnumeratedRange that can enumerate over all EnumeratedArray's values; here and in the a few functions below)

::: js/src/asmjs/AsmJSModule.h
@@ +634,5 @@
>      typedef Vector<uint32_t, 0, SystemAllocPolicy> OffsetVector;
>  
>      class AbsoluteLinkArray
>      {
> +        OffsetVector array_[unsigned(wasm::SymbolicAddress::Limit)];

Perhaps use a mozilla::EnumeratedArray here? The two asserts below (operator[]) come for free. Or AbsoluteLinkArray could also inherit from an EnumeratedArray, but [insert argumented discussion in favor of composition over inheritance here].

::: js/src/asmjs/Wasm.h
@@ +305,5 @@
> +{
> +    uint32_t targetIndex_;
> +
> +  public:
> +    explicit CallSiteAndTarget(CallSite cs, uint32_t targetIndex)

no strict need to be explicit, here

@@ +322,5 @@
> +// Summarizes a heap access made by wasm code that needs to be patched later
> +// and/or looked up by the wasm signal handlers. Different architectures need
> +// to know different things (x64: offset and length, ARM: where to patch in
> +// heap length, x86: where to patch in heap length and base) hence the massive
> +// #ifdefery.

Perhaps we could group the #ifdef by platform here? This struct always looked very weird to look at, and things are so different from one platform to the other that the shared parts could be at the top (insnOffsets and getter/setter + the empty ctor), and everything which is arch dependent could be grouped below, by platform. Plus, there doesn't seem to be any effort at packing the bits of the struct, so we wouldn't be losing anything doing that. What do you think?

@@ +410,5 @@
> +
> +// A wasm::Builtin represents a function implemented by the engine that is
> +// called directly from wasm code and should show up in the callstack.
> +
> +enum class Builtin

With the packing constraint in ExitReason, could we specify the storage class to be uint16_t?

@@ +454,5 @@
> +{
> +    ToInt32         = unsigned(Builtin::ToInt32),
> +#if defined(JS_CODEGEN_ARM)
> +    aeabi_idivmod   = unsigned(Builtin::aeabi_idivmod),
> +    aeabi_uidivmod  = unsigned(Builtin::aeabi_uidivmod)

nit: here and the lines below, there are a few commas missing at the end of line (arm won't build)

@@ +477,5 @@
> +    FloorF          = unsigned(Builtin::FloorF),
> +    ExpD            = unsigned(Builtin::ExpD),
> +    LogD            = unsigned(Builtin::LogD),
> +    PowD            = unsigned(Builtin::PowD),
> +    ATan2D          = unsigned(Builtin::ATan2D),

I'd be ok using a high-level macro FOR_EACH_WASM_BUILTIN, #undef after use, but copy/paste is fine too.

@@ +520,5 @@
> +class ExitReason
> +{
> +  public:
> +    // List of reasons for execution leaving compiled wasm code (or None, if
> +    // control hasn't exited ).

(micro-nit: space before closing parenthesis)

@@ +521,5 @@
> +{
> +  public:
> +    // List of reasons for execution leaving compiled wasm code (or None, if
> +    // control hasn't exited ).
> +    enum Kind

Could this be an enum class?

::: js/src/asmjs/WasmStubs.cpp
@@ +310,1 @@
>          MOZ_ALWAYS_TRUE(args.append(MIRType_Int32));

Wait, why are these append supposed to always succeed? I thought there was a args.reserve call somewhere above here in the past, but it seems it got removed. Should we add it back? Or just revert to fallible calls to args.append?

::: js/src/jit/arm64/MacroAssembler-arm64.h
@@ -3067,4 @@
>      }
>      void loadAsmJSHeapRegisterFromGlobalData() {
> -        loadPtr(Address(GlobalReg, AsmJSHeapGlobalDataOffset - AsmJSGlobalRegBias), HeapReg);
> -        loadPtr(Address(GlobalReg, AsmJSHeapGlobalDataOffset - AsmJSGlobalRegBias + 8), HeapLenReg);

pre-existing and totally out of scope, but: wow, this +8 deserves a static constant...

::: js/src/jit/mips32/MacroAssembler-mips32.cpp
@@ +968,3 @@
>  {
> +    movePtr(address, ScratchRegister);
> +    load32(Address(ScratchRegister, 0), imm);

nit: s/imm/dest

::: js/src/jit/mips32/MacroAssembler-mips32.h
@@ +1355,3 @@
>      }
>      void loadAsmJSHeapRegisterFromGlobalData() {
> +        MOZ_ASSERT(Imm16::IsInSignedRange(wasm::HeapGlobalDataOffset - AsmJSGlobalRegBias));

pre-existing: ditto, see also the mips64 (static_assert)

::: js/src/jit/mips64/MacroAssembler-mips64.h
@@ +1369,3 @@
>      }
>      void loadAsmJSHeapRegisterFromGlobalData() {
> +        MOZ_ASSERT(Imm16::IsInSignedRange(wasm::HeapGlobalDataOffset - AsmJSGlobalRegBias));

pre-existing, but could probably be a static_assert?

::: js/src/jit/shared/Assembler-shared.h
@@ +728,2 @@
>      Vector<AsmJSGlobalAccess, 0, SystemAllocPolicy> asmJSGlobalAccesses_;
>      Vector<AsmJSAbsoluteLink, 0, SystemAllocPolicy> asmJSAbsoluteLinks_;

Will asmJSGlobalAccesses_ and asmJSAbsoluteLinks_ be renamed / have their own typedef in a later patch? :-)

::: js/src/vm/Stack.h
@@ +1822,5 @@
>      // in this activation.
>      uint8_t* fp() const { return fp_; }
>  
>      // Returns the reason why asm.js code called out of asm.js code.
> +    wasm::ExitReason exitReason() const { return wasm::ExitReason::unpack(exitReason_); }

to make it clearer, maybe we could rename the attribute member packedExitReason_?
Attachment #8694563 - Flags: review?(benj) → review+
(In reply to Benjamin Bouvier [:bbouvier] from comment #3)
Wow, EnumeratedRange/Array are fantastic; all the casting really bugged me.  I even had an annoying bug while writing this patch when I used Builtin::Limit instead of SymbolicAddress::Limit that this would have totally caught.  The resulting code gets way cleaner. Great catch!

> I am not sure to understand here: does the size of AsmJSProfilingFrameIter
> change because ExitReason now has two enum with undefined storage class (so
> they're ending up being unsigned ? and up to 2 bytes on 64 bits
> architecture)? If so, using enum classes could maybe allow to not change
> this StorageSpace?

You're right, the static assert failed only on 32-bit going from 1 packed uint32 to two enum fields that each get represented as 32-bit ints.  Technically only 1 extra word is necessary but I felt like rounding up to an even number.  Also technically it's not a problem on 64-bit, but I didn't want to bother with #ifdef'ing since the size of this iterator doesn't matter at all; it's only stored on the stack and at most once.

> Perhaps we could group the #ifdef by platform here? This struct always
> looked very weird to look at, and things are so different from one platform
> to the other that the shared parts could be at the top (insnOffsets and
> getter/setter + the empty ctor), and everything which is arch dependent
> could be grouped below, by platform. Plus, there doesn't seem to be any
> effort at packing the bits of the struct, so we wouldn't be losing anything
> doing that. What do you think?

Good overall point.  I was still hoping to keep Wasm.h/WasmModule.h self-contained w/o pulling in the whole jit assembler header hierarchy, so how about "hoisting" the #ifdefs from inside the struct to around the structs (so there is 1 copy per x86/x64/arm+mips?

> With the packing constraint in ExitReason, could we specify the storage
> class to be uint16_t?

Yep, good idea.

> I'd be ok using a high-level macro FOR_EACH_WASM_BUILTIN, #undef after use,
> but copy/paste is fine too.

The macro would avoid repetition, but fancy macros are so terribly ugly that I'd prefer to only use them something more substantial.

> Could this be an enum class?

I would, but then we'd have to write 'ExitReason::Kind::None' which seemed overly verbose.  I wish C++ had made this scoping choice independent.

> ::: js/src/asmjs/WasmStubs.cpp
> @@ +310,1 @@
> >          MOZ_ALWAYS_TRUE(args.append(MIRType_Int32));
> 
> Wait, why are these append supposed to always succeed? I thought there was a
> args.reserve call somewhere above here in the past, but it seems it got
> removed. Should we add it back? Or just revert to fallible calls to
> args.append?

b/c of the inline size.  I'll add a static assert though.

> pre-existing and totally out of scope, but: wow, this +8 deserves a static
> constant...

Hah, I hadn't noticed that.  Agreed (in some later patch, though).

> pre-existing, but could probably be a static_assert?

Probably, but I'd be worried that constexpr support (on mips), so I'm inclined to leave it it as is.

> >      Vector<AsmJSGlobalAccess, 0, SystemAllocPolicy> asmJSGlobalAccesses_;
> >      Vector<AsmJSAbsoluteLink, 0, SystemAllocPolicy> asmJSAbsoluteLinks_;
> 
> Will asmJSGlobalAccesses_ and asmJSAbsoluteLinks_ be renamed / have their
> own typedef in a later patch? :-)

Yeah, so far I'm trying to confine myself to renaming things going into the files Wasm*.*.  After the dust has settled, though, yeah, I'd probably do a blanket conversion.  In general, I'd like to avoid "Wasm" and, if possible, put things into wasm::.  There are some unavoidable cases (like, say, JSFunction::isWasmNative) though.  But my reason for avoiding the mass s/AsmJS/Wasm/ is that I'd like to incrementally move stuff into wasm:: and do so carefully so that the header organization makes sense.

> to make it clearer, maybe we could rename the attribute member
> packedExitReason_?

I was on the edge, but you've convinced me.
Whiteboard: [leave open]
Depends on: 1230409
Posted patch mv-encode-latin1 (obsolete) — Splinter Review
This patch just moves EncodeLatin1 and gives it a UniqueChars return type so it can be easily reused in the following patch.
Attachment #8700049 - Flags: review?(jdemooij)
Posted patch wasm-module (obsolete) — Splinter Review
This is the last big refactoring patch.  I'm very sorry for big size, but it's mostly code motion.  A few notes that aren't just motion:
 - This patch renames more things to wasm, but it still leaves a bunch for a later trivial renaming / code-motion patch.
 - Scoped.h is deprecated so I moved everything to UniquePtr. UniquePtr is known to the rooting static analysis and so this actually allowed the rooting analysis to point out some preexisting GC hazards.
 - To avoid rooting hazards, the AsmJSModuleObjects are now created at the beginning so that they can root/trace the AsmJSModule/wasm::Module at all times.
 - Since the serialization/cloning code shares helpers between wasm/asm.js, I just split out a separate WasmSerialize.(h,cpp) which contains just that (for both asm.js and wasm).
 - Since StaticLinkData is only needed during static linking for wasm (and not in the future for link failure), it is stored in the AsmJSModule, not the wasm::Module.  For wasm, the StaticLinkData gets destroyed at the end of wasm compilation which saves memory for wasm code :)
 - The current builtin thunks were complicating things so I removed them.  This ended up removing a fair amount of complexity (e.g., wasm::Builtin can go away, leaving only wasm::SymbolicAddress).  Recall that the builtin thunks were only necessary for profiling to make certain builtins show up in the profile.  In wasm, there will be no hot math builtins (toolchains will need to compile their own trig functions, allowing them to choose the right perf/precision), so builtin thunks aren't necessary.  If we want to make builtins show up in profiles, we can use the slightly less-optimal strategy of storing an Activation::exitReason before calling out instead of the complicated thunk/patching scheme.
 - I realized with glee that the ProfiledFunctions were completely redundant wrt CodeRanges, so they were wholly removed.
 - "Exit" was renamed to "Import" (b/c that's what it is).  An Import is function name + signature.  "ImportExit" instead refers to the struct stored in global data that holds the JSFunction* and pointer to code and is kept entirely local to wasm::Module.
 - The code still cheats on the name/line/column stored in wasm::Module.  FuncDesc is added as an initial abstraction, but more work will be necessary to allow wasm to use bytecode offsets everywhere that get projected to text name/line/column on demand (e.g. CallSite).
 - I realized a while ago that it's kindof messy to have AsmJSModule support both the interface for building (during compilation) and using (after compilation).  So instead of simply moving over various building functions from AsmJSModule to wasm::Module, I put them in ModuleGenerator which builds up internal Vectors and then the wasm::Module is only constructed at the end (in ModuleGenerator::finish).  That means wasm::Module starts out fully formed and has a much simpler state machine.
 - Wasm.h was renamed to WasmTypes.h.  In a future patch I want Wasm.(h,cpp) to be the entry point (to the rest of the engien) for compiling and executing wasm.  Symmetrically, the three remaining AsmJS*.(h,cpp) would be merged into one AsmJS.(h,cpp) which is the entry point for all asm.js.

Looks like it's green on try.
Attachment #8700120 - Flags: review?(benj)
Posted patch wasm-module (obsolete) — Splinter Review
Now with stronger asserts about ICache flushing.
Attachment #8700120 - Attachment is obsolete: true
Attachment #8700120 - Flags: review?(benj)
Attachment #8700170 - Flags: review?(benj)
Comment on attachment 8700049 [details] [diff] [review]
mv-encode-latin1

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

::: js/src/vm/String.cpp
@@ +367,5 @@
> +        return nullptr;
> +
> +    JS::AutoCheckCannotGC nogc;
> +    if (linear->hasTwoByteChars())
> +        return UniqueChars(JS::LossyTwoByteCharsToNewLatin1CharsZ(cx, linear->twoByteRange(nogc)).c_str());

So the lossy TwoByte to Latin1 conversion is fine for the next patch?
Attachment #8700049 - Flags: review?(jdemooij) → review+
Actually, I hadn't thought carefully about that.  I wanted to avoid saving a bunch of PropertyNames in the module (there will be very many which makes GC more expensive *and* property names use 2-byte chars).  Ultimately, the chars are consumed by AtomizeChars() (to produce the JSAtom* returned from the FrameIter for the function name) or they are printed into the profiling function labels.  What's the best way to convert a PropertyName to chars for these purposes?
Learning more about this, it looks like I want to encode into utf8.  I tried a more ambitious patch to change all the CharacterEncoding.h functions to return UniquePtrs, but there is a fundamental mismatch: UniquePtr wants the PointerType to be a T* whereas CharacterEncoding uses RangePtr as the pointer type.
Attachment #8700049 - Attachment is obsolete: true
Attachment #8700751 - Flags: review?(jdemooij)
Posted patch wasm-moduleSplinter Review
Now using utf8 instead of latin1.  I also removed FuncDesc since, after thinking more about how we'll handle name/line info, I realized we probably want to compute this stuff eagerly (during decoding) so we don't need to keep the wasm binary around.
Attachment #8700170 - Attachment is obsolete: true
Attachment #8700170 - Flags: review?(benj)
Attachment #8700753 - Flags: review?(benj)
Attachment #8700751 - Flags: review?(jdemooij)
Attachment #8700753 - Flags: review?(benj)
Comment on attachment 8700820 [details]
MozReview Request: Bug 1229642 - Factor out StringToNewUTF8CharsZ (r?jandem)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28795/diff/1-2/
Comment on attachment 8700821 [details]
MozReview Request: Bug 1229642 - Split wasm::Module out of AsmJSModule (r?bbouvier)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28797/diff/1-2/
The last patch changes AsmJSActivation to store a wasm::Module instead of AsmJSModule (which is necessary for executing native wasm) by removing the dependency on FrameIter::scriptSource.  AsmJSActivation is then renamed to WasmActivation.
Comment on attachment 8700821 [details]
MozReview Request: Bug 1229642 - Split wasm::Module out of AsmJSModule (r?bbouvier)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28797/diff/2-3/
Comment on attachment 8700846 [details]
MozReview Request: Bug 1229642 - change to AsmJSActivation to WasmActivation (r?bbouvier)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28799/diff/1-2/
Ok, last patch wasn't quite the last.  The fourth one moves code out of AsmJSLink.cpp into WasmModule.cpp so that it can be reused by wasm calls.  With this, AsmJSModule.cpp and AsmJSLink.cpp are folded into AsmJSValidate.(h,cpp) which is renamed to AsmJS.(h,cpp).  This allows the external interface in AsmJS.h to be really simple; AsmJSModule is totally encapsulated.  I also decided I didn't like moving all the serialization code to WasmSerialize.cpp this code is moved back in the wasm-module patch and instead the shared serialization helpers are put into WasmSerialize.h.
Attachment #8700820 - Flags: review?(jdemooij) → review+
Comment on attachment 8700820 [details]
MozReview Request: Bug 1229642 - Factor out StringToNewUTF8CharsZ (r?jandem)

https://reviewboard.mozilla.org/r/28795/#review25723

Nice cleanup!
Blocks: 1234985
Attachment #8700821 - Flags: review?(benj) → review+
Comment on attachment 8700821 [details]
MozReview Request: Bug 1229642 - Split wasm::Module out of AsmJSModule (r?bbouvier)

https://reviewboard.mozilla.org/r/28797/#review25859

Very nice. I love how concepts are getting split, in a cleaner fashion. Although the number of concepts raises, they're simpler to understand as they seem smaller. Nice work.

Some files I've just skimmed at, as I assume they were code motion only (namely, WasmSerialize). The review is so big that I can't ensure that there are some hidden bugs I haven't caught. Well, the fuzzers will tell us :)

::: js/src/asmjs/WasmFrameIterator.h:54
(Diff revision 3)
> -    explicit AsmJSFrameIterator() : module_(nullptr) {}
> +    explicit FrameIterator() : fp_(nullptr) { MOZ_ASSERT(done()); }

Even though it might be useful only to the static analyzers, maybe worth it to initialize other members to nullptr as well? Or just PodZero it with the mozilla:: utilities?

::: js/src/asmjs/WasmFrameIterator.h:69
(Diff revision 3)
> +    Native         // implementation-dependent call to native C++ code

It might be nice for the reader to give some examples here? "(e.g. builtin calls, interrupt handling, etc.)"

::: js/src/asmjs/WasmFrameIterator.h:87
(Diff revision 3)
> -    AsmJSProfilingFrameIterator() : codeRange_(nullptr) {}
> +    ProfilingFrameIterator() : codeRange_(nullptr) {}

ditto.

Perhaps you can also MOZ_ASSERT(done()) in the ctor here?

::: js/src/asmjs/WasmFrameIterator.cpp:55
(Diff revision 3)
>      settle();

pre-existing, but it would be more concise to just write:

if (fp\_)
  settle();
  
Also, settle() will set codeRange\_ for sure, but maybe not callsite\_. Should we initialize it to nullptr, just to be safe?

::: js/src/asmjs/WasmFrameIterator.cpp:413
(Diff revision 3)
> -    // happens if profiling is enabled while module->active() (in this case,
> +    // happens if profiling is enabled while !module->active() (in this case,

Wasn't the original comment correct? setProfilingEnabled is only called at the top of CallAsmJS, so if there's an asmjs activation on the stack while the iterator goes through the iterations, we might not have called setProfilingEnabled yet...

::: js/src/asmjs/WasmFrameIterator.cpp:425
(Diff revision 3)
> -AssertMatchesCallSite(const AsmJSModule& module, const AsmJSModule::CodeRange* calleeCodeRange,
> +AssertMatchesCallSite(const Module& module, const CodeRange* calleeCodeRange,

calleeCodeRange is now unused

::: js/src/asmjs/WasmFrameIterator.cpp:511
(Diff revision 3)
> -    // happens if profiling is enabled while module->active() (in this case,
> +    // happens if profiling is enabled while !module->active() (in this case,

see comment above

::: js/src/asmjs/WasmFrameIterator.cpp:634
(Diff revision 3)
>      MOZ_ASSERT(callerPC_);

pre-existing, but this MOZ_ASSERT is tautological considering the above condition. Another assert below suggests that callerFP\_ can be nullptr, so it doesn't even look like a typo, and this assert might as well just be removed.

::: js/src/asmjs/WasmFrameIterator.cpp:668
(Diff revision 3)
>      //     devtools/client/profiler/cleopatra/js/parserWorker.js.

Not sure if it's done in the later series of patches, but don't forget to change the regexp in cleopatra, for this and for the builtins.

::: js/src/asmjs/WasmFrameIterator.cpp:702
(Diff revision 3)
> +wasm::EnableProfilingPrologue(Module& module, const CallSite& callSite, bool enabled)

Any chance these two functions could use a const Module? They seem to be just read here.

::: js/src/asmjs/WasmFrameIterator.cpp:779
(Diff revision 3)
> +    // 0x90. The offset is relative to the address of the instruction after

pre-existing: s/0x90/0xeb/

::: js/src/asmjs/WasmFrameIterator.cpp:823
(Diff revision 3)
> +        instr[0].makeNop();

pre-existing, but how about using a loop?

::: js/src/asmjs/AsmJSLink.cpp:108
(Diff revision 3)
> -ValidateGlobalVariable(JSContext* cx, const AsmJSModule& module, AsmJSModule::Global& global,
> +ValidateGlobalVariable(JSContext* cx, const AsmJSModule::Global& global, uint8_t* globalData,

Any chance it can be a const uint8_t\*, or too much hassle with the addition below?

::: js/src/asmjs/AsmJSLink.cpp:631
(Diff revision 3)
>      // The heap-changing function is a special-case and is implemented by C++.

I wouldn't have moved this comment, it's a bit further than what it now comments.

::: js/src/asmjs/AsmJSModule.h:81
(Diff revision 3)
> -// An asm.js module represents the collection of functions nested inside a
> +// An AsmJSModule extends (via containment) a wasm::Module with the extra persistent state

nit: comment longer than 80 chars

::: js/src/asmjs/AsmJSModule.h:294
(Diff revision 3)
> -            return pod.isChangeHeap_;
> +            return pod.wasmIndex_ == ChangeHeap;

Because of this sentinel value, we have to check that we have **strictly less** than UINT32_MAX functions when adding an export. Could be done in WasmGenerator::addExport, maybe?

::: js/src/asmjs/AsmJSModule.h:309
(Diff revision 3)
> -    struct Pod {
> +    UniqueWasmModule            wasm_;

This name isn't very talkative, especially considering that the getter has the same name, and it's weird to read asmJSModule\_.wasm().something().

Can we make it explicit and call it wasmModule\_? or commonModule\_? (a bit wordy) It might cause a lot of changes in other files, but it's really worth it, imo.

::: js/src/asmjs/AsmJSModule.h:315
(Diff revision 3)
> -        uint32_t                          maxHeapLength_;
> +        uint32_t                numFFIs_;

So I initially suggested to rename this numImports\_, then wondered why we couldn't just use imports\_.length(), then realized "imports" ~ (name, signature) while "ffi" ~ name and so imports \!== ffis, then read the comment above Import which explains that.

Could you add an inline comment like "// see comment above Import", here and/or right above numFFIs()?

::: js/src/asmjs/AsmJSModule.h:598
(Diff revision 3)
> -// there was a cache hit.
> +// successfully loaded and stored in moduleObj.extern bool

nit: rm |extern bool| at the end of this line

::: js/src/asmjs/AsmJSModule.cpp:107
(Diff revision 3)
> -    lineNumber_(0),
> +    pod.srcLength_ = endBeforeCurly - srcStart_;

pre-existing, but i realized there's no actual assertion that srcBodyStart\_ >= srcStart\_, neither here, or on the ctor, or on the ctor callee side. Can you add one, please?

::: js/src/asmjs/AsmJSModule.cpp:187
(Diff revision 3)
> -           SerializedPodVectorSize(elemOffsets_);
> +    return *(AsmJSModule*)getReservedSlot(MODULE_SLOT).toPrivate();

calling toPrivate() is going to check isPrivate(), which is != from isUndefined(), so I assume asserting hasModule() would be needless here?

::: js/src/asmjs/AsmJSModule.cpp:610
(Diff revision 3)
> +    module->staticallyLink(cx);

Wouldn't it be nice if staticallyLink were called if and only if loadedFromCache is set to true? (that is, three lines afterwards)

::: js/src/asmjs/WasmSignalHandlers.h:31
(Diff revision 3)
> +// Force any currently-executing asm.js code to call HandleExecutionInterrupt.

pre-existing, but a quick search on dxr, confirmed by the fact that you haven't put this under wasm::, shows that it's used in non-asmjs contexts. At least the comment should be updated, at best this should be moved out of the asmjs/ directory.

::: js/src/asmjs/WasmSignalHandlers.cpp:782
(Diff revision 3)
> -        if (pc == module.interruptExit() &&
> +        if (pc == module.interrupt() &&

pre-existing, but this could be rewritten using a single return and no if.

::: js/src/asmjs/AsmJSValidate.cpp:1223
(Diff revision 3)
> -        exits_(cx),
> +        imports_(cx),

Please initialize module\_ to nullptr

::: js/src/asmjs/AsmJSValidate.cpp
(Diff revision 3)
> -        // The type of a const is the exact type of the literal (since its value

I'd keep this comment and put it right above the initialization of global->u.varOrConst.type\_, as it nicely explains the bizarre conditional.

::: js/src/asmjs/AsmJSValidate.cpp:1486
(Diff revision 3)
> -        return global && globals_.putNew(name, global);
> +        return global && globals_.putNew(name, global) &&

It'd be nice to have one condition per line here.

::: js/src/asmjs/AsmJSValidate.cpp:1525
(Diff revision 3)
> +        return module().addExport(func.name(), maybeFieldName, wasmIndex,

The if statement above could also be folded in the return statement, here.

::: js/src/asmjs/AsmJSValidate.cpp:1584
(Diff revision 3)
>                   const LifoSig** lifoSig)

nit: alignment is wrong here

::: js/src/asmjs/AsmJSValidate.cpp:1598
(Diff revision 3)
> -        return exits_.add(p, ExitDescriptor(name, **lifoSig), *exitIndex);
> +        return imports_.add(p, ImportDescriptor(name, **lifoSig), *importIndex) &&

(the if can also be folded in the return statement)

::: js/src/asmjs/WasmTypes.h:41
(Diff revision 3)
> +using mozilla::MallocSizeOf;

I thought "using" statements in header files was prohibited?

::: js/src/asmjs/WasmTypes.h:570
(Diff revision 3)
> -    MOZ_IMPLICIT ExitReason(Kind kind) : kind_(kind) { MOZ_ASSERT(kind != Builtin); }
> +    bool operator==(CompileArgs rhs) const;

Why not use the default operator==? Is it so that if we add other members to the struct later, we don't forget to include them in the comparison operator?

::: js/src/asmjs/WasmTypes.h:580
(Diff revision 3)
> +static const uint32_t InitialGlobalDataBytes = NaN32GlobalDataOffset + sizeof(float);

For consistency, can it be an unsigned as the others, or all the others be uint32_t?

::: js/src/asmjs/WasmGenerator.h
(Diff revision 3)
> -namespace fronted { class TokenStream; }

Fun, this declaration was actually void, because of the typo in "fronted".

::: js/src/asmjs/WasmGenerator.h:80
(Diff revision 3)
> +    // Data scoped to the ModuleGenerator's lifetime

Nice comments\!

::: js/src/asmjs/WasmGenerator.h:149
(Diff revision 3)
> +    // non-null return value.

Why not make it return bool to indicate success and have a HandleModule outparam? This ensures that the return value be always rooted.

::: js/src/asmjs/WasmModule.h:223
(Diff revision 3)
> +    enum Kind { Function, Entry, ImportJitExit, ImportInterpExit, Interrupt, Inline };

enum class ftw

::: js/src/asmjs/WasmModule.h:225
(Diff revision 3)
> +    CodeRange() {}

Maybe use `= default` here?

::: js/src/asmjs/WasmModule.h:505
(Diff revision 3)
> +    Module* nextLinked() const;

This function should probably declared elsewhere, at least not in the `// wasm heap` subsection

::: js/src/asmjs/WasmModule.h:549
(Diff revision 3)
> +    // ProfilingFrameIterator when profilign is enabled.

nit: profiling

::: js/src/asmjs/WasmModule.h:562
(Diff revision 3)
> +    void addSizeOfMisc(MallocSizeOf mallocSizeOf, size_t* asmJSModuleCode,

nit: mozilla::MallocSizeOf (this probably works because an included header contains a `using mozilla::MallocSizeOf`, see also other comment :))

::: js/src/asmjs/WasmModule.cpp:518
(Diff revision 3)
> +# error "Missing architecture"

This will break non-ion builds!

::: js/src/asmjs/WasmModule.cpp:616
(Diff revision 3)
> +    CacheablePod pod = {0, 0, 0, false, false, false, false};

Wouldn't mozilla::PodZero work here?

::: js/src/asmjs/WasmModule.cpp:783
(Diff revision 3)
> +    auto target = CodeRange::PC((uint8_t*)pc - code());

Why not simply CodeRange::PC target(...);

::: js/src/asmjs/WasmStubs.cpp:102
(Diff revision 3)
> -GenerateEntry(MacroAssembler& masm, AsmJSModule& module, unsigned exportIndex,
> +GenerateEntry(ModuleGenerator& mg, unsigned exportIndex, Module::HeapBool usesHeap)

usesHeap is unused (ditto in GenerateAsyncInterruptStub)

::: js/src/asmjs/WasmStubs.cpp:105
(Diff revision 3)
> -    if (exp.isChangeHeap())
> +    const MallocSig& sig = mg.exportSig(exportIndex);

Just to confirm: this check isn't needed anymore because addChangeHeapExport doesn't call mg.addExport, right?

::: js/src/asmjs/WasmStubs.cpp:441
(Diff revision 3)
> -    CheckForHeapDetachment(masm, module, ABIArgGenerator::NonReturn_VolatileReg0, onDetached);
> +    if (usesHeap)

Could we load the asmjs heap register only if we use the heap, as we now have this piece of information?

::: js/src/jit/BaselineJIT.cpp:496
(Diff revision 3)
> -    // Remove any links from AsmJSModules that contain optimized FFI calls into
> +    // Remove any links from wasm:;Modules that contain optimized import calls into

nit: `wasm::`

::: js/src/vm/Runtime.h:26
(Diff revision 3)
> -#ifdef XP_DARWIN
> +#include "asmjs/WasmSignalHandlers.h"

The ifdef could be kept here.
Comment on attachment 8700846 [details]
MozReview Request: Bug 1229642 - change to AsmJSActivation to WasmActivation (r?bbouvier)

https://reviewboard.mozilla.org/r/28799/#review25905

Looks good to me, as long as the debugger or stack traces don't get too broken by this change, see comment below.

::: js/src/asmjs/WasmTypes.cpp:124
(Diff revision 2)
> -    Module& module = activation->module().wasm();
> +    Module& module = activation->module();

In this function and a few others below, module is used only once, this use could as well be inlined now that the def is short.

::: js/src/builtin/TestingFunctions.cpp:1455
(Diff revision 2)
>                  frameKindStr = "asmjs";

WasmFrame is going to be shared for asm.js and webassembly. Should we change the frameKindStr to "asmjs/webassembly" (or just "webassembly" or "wasm") now, so that we don't have to think about doing it later?

::: js/src/jsapi.h:5214
(Diff revision 2)
> -class MOZ_STACK_CLASS JS_PUBLIC_API(AutoFilename)
> +class JS_PUBLIC_API(AutoFilename)

Another possibility: just make AutoFilename an alias for UniqueChars. It won't break outside compatibility and that would help getting rid of some code.

::: js/src/jsapi.h
(Diff revision 2)
> -    void operator=(const AutoFilename&) = delete;

Why not keeping the two deleted ctor and operator=?

::: js/src/vm/Stack.h:1387
(Diff revision 2)
> -    AsmJSActivation* asAsmJS() const {
> +    WasmActivation* asWasm() const {

(almost a palindrome)

::: js/src/vm/Stack.cpp:981
(Diff revision 2)
> -    ScriptSource* ss = scriptSource();
> +    if (!hasScript())

If I understand correctly this change, and the code that calls it (JS::CaptureCurrentStack), this means that everytime we have a callback / stack trace / promise stack trace referencing an asm.js function, the displayed name of a function will be just the filename if it's running under Odin/Baldr, but it would be the entire URL otherwise?

You told me last week that retrieving the source consisted in doing a network request to fetch the original source code. If that code fetching the source were using the displayURL, or any Debugger code would be using the displayURL for networky stuff, this would be broken, which sounds bad. Can you please check you can still download the source of an asm.js module by clicking on its name in the Debugger? (even the warning successful compilation message wouldn't be tied anymore to an URL?)

If the behavior isn't changed for stack traces, debugger, etc., I'm fine with this change. But if it doesn't, maybe some Debugger people could chime in?
Attachment #8700846 - Flags: review?(benj) → review+
Comment on attachment 8701356 [details]
MozReview Request: Bug 1229642 - Factor AsmJSLink.cpp into wasm/asm.js and consolidate AsmJS* into AsmJS.cpp (r?bbouvier)

https://reviewboard.mozilla.org/r/28953/#review25911

Assuming this is mostly code motion, I just quickly skimmed at the code and just commented on the new parts. Looks nice. AsmJS.h is indeed very tied up!

::: js/src/asmjs/AsmJS.h:122
(Diff revision 1)
> +// To support the use of signal handlers for catching Out Of Bounds accesses,

This comment would probably be more useful next to the definition (the comments in the Architecture-* files would need to be updated in this case).

::: js/src/asmjs/AsmJS.cpp:695
(Diff revision 1)
> +NewAsmJSModuleObject(ExclusiveContext* cx)

As for NewAsmJSModuleFunction, it is static so it could be renamed into NewModuleObject; or you didn't want to make it confusing with respect to AsmJSModule vs wasmModule?

::: js/src/asmjs/AsmJS.cpp:8213
(Diff revision 1)
> +    // The LinkAsmJS builtin (created by NewAsmJSModuleFunction) is an extended

nit: NewAsmJSModuleFunction => NewModuleFunction

::: js/src/asmjs/WasmModule.h:551
(Diff revision 1)
> -    // can be toggled. WebAssembly frames only show up in the
> +    // SPSProfiler::enabled() on the next Modue::call when there are no frames

nit: Modue => Module
Attachment #8701356 - Flags: review?(benj) → review+
Thanks as always for the careful and expedient review!  Agreed on all points with these comments:

> Any chance it can be a const uint8_t\*, or too much hassle with the addition
> below?

ValidateGlobalVariable actually writes into globalData so it needs to be mutable.

> Because of this sentinel value, we have to check that we have **strictly
> less** than UINT32_MAX functions when adding an export. Could be done in
> WasmGenerator::addExport, maybe?

I went with adding a check to ModuleValidator::addExport so it's an asm.js-specific detail.

> Can we make it explicit and call it wasmModule\_?

wasmModule wfm (note: I'll fix this in the final patch, once AsmJSModule is in AsmJS.cpp).

> Wouldn't it be nice if staticallyLink were called if and only if
> loadedFromCache is set to true? (that is, three lines afterwards)

It's also called in the non-cache-hit case after StoreAsmJSModuleInCache in CompileAsmJS.

> > +// Force any currently-executing asm.js code to call HandleExecutionInterrupt.
> 
> pre-existing, but a quick search on dxr, confirmed by the fact that you
> haven't put this under wasm::, shows that it's used in non-asmjs contexts.
> At least the comment should be updated, at best this should be moved out of
> the asmjs/ directory.

Agreed; this is actually on my todo-cleanup to pull out the Ion stuff so WasmSignalHandlers.(cpp,h) only implement the wasm stuff.  The inclusion goes back to the original implementation of Ion's signal-based interrupt.  Ion used to share a fault handler with asm.js so they were tightly coupled.  Now they just share the mechanism for halting a thread (pthread_kill/SuspendThread) so it should be easier to hoist into vm/.

> > +using mozilla::MallocSizeOf;
> 
> I thought "using" statements in header files was prohibited?

We do it in a few restricted cases (see NamespaceImports.h) for things that we always want to have available (and avoid qualifying in all headers).  In this case, the using is inside namespace 'wasm' and for names that should never conflict anyway, so it seemed reasonable to me.

> Why not use the default operator==?

Because there is no default operator== :)

> Why not make it return bool to indicate success and have a HandleModule
> outparam? This ensures that the return value be always rooted.

Module is not a GC thing, only the AsmJSModuleObject (and in bug 1234985, WasmModuleObject).  MG::finish doesn't know what will happen to the returned wasm::Module (maybe it will be owned by an AsmJSModule, maybe a WasmModuleObject).

> > +    enum Kind { Function, Entry, ImportJitExit, ImportInterpExit, Interrupt, Inline };
> 
> enum class ftw

I would've, but it messes up the scoping so you have to write 'CodeRange::Kind::Function' which gets kinda long.

> > +    CacheablePod pod = {0, 0, 0, false, false, false, false};
> 
> Wouldn't mozilla::PodZero work here?

The trick is avoiding the "uninitialized const member" error.  I would've written a member-initializer-list (where pod{} initializes to zero) but MSVC2013 doesn't support these.

> Just to confirm: this check isn't needed anymore because addChangeHeapExport
> doesn't call mg.addExport, right?

Correct: the "change heap" function is a purely asm.js construct and thus confined to AsmJSModule (until it is removed).

> >                  frameKindStr = "asmjs";
> 
> WasmFrame is going to be shared for asm.js and webassembly. Should we change
> the frameKindStr to "asmjs/webassembly" (or just "webassembly" or "wasm")
> now, so that we don't have to think about doing it later?

These changes end up going outside js/src and tests outside, so I was hoping to put this off to a later patch which did more mass-renaming from asm.js to wasm.

> If I understand correctly this change, and the code that calls it
> (JS::CaptureCurrentStack), this means that everytime we have a callback /
> stack trace / promise stack trace referencing an asm.js function, the
> displayed name of a function will be just the filename if it's running under
> Odin/Baldr, but it would be the entire URL otherwise?

Technically, it's just the sourceURL from source notes but you're right, we should continue to handle this properly.  I'll just make a copy into wasm::Module like is done currently with filename by making CacheableChars into a template over char type.

(In reply to Benjamin Bouvier [:bbouvier] from comment #29)
> As for NewAsmJSModuleFunction, it is static so it could be renamed into
> NewModuleObject; or you didn't want to make it confusing with respect to
> AsmJSModule vs wasmModule?

That's what I started with but, with unified builds, they end up in the same translation unit.
Whiteboard: [leave open]
(In reply to Luke Wagner [:luke] from comment #30)
> > Why not use the default operator==?
> 
> Because there is no default operator== :)

Wow, my bad. I guess my false assumption was that if C++ could provide a default operator= (and therefore automatically choose to copy by value/pointer), it could provide a default operator== (and choose to compare by value/pointer), but it doesn't. Reading a bit more about this, it seems that the default operator= was provided for backward compatibility with C, although Stroustrup recommends not using the default one at all.
(Personally, I wish C++next would provide default impls for most operators that weren't generated by default (overloaded use of "default" here... and "overloaded" ;) but could be requested via "= default" (or perhaps some new "= explicit").)
Depends on: 1235874
Depends on: 1235989
Depends on: 1236541
Depends on: 1236552
Depends on: 1267696
No longer depends on: 1267696
You need to log in before you can comment on or make changes to this bug.