1478170 - Instantiate TokenStreamChars for UTF-8 source text

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Jeff Walden [:Waldo]]

TokenStreamCharsBase was previously finished, gotta keep moving through the hierarchy.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Make SourceUnits::findWindow{Start,End} const because they can/should be

This isn't really related to this bug's work, but it should happen and better before I specialize these functions for UTF-8, IMO.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Replace SpecializedTokenStreamCharsBase::consumeRestOfSingleLineComment with an infallible SourceUnits::consumeRestOfSingleLineComment

I had an epiphany: if we encounter an encoding error in this function, we can just stop consuming code points.  When we return from this and continue looking at code units, the next thing looking will report an error failing to decode a code point.  So *this* function doesn't have to!  A nice mini-simplification.

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Implement SourceUnits::peekCodePoint for UTF-8

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Specialize TokenStreamCharsBase::fillCharBufferWithTemplateStringContents for UTF-8

Comment 5

[Jeff Walden [:Waldo]]

Attachment: Implement getNonAsciiCodePointDontNormalize for UTF-8

This compiles, but roughly unsurprisingly as yet, it is untested.  🤠  The basic ideas of it are the interesting things, tho, IMO -- not mini-buglets that potentially exist in it (tho I do not believe any do).

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Specialize SourceUnits::consumeKnownCodePoint for UTF-8

Comment 7

[Jeff Walden [:Waldo]]

Attachment: Implement consumeRestOfSingleLine for UTF-8

Comment 8

[Jeff Walden [:Waldo]]

Attachment: Specialize SourceUnits::findWindowEnd for UTF-8

Comment 9

[Jeff Walden [:Waldo]]

Attachment: Specialize SourceUnits::findWindowStart for UTF-8

Comment 10

[Jeff Walden [:Waldo]]

Attachment: Stop specializing TokenStreamCharsBase::addLineOfContext now that the CharT-specialized things it relies on are fully specialized

Comment 11

[Jeff Walden [:Waldo]]

Attachment: Implement TokenStreamChars::getNonAsciiCodePoint for UTF-8

Comment 12

[Jeff Walden [:Waldo]]

Attachment: Move TokenStreamChars::getCodePoint to a generalized TokenStreamSpecific::getCodePoint that works for any CharT

Comment 13

[Jeff Walden [:Waldo]]

Attachment: Make atomizeSourceChars take a Span<CharT>, and define a specialization for UTF-8

This is pretty hack, but it is at least forward progress.

Comment 14

[Jeff Walden [:Waldo]]

Attachment: Instantiate TokenStreamChars for UTF-8

Comment 15

[Tooru Fujisawa [:arai]]

Comment on attachment 8994690 [details] [diff] [review]
Replace SpecializedTokenStreamCharsBase::consumeRestOfSingleLineComment with an infallible SourceUnits::consumeRestOfSingleLineComment

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

::: js/src/frontend/TokenStream.h
@@ +1293,5 @@
> +     * terminates it).
> +     *
> +     * If an encoding error is encountered -- possible only for UTF-8 because
> +     * JavaScript's conception of UTF-16 encompasses any sequence of 16-bit
> +     * code units -- code points up to it will be consumed.  (The caller is

can you clarify whether the part of erroneous code units are consumed or not?
("up to" seems to be ambiguous)

Comment 16

[Tooru Fujisawa [:arai]]

Comment on attachment 8994692 [details] [diff] [review]
Specialize TokenStreamCharsBase::fillCharBufferWithTemplateStringContents for UTF-8

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

the method name in the commit message doesn't match to the change.

Comment 17

[Tooru Fujisawa [:arai]]

Comment on attachment 8994693 [details] [diff] [review]
Implement getNonAsciiCodePointDontNormalize for UTF-8

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

::: js/src/frontend/TokenStream.cpp
@@ +661,5 @@
> +    char leadByteStr[5];
> +    byteToTerminatedString(leadValue, leadByteStr);
> +
> +    const char expectedStr[] = { toHexChar(required - 1), '\0' };
> +    const char actualStr[] = { toHexChar(remaining - 1), '\0' };

can you add some comment that those strings are expected to be decimal numbers,
and we reuse toHexChar because it's same for 0-4 range.

@@ +697,5 @@
> +    }
> +
> +    MOZ_ASSERT(codePointCharsArray + 2 <= codePointStr);
> +    *--codePointStr = 'x';
> +    *--codePointStr = '0';

can you add some helper function with descriptive name for above code?
or maybe add comment what it's doing.
the resulting content of codePointCharsArray and codePointStr are complicated
(like, the string starts from the middle of codePointCharsArray),
and a comment or descriptive function name will help figuring out what it's doing.

