dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.04k stars 1.55k forks source link

[dart2wasm] Follow-up tasks on source maps #56232

Open mkustermann opened 1 month ago

mkustermann commented 1 month ago

Source map support has now landed in dart2wasm (thanks to @osa1 ) in 10742d9a12a194b07272640cb342572b8989508a (see flutter/flutter/pull/151643 for the flutter tools PR to take advantage of this).

Though we do have a set of follow-up tasks:

We should align the approach with dart2js here (@sigmundch). (See also existing dart2js issues with source maps in flutter https://github.com/flutter/flutter/issues/151641)

@yjbanov @eyebrowsoffire any preferences here what would be best for flutter? (the current dart2wasm approach of baking in absolute file uris works really well for local development but not if app is used on non-developer machines)

/cc @osa1

eyebrowsoffire commented 1 month ago

Ideally I think we'd have the source maps use relative URLs or something similar rather than file:// absolute paths. Source maps have two purposes in my eyes: (1) local development (for which absolute paths are perfectly fine) and (2) remapping stack traces of crashes/exceptions after the fact (for which absolute paths are not really ideal). I'm not exactly sure what the right scheme is here to achieve both of those goals.

osa1 commented 1 month ago

Currently method/function tear-offs have no mapping (we should map them to the function that's torn off I think)

I think this is OK, tear-off allocation code does not correspond to Dart code.

If we really want to map tear-off allocation to some code, it should be mapped to the allocation line rather than the torn-off member. E.g.

var x = "...";
var y = x.contains;

Here we generate

  (func $OneByteString.contains tear-off (;773;) (param $var0 (ref $OneByteString)) (result (ref $#Closure-0-2))
    i32.const 52
    i32.const 0
    local.get $var0
    global.get $global786
    global.get $global790
    struct.new $#Closure-0-2
  )

which shouldn't be mapped to String.contains in dart:core. We could map it to var y = x.contains, but I don't think that's possible, because the same function can be called in other tear-off expressions as well.

So I think it's best to leave this unmapped.


Another follow-up task could be mapping locations in StackTrace objects to Dart locations by using the generated source maps. Parsing the source maps can be done using the source_maps package.

sigmundch commented 1 month ago

I think what we do in dart2js is very applicable here for dart2wasm. I shared also some context in https://github.com/flutter/flutter/issues/151641#issuecomment-2226288655.

In general, we view source mapping as a 2 step process: producing the actual mapping (a compiler task) and integrating into a service that hosts the sources (a non-compiler task). Sometimes the same output is integrated in multiple ways (compiled output served locally and later served in production). We leverage custom schemes to make the compiler more independent, and delay integration work until it is needed.

Core libraries have currently no sources ...

Expose an option that flutter can pass to remap ...

We view this step of replacing the custom org-dartlang-sdk scheme as an integration issue, hence we don't do it within compiler. Instead, toolchains coordinate updates to the source-map as needed. Note that updating URLs is simple and doesn't require parsing the full source-map. In particular, the table of source URLs is available in plain JSON and can be updated without reading the VSLQ encoded map entries. Sometimes we can leverage the sourceRoot field to reduce duplication.

Have an option to not bake in file:///*/.dart urls in the mapping file

Related to https://github.com/flutter/flutter/issues/151641#issuecomment-2226288655, the dart2js approach here is to mimic what we do for the SDK: adopt a custom scheme to hide absolute file URLs, and later allow tools to expand it or replace it as needed for integration with a source hosting service.

In monorepos, a simple approach we've used is to pick a project root that holds all the code, and use a custom scheme to be the root for all of the code. We may need to revisit and propose a consistent approach for non-monorepos. I don't believe we have a standardized approach yet.

Make StackTrace.toString() fetch source maps at runtime & stringify stack. (Possibly only in --enable-assertions / profile mode?)

This is something that has made me uncomfortable in the past for two reasons:

Long term, I'd prefer that we don't conflate StackTrace.toString with debuggable stacktraces, but instead provide a new async API for that purpose (like StackTrace.debugString). This would require changing how flutter consumes stack traces and be explicit about where a debuggable form is expected.

(Possibly only in --enable-assertions / profile mode?)

Agree. In release mode, it is also important to keep sources private and not provide a channel that could expose them inadvertently.

mkustermann commented 2 weeks ago

In general, we view source mapping as a 2 step process: producing the actual mapping (a compiler task) and integrating into a service that hosts the sources (a non-compiler task).

I agree that it's very beneficial to have a two step mechanism and we should introduce that in dart2wasm as well (and align with dart2js)

Though I see a lot of value of having a mode where one can just serve the files with an arbitrary http server and things just work. For example, when I try out flutter web with wasm, I do this:

% flutter build web --wasm ...
% cd build/web
build/web % dhttpd --host=... '--headers=Cross-Origin-Embedder-Policy=credentialless;...'
...

and navigate a browser to that URL. I want stack maps to work in this case. And currently with dart2wasm's 1 step process it works very nicely (better than dart2js, see https://github.com/flutter/flutter/issues/151641)

So we could consider having two modes for the compiler:

@sigmundch Do we currently have a helper package that helps with rewriting of the source maps? Is it baked into e.g. package:webdev http serving code?