1483275 - Fix more SpiderMonkey unified build conflicts

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P5)

Description

[Ted Campbell [:tcampbell]]

Fix some more unified build problems in js/src. This still doesn't fix |FILES_PER_UNIFIED_FILE = 1000|, but it gets us closer.

Comment 1

[Ted Campbell [:tcampbell]]

Attachment: 0001-Bug-1483275-Fix-some-SpiderMonkey-unified-build-conf.patch

Small tweaks to prevent things from colliding in unified builds.

- De-duplicate ReportExceptionClosure
- Rename IsIdentifier
- Other boring changes

Comment 2

[Ted Campbell [:tcampbell]]

Attachment: 0002-Bug-1483275-Remove-duplicate-VMFunction-definitions.patch

Move duplicated VMFunctions to jit/VMFunctions.cpp

Comment 3

[Ted Campbell [:tcampbell]]

Attachment: 0003-Bug-1483275-Use-one-definition-of-js-jit-SimpleArith.patch

Share SimpleArithOperand. This technically changes semantics, but it seems minor and the right call.

Comment 4

[Ted Campbell [:tcampbell]]

Some remaining problems:
- gc::AbortReason / jit::AbortReason conflict. Bug 1483269
- LIR defines ToInt32, IsConstant, etc which are all a pain..
- ???

This clears my patch queue on this matter and I'll let anyone else do more follow-up in another bug if they desire.

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 8999985 [details] [diff] [review]
0001-Bug-1483275-Fix-some-SpiderMonkey-unified-build-conf.patch

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

::: js/src/builtin/intl/IntlObject.cpp
@@ -30,5 @@
>  #include "vm/JSObject-inl.h"
>  
>  using namespace js;
>  
> -using mozilla::Range;

I'm not particularly happy about this removal.  It seems entirely fine/good to me for Range to be a thing using'd in individual files that are distinct, and I don't really like gunking up totally non-JIT files just for this.  But, uh, I guess.

(The same is also somewhat true for IsIdentifier -- but also there those are relatively fairly deemed "impl" functions, so the rename is justified in its own rights.)

::: js/src/irregexp/RegExpInterpreter.cpp
@@ +141,5 @@
>      }
>      for (size_t i = 0; i < (size_t) numRegisters; i++)
>          registers[i] = -1;
>  
> +#define BYTECODE(name)  case BC_##name:

Honestly, this seems dumb and I would just replace it with actually typing out the expansion.  IMO.

::: js/src/vm/ErrorReporting.h
@@ +69,5 @@
>      void throwError(JSContext* cx);
>  };
>  
> +class MOZ_STACK_CLASS ReportExceptionClosure
> +  : public ScriptEnvironmentPreparer::Closure

Save for the inheritance bit, IMO this really should just go in JSContext.h with an inline definition for operator().  But I guess that would require putting SEP into its own public header or something, and that requires figuring out what a *real* API for that thing is (docs in jsfriendapi.h are about as sparse as you would unfortunately expect), so for now okay.

(Also if REC can be final that would be a nice driveby improvement.)

@@ +78,5 @@
> +
> +    bool operator()(JSContext* cx) override;
> +
> +  private:
> +    JS::HandleValue exn_;

Wasn't style to put these at top?  (double-check me, I don't know I've internalized the rule well enough myself yet)

::: js/src/vm/RegExpObject.cpp
@@ +329,5 @@
>      return c == '\n' || c == '\r';
>  }
>  
>  static MOZ_ALWAYS_INLINE bool
> +IsRegExpLineTerminator(const char16_t c)

Perhaps in a followup patch these two functions should die and we should define them in util/Unicode.h instead, removing the presumably-conflicting frontend/ ones?  Poke r?me on that patch please.

Comment 6

[Ted Campbell [:tcampbell]]

Attachment: 0001-Bug-1483275-Fix-some-SpiderMonkey-unified-build-conf.patch

Carrying r=waldo from Comment 5.

- Leave |using mozilla::Range| alone. Probably doesn't seem to actually come up any more and change should have been on JIT side anyways.
- Expand BYTECODE macro
- Address ReportExceptionClosure nits.

Comment 7

[André Bargull [:anba]]

(In reply to Ted Campbell [:tcampbell] from comment #6)
> - Expand BYTECODE macro

Fair warning: Whenever I get back to bug 1367105, I'll 100% revert this change to reduce our local changes compared to upstream irregexp. :-)

Comment 8

[Jeff Walden [:Waldo]]

Ugh, I didn't notice that was an import.  Don't bother with expanding BYTECODE except to move it, I guess.

Comment 9

[Ted Campbell [:tcampbell]]

Attachment: 0001-Bug-1483275-Fix-some-SpiderMonkey-unified-build-conf.patch

Carrying r=waldo from Comment 3.

I should have seen that coming.. reducing delta on irregexp to just |#undef BYTECODE| which Andre may end up reverting on me anyways...

Comment 10

[Jan de Mooij [:jandem]]

Comment on attachment 8999987 [details] [diff] [review]
0003-Bug-1483275-Use-one-definition-of-js-jit-SimpleArith.patch

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

\o/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf5e7017ffdc
Fix some SpiderMonkey unified-build conflicts. r=waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/c13d9e177361
Remove duplicate VMFunction definitions. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5f5d094aa19
Use one definition of js::jit::SimpleArithOperand. r=jandem

Comment 12

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/cf5e7017ffdc
https://hg.mozilla.org/mozilla-central/rev/c13d9e177361
https://hg.mozilla.org/mozilla-central/rev/a5f5d094aa19