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

Kernel const evaluation must use platform integers #33285

Closed
leafpetersen opened this issue May 30, 2018 · 3 comments
Closed

Kernel const evaluation must use platform integers #33285

leafpetersen opened this issue May 30, 2018 · 3 comments
Assignees
Labels
area-front-end Use area-front-end for front end / CFE / kernel format related issues. front-end-kernel P1 A high priority bug; for example, a single project is unusable or has many test failures
Milestone

Comments

@leafpetersen
Copy link
Member

See #33282 for context.

The semantics of const object evaluation depends on the platform. On platforms that compile to JS, literals are interpreted as doubles, and integer operations are performed using double operations.

This issue is to verify that no const evaluation is done using the wrong semantics, since in the short term dart2js and ddc will be relying on receiving faithful representations of the original program literals in order to issue errors.

cc @rakudrama @jmesserly

@dgrove dgrove added this to the Dart2Stable milestone May 30, 2018
@dgrove dgrove added the P1 A high priority bug; for example, a single project is unusable or has many test failures label May 30, 2018
@kmillikin
Copy link

We don't do any const evaluation using the wrong semantics because we don't do any const evaluation. Eventually we should, and we're definitely aware that the different platforms have different number semantics.

@jmesserly
Copy link

jmesserly commented Jun 14, 2018

yeah, FWIW, dev_compiler overrides kernel's ConstantEvaluator class, in particular evaluateBinaryNumericOperation and canonicalize, so we can handle JS number semantics. It's not the cleanest approach but it's working alright. (we needed to override it anyway to handle "outline summaries". They make constant fields look like their values are null, which leads to erroneous results).

@mkustermann
Copy link
Member

@jmesserly @leafpetersen

The kernel2kernel constant evaluator in package:kernel was written by me. Currently we only use it in our AOT compilation in the VM (and hopefully in the JIT in the non too-distant future as well).

The front-end itself doesn't use it (that was what @kmillikin is referring to), so we run it as an extra transformation afterwards. It surfaces also compile-time errors which the front-end didn't catch (since it doesn't do constant evaluation atm afaik).

If you would want to restructure the package:kernel/transformations/constants.dart to make the interface nicer for the package:dev_compiler, you are welcome to send a CL to me!

@kmillikin kmillikin added area-front-end Use area-front-end for front end / CFE / kernel format related issues. front-end-kernel and removed area-front-end Use area-front-end for front end / CFE / kernel format related issues. labels Sep 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-front-end Use area-front-end for front end / CFE / kernel format related issues. front-end-kernel P1 A high priority bug; for example, a single project is unusable or has many test failures
Projects
None yet
Development

No branches or pull requests

5 participants