::: js/src/frontend/TokenStream.h
@@ +1633,5 @@
> +
> +        mozilla::Utf8Unit operator*() const {
> +            // This function is expected to be called only during an expression
> +            // of the form |*iter++| that *post-increments* this iterator, then
> +            // dereferences the old iterator.  Because operator++(int) will

can you clarify they share single SourceUnits instance?

@@ +1636,5 @@
> +            // of the form |*iter++| that *post-increments* this iterator, then
> +            // dereferences the old iterator.  Because operator++(int) will
> +            // consume the next code unit in |sourceUnits_|, dereferencing to
> +            // compute the just-consumed code unit must look at the *previous*
> +            // code unit here.

looks like a pitfall.
can we add some debug-only fields to detect mis-use?
like, storing the expected `current()` pointer or something when copying, and validating it when dereferencing?

also, it would be better preventing any other operations on the copied iterator, which modifies the shared SourceUnits.

::: js/src/util/Unicode.h
@@ +582,5 @@
>      return codePoint >= TrailSurrogateMin && codePoint <= TrailSurrogateMax;
>  }
>  
> +inline bool
> +IsSurrogate(uint32_t codePoint)

might be better adding some comment that explains why it receives `codePoint`.
like, this function is used for detecting invalid text, instead of used while parsing UTF16 surrogate pair or some similar usage.

Comment 18

[Tooru Fujisawa [:arai]]

Comment on attachment 8994710 [details] [diff] [review]
Specialize SourceUnits::findWindowStart for UTF-8

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

::: js/src/frontend/TokenStream.cpp
@@ +943,5 @@
> +
> +    while (true) {
> +        MOZ_ASSERT(earliestPossibleStart <= p);
> +        MOZ_ASSERT(HalfWindowSize() <= WindowRadius);
> +        if (p <= earliestPossibleStart || HalfWindowSize() >= WindowRadius)

according to the above assertion, `p < earliestPossibleStart` shouldn't happen.

@@ +955,5 @@
> +        // If there aren't three code units available, some comparison here
> +        // will fail before we'd underflow.
> +        if (MOZ_UNLIKELY((prev == 0xA8 || prev == 0xA9) &&
> +                         p[-2].toUint8() == 0x80 &&
> +                         p[-3].toUint8() == 0xE2))

can you add some comment that says they're LINE_SEPARATOR and PARA_SEPARATOR ?

Comment 19

[Tooru Fujisawa [:arai]]

Comment on attachment 8994713 [details] [diff] [review]
Implement TokenStreamChars::getNonAsciiCodePoint for UTF-8

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

::: js/src/frontend/TokenStream.cpp
@@ +901,5 @@
> +        this->notShortestForm(badCodePoint, unitsObserved);
> +    };
> +
> +    // This consumes the full, valid code point or ungets |lead| and calls the
> +    // appropriate error functor on failure.

where do we unget |lead| ?

Comment 20

[Tooru Fujisawa [:arai]]

Comment on attachment 8994719 [details] [diff] [review]
Make atomizeSourceChars take a Span<CharT>, and define a specialization for UTF-8

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

::: js/src/frontend/TokenStream.h
@@ +1450,5 @@
>      CharBuffer& getCharBuffer() { return charBuffer; }
>  };
>  
> +inline mozilla::Span<const char16_t>
> +ToCharSpan(mozilla::Span<const char16_t> codeUnits)

is this going to be used later?
if we're not going to use, it would be better removing this definition.

Comment 21

[Tooru Fujisawa [:arai]]

Comment on attachment 8994720 [details] [diff] [review]
Instantiate TokenStreamChars for UTF-8

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

\o/

Comment 22

[Jeff Walden [:Waldo]]

