[dart2js] --test-mode changes generated code and sourcemaps

[rakudrama]

Run these two compiles in the Dart sdk directory

out/ReleaseX64/dart-sdk/bin/dart compile js benchmarks/OmnibusDeferred/dart/OmnibusDeferred.dart --out=om1a/m.js
out/ReleaseX64/dart-sdk/bin/dart compile js benchmarks/OmnibusDeferred/dart/OmnibusDeferred.dart --test-mode --out=om1b/m.js

The commands differ in the --test-mode flag which does some serialization and deserialization. This should not change results, but does. I see the following changes:

• The main unit is changed because the part files have different hashes.

• Some (5) part files are changed in the order of the initialization of the holder alias variables. This is harmless by itself but we would prefer stability and consistent hashes.

• Many (19) sourcemap files are different. We would expect zero differences since the holder initialization difference does not lead to changes of the line or offset of code that can be mapped back to Dart source code.

Diffing the output of the pkg/compiler/test/sourcemaps/tools/sourcemap_visualizer.dart tool on om1a/m.js_1.part.js and om1b/m.js_1.part.js shows a difference in attribution of the same text:

native_version_javascript.dart:58:24
native_version_javascript.dart:57:22

-<span class="lineNumber">1321 </span><span class="mapped0continued" title="continued: ..&#47;benchmarks&#47;BigIntParsePrint&#47;dart&#47;native_version_javascript.dart:57:8 (_Methods._initialize)">      </span><span class="mapped1" title="..&#47;benchmarks&#47;BigIntParsePrint&#47;dart&#47;native_version_javascript.dart:58:24 (_Methods._initialize)">var e, exception;</span>
+<span class="lineNumber">1321 </span><span class="mapped0continued" title="continued: ..&#47;benchmarks&#47;BigIntParsePrint&#47;dart&#47;native_version_javascript.dart:57:8 (_Methods._initialize)">      </span><span class="mapped1" title="..&#47;benchmarks&#47;BigIntParsePrint&#47;dart&#47;native_version_javascript.dart:57:22 (_Methods._initialize)">var e, exception;</span>

Some of the differences seem to be for the JavaScript access (D.C_PrintEmitter) to the const PrintEmitter() parameter default value.
'a' attributes the constant to emitter, 'b' (--test-mode) attributes it to the constructor.

class BenchmarkBase {
  final String name;
  final ScoreEmitter emitter;

  const BenchmarkBase(this.name, {this.emitter = const PrintEmitter()});
        ^b                             ^a

Points to ponder:

• How does serialization/deserialization change the values? Are some values being written but not read? How could we pick this up earlier? Can we count the source information objects an assert the same number are written/read?
• We don't have a test that --test-mode, no --test-mode and sharded modular compilation all produce the same JavaScript and sourcemap results for a program of this (modest) size.

[biggs0125]

I thought (as you probably did @rakudrama) this was caused by my recent change to defer JS AST bodies. But if you sync back passed my change, the same issue can still be seen.

I also assumed that it was because of serialization of our JS AST for codegen. Turns out that isn't the case either, it's actually due to serializing and deserializing the Kernel in test mode. You can see this if you comment out those lines and pass along the original ir.Component without the serialization round trip.

So then I dug into the Kernel (de-)serialization logic and after a while found the node causing the diff was a FieldInitializer node. And sure enough when reading (and writing) that type of node the offset gets skipped.

I've created a fix here: https://dart-review.googlesource.com/c/sdk/+/325020 Just working on updating tests for this scenario.

[rakudrama]

I would not be surprised if we have more than one bug in more than one component.

@biggs0125 Nice find. I was going to start syncing back today. That explains the difference I labeled with ^a ^b. Does it also explain the difference between native_version_javascript.dart:58:24 and native_version_javascript.dart:57:22?

@johnniwinther, @jensjoha - do you think there are other bugs of this category? Could a Kernel round-trip test store the before-and-after sequence of offsets?

This is a screenshot of the sourcemap regions of some code compiled without any serialization:

With a full modular compile, the call sites (arrows) are in the same region (strictly: the region (beige) and the continuation of that region onto subsequent lines (cream)). This caused the second call to be incorrectly decoded to the first call.

There is a lot of coverage difference here, too much to be explained just by FieldInitializer.
I expect we will find multiple bugs that drop source information.

Next time I'll try harder to screenshot the exact same lines 😉.

[biggs0125]

Yeah I was going to start digging into that other example as well, I don't think FieldInitializer is the culprit there. I'm guessing its a similar thing but haven't downloaded the code yet to start digging into it.

[biggs0125]

This is now fixed by 655687d. The issue was due to a JS function bodies being deferred. Leaving this issue open until I've added a test to prevent this sort of regression in the future.