1451826 - Decouple Parser and BytecodeEmitter

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Eric Faust [:efaust]]

Right now, BytecodeEmitter takes an EitherParser and consults it on various matters. If we want to use BCE to emit BinAST, we'll want to decouple these, so that we can use the BinAST parser to create a tree, and BCE to emit it.

Comment 1

[Eric Faust [:efaust]]

Attachment: Part 1: Don't print bogus error locations on BCE internal errors.

Comment 2

[Eric Faust [:efaust]]

Attachment: Part 2: Don't call parser.reportError, which uses the current token position

Comment 3

[Eric Faust [:efaust]]

Attachment: Part 3: No longer call TokenStream::computeErrorMetadata from BCE.

Comment 4

[Eric Faust [:efaust]]

Attachment: Part 4: Make TokenStreamSpecific, rather than TokenStreamAnyChars, an ErrorReporter

Later in the stack, we'll want to add context-adding error reporting functions to ErrorReporter, as a convenient common interface.

Comment 5

[Eric Faust [:efaust]]

Attachment: Part 5: Move displayURL and sourceMap initialization from BCE to Parser.

Comment 6

[Eric Faust [:efaust]]

Attachment: Part 6: Factor SourceCoords references in BCE to ErrorReporter.

Not 100% on this one. On the one hand, ErrorReporter had a grab-bag of this stuff already, so it seemed natural, on the other, it has little to do, in most of these applications with reporting errors...

Comment 7

[Eric Faust [:efaust]]

Attachment: Part 7: Move the remaining error reporting bits in BCE to ErrorReporter

As promised.

Comment 8

[Eric Faust [:efaust]]

Attachment: Part 8: Insist that EitherParser always have a FullParseHandler.

Comment 9

[Eric Faust [:efaust]]

Attachment: Part 9: Introduce BCEParserHandle

This is intended to be the common interface for BCE interactions shared by BinASTParser in future and the parsers now.

In retrospect, maybe this should just live in BytecodeEmitter.h

Comment 10

[Eric Faust [:efaust]]

Comment on attachment 8966853 [details] [diff] [review]
Part 7: Move the remaining error reporting bits in BCE to ErrorReporter

r? Yoric for BinSource.h changes.

Comment 11

[Eric Faust [:efaust]]

Attachment: Part 10: Finally, convert BCE to using BCEParserHandle

Comment 12

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8966853 [details] [diff] [review]
Part 7: Move the remaining error reporting bits in BCE to ErrorReporter

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

Not sure what NYI method is supposed to do.