(In reply to Tooru Fujisawa [:arai] from comment #17)
> can you add some helper function with descriptive name for above code?
> or maybe add comment what it's doing.

Added a comment.  Separate helper function right now is tricky, because until this code is instantiated, a helper function would actually be unused -- and then you court compiler warnings.  (Granted at the end of this bug that *does* happen, but it's messy in advance of that, and I have the actually-instantiate patch at the end of the patch series for a reason.)

> can you clarify they share single SourceUnits instance?

Better yet, there's no point in even having the pointer in |SourceUnitsEnd| -- so I just made operator- return |sourceUnits_.remaining()| stored in |SourceUnitsIterator|.

> can we add some debug-only fields to detect mis-use?
> like, storing the expected `current()` pointer or something when copying,
> and validating it when dereferencing?
> 
> also, it would be better preventing any other operations on the copied
> iterator, which modifies the shared SourceUnits.

I added a |Maybe<const mozilla::Utf8Unit*>| field to the iterator, then assert it's |isNothing()| in all functions except operator* and operator++, and in those two assert the expected value in the first and then |reset()| it, in the second |emplace()| |current()| (which incidentally will assert |isNothing()| and verify it's clear).  I think that covers all the bases you mention.

> might be better adding some comment that explains why it receives
> `codePoint`.
> like, this function is used for detecting invalid text, instead of used
> while parsing UTF16 surrogate pair or some similar usage.

Added a comment, but note that most other functions in this file take |uint32_t| and not |char16_t| already, so it's not really breaking new ground.

(In reply to Tooru Fujisawa [:arai] from comment #18)
> > +        MOZ_ASSERT(earliestPossibleStart <= p);
> > +        MOZ_ASSERT(HalfWindowSize() <= WindowRadius);
> > +        if (p <= earliestPossibleStart || HalfWindowSize() >= WindowRadius)
> 
> according to the above assertion, `p < earliestPossibleStart` shouldn't
> happen.

True.  I wrote it that way for extra paranoia at runtime, of the sort that meant bug 1476409 was not actually dangerous -- we had a similar |>=| check there where, assuming proper incrementing, |==| would have been sufficient.

(In reply to Tooru Fujisawa [:arai] from comment #19)
> > +    // This consumes the full, valid code point or ungets |lead| and calls the
> > +    // appropriate error functor on failure.
> 
> where do we unget |lead| ?

See all the various |*aIter -= N| in |DecodeOneUtf8CodePointInline| in mfbt/Utf8.h, that end up invoking |SourceUnitsIterator::operator-=|, that invokes |sourceUnits.unskipCodeUnits(N)|.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/66952e88e68c
Make SourceUnits::findWindow{Start,End} const because they can/should be.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ccbc10cd6a1
Replace SpecializedTokenStreamCharsBase::consumeRestOfSingleLineComment with an infallible SourceUnits::consumeRestOfSingleLineComment, in light of the fact that the UTF-8 version can just stop consuming code points when an encoding error is encountered, leaving error-reporting to whatever actually tries to consume the first code point of the "next" line.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/292bd4af4056
Implement SourceUnits::peekCodePoint for UTF-8.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f427db8f6f8
Specialize TokenStreamCharsBase::fillCharBufferFromSourceNormalizingAsciiLineBreaks for UTF-8.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/45b34e5af77e
Implement getNonAsciiCodePointDontNormalize for UTF-8.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0847a6bfa7a
Specialize SourceUnits::consumeKnownCodePoint for UTF-8.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/46c400e4a018
Implement consumeRestOfSingleLine for UTF-8.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/f00dc821eb42
Specialize SourceUnits::findWindowEnd for UTF-8.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/26f77e15d7d2
Specialize SourceUnits::findWindowStart for UTF-8.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/cafc89ca6a87
Stop specializing TokenStreamCharsBase::addLineOfContext now that the CharT-specialized things it relies on are fully specialized.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/107211d728e6
Implement TokenStreamChars::getNonAsciiCodePoint for UTF-8.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/38216fdba3dd
Move TokenStreamChars::getCodePoint to a generalized TokenStreamSpecific::getCodePoint that works for any CharT.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/dad6f1d06271
Make atomizeSourceChars take a Span<CharT>, and define a specialization for UTF-8.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ad2890523ff
Instantiate TokenStreamChars for UTF-8.  r=arai

Comment 24

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/66952e88e68c
https://hg.mozilla.org/mozilla-central/rev/0ccbc10cd6a1
https://hg.mozilla.org/mozilla-central/rev/292bd4af4056
https://hg.mozilla.org/mozilla-central/rev/7f427db8f6f8
https://hg.mozilla.org/mozilla-central/rev/45b34e5af77e
https://hg.mozilla.org/mozilla-central/rev/d0847a6bfa7a
https://hg.mozilla.org/mozilla-central/rev/46c400e4a018
https://hg.mozilla.org/mozilla-central/rev/f00dc821eb42
https://hg.mozilla.org/mozilla-central/rev/26f77e15d7d2
https://hg.mozilla.org/mozilla-central/rev/cafc89ca6a87
https://hg.mozilla.org/mozilla-central/rev/107211d728e6
https://hg.mozilla.org/mozilla-central/rev/38216fdba3dd
https://hg.mozilla.org/mozilla-central/rev/dad6f1d06271
https://hg.mozilla.org/mozilla-central/rev/0ad2890523ff