Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
1 of 10 tasks
mkustermann opened this issue Jul 12, 2024 · 4 comments
Open
1 of 10 tasks

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

mkustermann opened this issue Jul 12, 2024 · 4 comments
Labels
area-dart2wasm Issues for the dart2wasm compiler.

Comments

@mkustermann
Copy link
Member

mkustermann commented Jul 12, 2024

Source map support has now landed in dart2wasm (thanks to @osa1 ) in 10742d9 (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:

  • Currently method/function tear-offs have no mapping (we should map them to the function that's torn off I think)
  • Core libraries have currently no sources iff we compile from platform file (the org-dartlang-sdk://dart-sdk/lib/**/*.dart scheme we emit isn't resolvable by the browser. We have a few options here
    • Be fine with not having source maps for dart:* code
    • Put the sources for the dart:* into the source map file - downside: increases size of mapping file
    • Expose an option that flutter can pass to remap the org-dartlang-sdk://* scheme to a different uri scheme that flutter's http server can serve in e.g. flutter run
  • Have an option to not bake in file:///**/*.dart urls in the mapping file
    • Make it configurable on how package URIs are encoded in the mapping file - the user (or tool like flutter run) knows how the app is hosted and served and can therefore choose to serve package sources as under a specific url space
      => it just needs to tell dart2wasm that url space we should use for packages
    • Put the sources for the dart code into the source map file - downside: increases size of mapping file significantly
  • Make StackTrace.toString() fetch source maps at runtime & stringify stack. (Possibly only in --enable-assertions / profile mode?)
  • Pass source map file to wasm-opt: https://dart-review.googlesource.com/c/sdk/+/378421.

We should align the approach with dart2js here (@sigmundch). (See also existing dart2js issues with source maps in flutter flutter/flutter#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

@mkustermann mkustermann added the area-dart2wasm Issues for the dart2wasm compiler. label Jul 12, 2024
mkustermann added a commit to mkustermann/flutter that referenced this issue Jul 12, 2024
This will make

* `flutter run` have source maps enabled by default
* `flutter build` have source maps disabled by default

which mirrors what happens already today with the js compilers.

For local development this works quite well. We do have some follow-up
items for source maps in dart2wasm compiler, see [0]

[0] dart-lang/sdk#56232
@eyebrowsoffire
Copy link
Contributor

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.

mkustermann added a commit to flutter/flutter that referenced this issue Jul 13, 2024
This will make

* `flutter run` have source maps enabled by default
* `flutter build` have source maps disabled by default

which mirrors what happens already today with the js compilers.

For local development this works quite well - even better than with
dart2js (see dart2js issues in [0]).
We do have some follow-up items for source maps in dart2wasm compiler,
see [1]

[0]
[/issues/151641](#151641)
[1]
[dart-lang/sdk/issues/56232](dart-lang/sdk#56232)
@osa1
Copy link
Member

osa1 commented Jul 15, 2024

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
Copy link
Member

I think what we do in dart2js is very applicable here for dart2wasm. I shared also some context in flutter/flutter#151641 (comment).

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 flutter/flutter#151641 (comment), 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:

  • StratTrace.toString is synchronous. We support it in DDC by shipping the source-maps embedded in the JS code ahead of time and using a helper script to rewrite stacks.
  • Apps in production need access to the original JS stack trace. Apps log these stack traces and use it for offline deobfuscation using the source-map file, and they need to be certain that the API they call gives them what they expect. Today StackTrace.toString is inconsistent between debug vs production.

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 added a commit to mkustermann/flutter that referenced this issue Aug 12, 2024
This will make

* `flutter run` have source maps enabled by default
* `flutter build` have source maps disabled by default

which mirrors what happens already today with the js compilers.

For local development this works quite well - even better than with
dart2js (see dart2js issues in [0]).
We do have some follow-up items for source maps in dart2wasm compiler,
see [1]

[0]
[flutter/issues/151641](flutter#151641)
[1]
[dart-lang/sdk/issues/56232](dart-lang/sdk#56232)
mkustermann added a commit to mkustermann/flutter that referenced this issue Aug 12, 2024
This will make

* `flutter run` have source maps enabled by default
* `flutter build` have source maps disabled by default

which mirrors what happens already today with the js compilers.

For local development this works quite well - even better than with
dart2js (see dart2js issues in [0]).
We do have some follow-up items for source maps in dart2wasm compiler,
see [1]

[0]
[flutter/issues/151641](flutter#151641)
[1]
[dart-lang/sdk/issues/56232](dart-lang/sdk#56232)
@mkustermann
Copy link
Member Author

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 flutter/flutter#151641)

So we could consider having two modes for the compiler:

  • 1 step: embed absolute uris: allows serving locally with off-the-shelf http server, debugging works nicely
  • 2 step: embed uris with custom scheme, require custom http serving infrastructure or tools to re-write the source maps

@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?

auto-submit bot pushed a commit to flutter/flutter that referenced this issue Aug 13, 2024
…151643) (#153310)

Cherry-pick request: #153308

Original CL:

This will make

* `flutter run` have source maps enabled by default
* `flutter build` have source maps disabled by default

which mirrors what happens already today with the js compilers.

For local development this works quite well - even better than with dart2js (see dart2js issues in [0]).
We do have some follow-up items for source maps in dart2wasm compiler, see [1]

[0] [/issues/151641](#151641)
[1] [dart-lang/sdk/issues/56232](dart-lang/sdk#56232)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-dart2wasm Issues for the dart2wasm compiler.
Projects
None yet
Development

No branches or pull requests

4 participants