Closed Bug 1483275 Opened 6 years ago Closed 6 years ago

Fix more SpiderMonkey unified build conflicts

Categories

(Core :: JavaScript Engine, enhancement, P5)

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: tcampbell, Assigned: tcampbell)

References

Details

Attachments

(3 files, 2 obsolete files)

Fix some more unified build problems in js/src. This still doesn't fix |FILES_PER_UNIFIED_FILE = 1000|, but it gets us closer.
Small tweaks to prevent things from colliding in unified builds.

- De-duplicate ReportExceptionClosure
- Rename IsIdentifier
- Other boring changes
Attachment #8999985 - Flags: review?(jwalden+bmo)
Move duplicated VMFunctions to jit/VMFunctions.cpp
Attachment #8999986 - Flags: review?(jdemooij)
Share SimpleArithOperand. This technically changes semantics, but it seems minor and the right call.
Attachment #8999987 - Flags: review?(jdemooij)
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 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.
Attachment #8999985 - Flags: review?(jwalden+bmo) → review+
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.
Attachment #8999985 - Attachment is obsolete: true
(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. :-)
Ugh, I didn't notice that was an import.  Don't bother with expanding BYTECODE except to move it, I guess.
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...
Attachment #9000066 - Attachment is obsolete: true
Attachment #9000076 - Flags: review+
Attachment #8999986 - Flags: review?(jdemooij) → review+
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/
Attachment #8999987 - Flags: review?(jdemooij) → review+
Pushed by tcampbell@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: