Skip to content

web runtimes should enforce int types on bit operations #45438

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

Open
rakudrama opened this issue Mar 23, 2021 · 2 comments
Open

web runtimes should enforce int types on bit operations #45438

rakudrama opened this issue Mar 23, 2021 · 2 comments
Labels
area-web-js Issues related to JavaScript support for Dart Web, including DDC, dart2js, and JS interop. P3 A lower priority bug or feature request web-dart2js web-dev-compiler

Comments

@rakudrama
Copy link
Member

The DDC and dart2js runtimes implement the bit operations like & and << on (JavaScript) Number rather than on Dart int.
They should be upgraded to enforce int receiver and argument types. The lack of enforcement is a hold-over from Dart 1, where the extra cost of int checks would be too expensive. The enforcement is now mostly static.

The lack of enforcement does not affect statically typed code since the int interface enforces the receiver and argument types at compile time.

Dynamic code fails to check both the receiver and argument for being int, allowing non-integer floating point numbers into the calculation (where they are truncated according to the JavaScript ToInt32 operation).

The main benefit of better enforcement of the int types would be more uniform behaviour in testing, e.g. #45419.

The main disadvantage is that there will be performance regressions on paths that do the extra checks, and these regressions would need to be fixed

  • dynamic calls
  • calls that are not lowered to more basic operations (more of an issue with shifts that bit-wise operations)

DDC would need to go first, since the extra checks should not be a surprise in production.

@rakudrama rakudrama added web-dart2js P3 A lower priority bug or feature request web-dev-compiler area-web-js Issues related to JavaScript support for Dart Web, including DDC, dart2js, and JS interop. labels Mar 23, 2021
@johnniwinther
Copy link
Member

To enforce this, the CFE constant evaluator needs to be augmented with knowledge of the "static type" of a constant value. Currently all integer and double literals are encoded as DoubleConstant values and these values are allowed as integers where the value is not infinite and truncates to itself (i.e. has no fractional part).

To enforce this new behavior we would need to retain integer as IntegerConstant values but evaluate them as their corresponding double values (notably taking the integer precision limit of the double encoding into account).

dart-bot pushed a commit that referenced this issue Mar 30, 2021
These tests show that we handle >> and >>> similarly wrt to allowing
integer operations on double constants with integer values.

#45438 proposes to disallow these,
but that requires a new js evaluation strategy and the issue in 45376
is therefore just a consequence of the current approach.

Change-Id: I703b57d5a6562ff1c1410858b22a96655f59dc4c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/193406
Reviewed-by: Dmitry Stefantsov <[email protected]>
Commit-Queue: Johnni Winther <[email protected]>
@rakudrama
Copy link
Member Author

@johnniwinther I don't think the CFE has to do anything. It already ready shows an error for this:

const dynamic a = 2.3;
const b = a << 1;

The issue is about preventing the current sketchy behaviour in the runtime of returning 4 (aka 4.0) for the above with non-const variables.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-web-js Issues related to JavaScript support for Dart Web, including DDC, dart2js, and JS interop. P3 A lower priority bug or feature request web-dart2js web-dev-compiler
Projects
None yet
Development

No branches or pull requests

2 participants