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

DDC does not build a working SDK with CFE constants flag enabled #36532

Closed
jmesserly opened this issue Apr 8, 2019 · 9 comments
Closed

DDC does not build a working SDK with CFE constants flag enabled #36532

jmesserly opened this issue Apr 8, 2019 · 9 comments
Assignees
Labels
P1 A high priority bug; for example, a single project is unusable or has many test failures web-dev-compiler
Milestone

Comments

@jmesserly
Copy link

Use the instructions and patches here to enable the constants flag: #36479

DDC's Kernel-backend compiled SDK will not load. I've found a few issues so far:

  • the SDK's internal annotations are not found. For example @nullCheck, @notNull, @JSExportName, @NoReifyGenerics, and probably other critical ones such as @JsPeerIterface.
  • static calls seem to be tagging the target with RTTI, as if it were a tearoff. As a result, we attempt to create a dart.FunctionType before it has been defined. This might partly be an issue with @ReifyFunctionTypes, but even so, we should not be reifying in these cases as it is unnecessary. We'll need to double check that this works properly for user code.

Also a few minor/trivial issues (not P1) such as:

  • integer constants are generated as if they were doubles: 1.0 instead of 1. This is valid, but has a very slight code size/readability cost.
@jmesserly jmesserly added P1 A high priority bug; for example, a single project is unusable or has many test failures web-dev-compiler labels Apr 8, 2019
@jmesserly jmesserly self-assigned this Apr 8, 2019
@jmesserly
Copy link
Author

I found a serious perf issue: #36535. My hope is that it isn't bad enough to require a fix in 2.3

@jmesserly
Copy link
Author

jmesserly commented Apr 9, 2019

I'm testing https://dart-review.googlesource.com/c/sdk/+/98875.
Update: there's a new testing CL, see my comment below

Once it passes, we can pull out the "pkg/dev_compiler" fixes into a CL and submit that separately. But for now I've merged it with the patches in #36479 so I can test how DDC is doing.

@sigmundch sigmundch added this to the Dart 2.3 milestone Apr 9, 2019
@sigmundch
Copy link
Member

@jmesserly - I just added the 2.3 milestone, I assume this is necessary before we ship, correct?

@jmesserly
Copy link
Author

ah, thank you. I meant to put it in 2.3, yup.

@jmesserly
Copy link
Author

I'm now testing a fix here: https://dart-review.googlesource.com/c/sdk/+/98927

Once DDC is green, I can pull out the DDC changes. It also includes a 1-liner CFE fix for #36545

@jmesserly
Copy link
Author

jmesserly commented Apr 11, 2019

I uploaded a few more workarounds:

  • there's some unusual CFE behavior where annotations end up as ConstantExpressions wrapping an UnevaluatedConstant. (this broke DDC's ability to pattern match certain annotations, such as @JS())
  • external factories appear to be eliminated currently, but are preserved with the constants flag. The compiler did not expect this and tried to emit a Procedure with a null function body.

I'm dry-running now. I think those will fix most/all of the remaining test failures.

@jmesserly
Copy link
Author

jmesserly commented Apr 11, 2019

Last night's test run found a few more issues. Based on local testing, I believe those are now fixed, so I made a CL that is DDC-only. https://dart-review.googlesource.com/c/sdk/+/99240. I'm also doing another test run on the above CL that contains the code to flip the flag.

@jmesserly
Copy link
Author

The CL passed on both with and without the constants flag globally enabled (other than 2 co19 tests that are failing on other platforms too w/ constants flag, so not a DDC issue). I've sent it out for review.

@sigmundch
Copy link
Member

@jmesserly - just saw that your CL landed, woohoo!

Is it safe to close this issue at this time?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 A high priority bug; for example, a single project is unusable or has many test failures web-dev-compiler
Projects
None yet
Development

No branches or pull requests

2 participants