::: js/src/frontend/BinSource.cpp
@@ +2497,5 @@
> +
> +bool
> +BinASTParser::reportExtraWarningErrorNumberVA(UniquePtr<JSErrorNotes> notes, uint32_t offset, unsigned errorNumber, va_list* args)
> +{
> +    // NYI

Not Yet Initialized? Or probably Not Yet Implemented?

Shouldn't it rather MOZ_CRASH() or at least return false?

Comment 13

[Eric Faust [:efaust]]

Comment on attachment 8966852 [details] [diff] [review]
Part 6: Factor SourceCoords references in BCE to ErrorReporter.

Oops! Add Yoric for BinSource.h

Comment 14

[Eric Faust [:efaust]]

Attachment: Part 7 (2): Move the rest of the error reporting bits to ErrorReporter

Remove erroneous NYI; fill in completed function.

Comment 15

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8967099 [details] [diff] [review]
Part 7 (2): Move the rest of the error reporting bits to ErrorReporter

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

lgtm, thanks

Comment 16

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8966852 [details] [diff] [review]
Part 6: Factor SourceCoords references in BCE to ErrorReporter.

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

BinSource lgtm

Comment 17

[Jeff Walden [:Waldo]]

Comment on attachment 8966855 [details] [diff] [review]
Part 9: Introduce BCEParserHandle

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

This idea seems not unreasonable.

Comment 18

[Jeff Walden [:Waldo]]

Comment on attachment 8966846 [details] [diff] [review]
Part 1: Don't print bogus error locations on BCE internal errors.

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

So this just gives the error location as in the start of the function/script when no specific location was chosen?  Big improvement over now, tho maybe we could do better at some point.

Comment 19

[Jeff Walden [:Waldo]]

Comment on attachment 8966847 [details] [diff] [review]
Part 2: Don't call parser.reportError, which uses the current token position

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

Well, I guess *now* is when we start reporting better-honed locations.

Comment 20

[Jeff Walden [:Waldo]]

Comment on attachment 8966848 [details] [diff] [review]
Part 3: No longer call TokenStream::computeErrorMetadata from BCE.

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

::: js/src/frontend/TokenStream.h
@@ +1191,5 @@
>      void error(unsigned errorNumber, ...);
>  
>      // Report the given error at the given offset.
>      void errorAt(uint32_t offset, unsigned errorNumber, ...);
> +    void errorAtVA(uint32_t offset, unsigned errorNumber, va_list* args);

Bah, varargs.

Comment 21

[Jeff Walden [:Waldo]]

Comment on attachment 8966849 [details] [diff] [review]
Part 4: Make TokenStreamSpecific, rather than TokenStreamAnyChars, an ErrorReporter

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

::: js/src/frontend/TokenStream.cpp
@@ +449,5 @@
>      // Constrain starting columns to half of the range of a signed 32-bit value,
>      // to avoid overflow.
>      if (options().column >= mozilla::MaxValue<int32_t>::value / 2 + 1) {
> +        va_list empty;
> +        reportErrorNoOffsetVA(JSMSG_BAD_COLUMN_NUMBER, empty);

This is not a kosher use of va_list/varargs.  It is not actually possible to cons up a va_list containing a specific set of arguments, other than by calling a varargs function with the desired args.  You should add a helper member function that's varargs, then inside it define/start a va_list and pass that va_list to the VA function you've added here.  Joy.  (Just the way ErrorReporter::reportErrorNoOffset works now, of delegating to the virtual, VA-named function.)

(All of error reporting using varargs needs to be burned to the ground.  I will have the time to do this eventually.  I will.  I'm serious.  Why are you not believing me???)

Comment 22

[Jeff Walden [:Waldo]]

Comment on attachment 8966851 [details] [diff] [review]
Part 5: Move displayURL and sourceMap initialization from BCE to Parser.

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

I'm pretty sure I audited this right, but if I didn't the r- is subject to adjustment as necessary, as always.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +4944,5 @@
>  
>      if (!JSScript::fullyInitFromEmitter(cx, script, this))
>          return false;
>  
> +    tellDebuggerAboutCompiledScript(cx);

I'm not sure why you concluded that it's always the case that |emitterMode != LazyFunction && !parent|, such that the tell-call can just always happen here.

In particular frontend::CompileLazyFunction will hit here with |emitterMode == LazyFunction && !parent|, and that shouldn't be tell-debuggering.

Also BytecodeEmitter::emitFunction calls this and will have non-null |parent|, and that wasn't tell-debuggering before either.

It seems to me you should just get rid of the maybe-set bits and leave this condition and the remaining part of its body in place.  What am I missing?

::: js/src/frontend/Parser.cpp
@@ +1048,5 @@
>              return null();
>      }
>  
> +    if (!this->setSourceMapInfo())
> +        return null();

I don't think you need this -- or at least, this set-SMI would not previously have happened in any callers of this function.  If I track back BytecodeEmitter::emitScript, it has two callers.

One caller is BytecodeCompiler::compileModule which previously calls Parser::moduleBody which should itself be modified (see comment further down).

And the other caller is BytecodeCompiler::compileScript which created the relevant ParseNode using Parser::{eval,global}Body, both of which you've modified, so they don't need change.

@@ +2293,5 @@
>  }
>  
>  template <typename CharT>
>  ParseNode*
>  Parser<FullParseHandler, CharT>::moduleBody(ModuleSharedContext* modulesc)

I think you're missing an addition of a setSMI call to Parser::moduleBody.  Previously BytecodeCompiler::compileModule would have called BCE::emitScript which would have done SMI-setting, but with the removal of that from emitScript, it doesn't look like anything sets source-map info for modules that go through BC::compileModule.

Comment 23

[Jeff Walden [:Waldo]]

Comment on attachment 8966852 [details] [diff] [review]
Part 6: Factor SourceCoords references in BCE to ErrorReporter.

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

::: js/src/frontend/BinSource.h
@@ +231,5 @@
> +    virtual uint32_t lineAt(size_t offset) const override {
> +        return 0;
> +    }
> +    virtual uint32_t columnAt(size_t offset) const override {
> +        return offset;

Huh, so binsource goo is imagined has being all on a single line, with offsets that are (probably) byte offsets into the source?  Okay.

(Either that or this is mostly placeholder, which, okay if so.)

::: js/src/frontend/BytecodeEmitter.cpp
@@ +2184,5 @@
>                                   const EitherParser<FullParseHandler>& parser, SharedContext* sc,
>                                   HandleScript script, Handle<LazyScript*> lazyScript,
>                                   TokenPos bodyPosition, EmitterMode emitterMode)
>      : BytecodeEmitter(parent, parser, sc, script, lazyScript,
> +                      parser.errorReporter().lineAt(bodyPosition.begin),

Mm, now I see what you meant about awkward fits.  We should come up with a better name for this soonish, but I can run with this for the moment.

@@ +2527,5 @@
>  /* Updates line number notes, not column notes. */
>  bool
>  BytecodeEmitter::updateLineNumberNotes(uint32_t offset)
>  {
> +    ErrorReporter *er = &parser.errorReporter();

ErrorReporter* er

::: js/src/frontend/TokenStream.h
@@ +1197,5 @@
> +    }
> +    uint32_t lineAt(size_t offset) const final {
> +        return anyCharsAccess().srcCoords.lineNum(offset);
> +    }
> +    uint32_t columnAt(size_t offset) const final {

I'm somewhat inclined to have one |lineAndColumnAt| function that returns both of these, because if memory serves the effort to compute one gives you the other basically for free, but okay, I'll roll with this for the moment.

Comment 24

[Eric Faust [:efaust]]

Attachment: Part 5 (2): Move displayURL and sourceMap initialization from BCE to Parser.

Comment 25

[Eric Faust [:efaust]]

As discussed with Waldo IRL, the check that he was concerned about is already present in BytecodeEmitter::tellDebuggerAboutScript. The removed branch was totally redundant.

Comment 26

[Jeff Walden [:Waldo]]

Comment on attachment 8967099 [details] [diff] [review]
Part 7 (2): Move the rest of the error reporting bits to ErrorReporter

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

r- if this is easily fixed.  If it's not, please file a followup bug, and I think it needs to be a high-importance bug, because I think the muted-errors thing is a cross-origin security/info-hiding mechanism and this is a cross-origin info leak vector.  (Maybe verify that claim with bz if you can catch him, too.)

::: js/src/frontend/BinSource.cpp
@@ +2480,5 @@
>  {
>      ErrorMetadata metadata;
>      metadata.filename = getFilename();
>      metadata.lineNumber = 0;
>      metadata.columnNumber = offset();

You haven't initialized mutedErrors here, and that field is not optional (unlike the others you omitted).

Or, wait.  This is an *existing* line of code.  So I guess there's already a bug here.  And in the *new* code you add below, you have replicated this bug.  Joy.

Comment 27

[Jeff Walden [:Waldo]]

Comment on attachment 8966854 [details] [diff] [review]
Part 8: Insist that EitherParser always have a FullParseHandler.

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

Hum, why did this ever take a ParseHandler?

Comment 28

[Eric Faust [:efaust]]

Attachment: Part 7 (3): Move the rest of the error reporting bits to ErrorReporter

Address feedback about using mutedErrors properly.

Comment 29

[Jeff Walden [:Waldo]]

Comment on attachment 8966856 [details] [diff] [review]
Part 10: Finally, convert BCE to using BCEParserHandle

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +2183,5 @@
> +BytecodeEmitter::BytecodeEmitter(BytecodeEmitter* parent,
> +                                 BCEParserHandle* handle, SharedContext* sc,
> +                                 HandleScript script, Handle<LazyScript*> lazyScript,
> +                                 uint32_t lineNum, EmitterMode emitterMode)
> +    : BytecodeEmitter(parent, sc, script, lazyScript, lineNum, emitterMode)

If you can tack on a member-initializer

  : BCE(...),
    parser(handle)

to do this, I think that's preferable.  I'm not sure offhand whether C++ (and compilers, warning-free) allows this, tho.

Comment 30

[Jeff Walden [:Waldo]]

Comment on attachment 8971427 [details] [diff] [review]
Part 5 (2): Move displayURL and sourceMap initialization from BCE to Parser.

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

::: js/src/frontend/Parser.cpp
@@ +1004,5 @@
> +            }
> +        }
> +
> +        if (!ss->setSourceMapURL(context, options().sourceMapURL()))
> +            return false;

Mm.  Actually reading this, I sort of wish we only had one call to set the source-map URL, and we did preprocessing beforehand to determine what URL that was (and warn about overriding if necessary.  But this is what we did, fine to keep doing for now.

@@ +2283,5 @@
>          return nullptr;
>      globalsc->bindings = *bindings;
>  
> +    if (!this->setSourceMapInfo())
> +        return nullptr;

Can we do this at a consistent location within these functions?  Like, right after constant-folding in every case, or at the end of the function, or something.  It doesn't appear to matter where, just be consistent.  Maybe change this one, since the other hits are all after FoldConstants.

::: js/src/frontend/Parser.h
@@ +393,5 @@
>      bool leaveInnerFunction(ParseContext* outerpc);
>  
>      JSAtom* prefixAccessorName(PropertyType propType, HandleAtom propAtom);
> +
> +    bool setSourceMapInfo();

I guess for consistency with the prior thing put a MOZ_MUST_USE on this.

Comment 31

[Jeff Walden [:Waldo]]

Comment on attachment 8971448 [details] [diff] [review]
Part 7 (3): Move the rest of the error reporting bits to ErrorReporter

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

::: js/src/frontend/BinSource.cpp
@@ +399,5 @@
> +    ErrorMetadata metadata;
> +    metadata.filename = getFilename();
> +    metadata.lineNumber = 0;
> +    metadata.columnNumber = offset;
> +

Isn't this missing an isMuted set as well?  (A: Yes it is.)

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/51258270354b
Part 1: Don't print bogus error locations on BCE internal errors. (r=Waldo)
https://hg.mozilla.org/integration/mozilla-inbound/rev/b945db303abc
Part 2: Don't call parser.reportError from the BCE, as it uses the current token offset. (r=Waldo)
https://hg.mozilla.org/integration/mozilla-inbound/rev/1afcb9992642
Part 3: No longer call TokenStream::computeErrorMetadata from BCE. (r=Waldo)
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d712c91d06d
Part 4: Make TokenStreamSpecific and ErrorReporter, rather than TokenStreamAnyChars. (r=Waldo)
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe50254665a4
Part 5: Move displayURL and sourceMap intialization from BCE to Parser. (r=Waldo)
https://hg.mozilla.org/integration/mozilla-inbound/rev/df51f81b5ed4
Part 6: Factor SourceCoords references in BCE to ErrorReporter. (r=Waldo, r=Yoric)
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b20e7b26c0d
Part 7: Add the last error reporting bits to ErrorReporter from EitherParser. (r=Waldo, r=Yoric)
https://hg.mozilla.org/integration/mozilla-inbound/rev/598418d95c96
Part 8: Make EitherParser always a include a FullParseHandler. (r=Waldo)
https://hg.mozilla.org/integration/mozilla-inbound/rev/60bdf6086590
Part 9: Introduce BCEParserHandle. (r=Waldo)
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4ce5e759df5
Part 10: Convert BCE to using BCEParserHandle. (r=Waldo)

Comment 33

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/51258270354b
https://hg.mozilla.org/mozilla-central/rev/b945db303abc
https://hg.mozilla.org/mozilla-central/rev/1afcb9992642
https://hg.mozilla.org/mozilla-central/rev/6d712c91d06d
https://hg.mozilla.org/mozilla-central/rev/fe50254665a4
https://hg.mozilla.org/mozilla-central/rev/df51f81b5ed4
https://hg.mozilla.org/mozilla-central/rev/9b20e7b26c0d
https://hg.mozilla.org/mozilla-central/rev/598418d95c96
https://hg.mozilla.org/mozilla-central/rev/60bdf6086590
https://hg.mozilla.org/mozilla-central/rev/e4ce5e759df5