New issue
Advanced search Search tips

Issue 851415 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2018
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug



Sign in to add a comment

Reenable JSONParserTest.Reading on Android

Project Member Reported by altimin@chromium.org, Jun 11 2018

Issue description

Test JSONParserTest.Reading
 failed and was disabled when blink_platform_unittests were reenabled on Android[1].

Assigning to platform/json/OWNERS for triage.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/1088913
 
Labels: Test-Disabled OS-Android
Confirmed on Pixel device.

This is failing on ARM64, the log messages show that rather than when parsing a number 2147483648 (2^31), it is not being promoted to a float, but is somehow creating an integer with value 2147483647. Not sure why it is being clamped to this value, but investigating.
Looks like a possible bug in Clang (or else this is allowed by some undefined behaviour of static_cast<>)

The problem is in the pair of lines:

    int number = static_cast<int>(value);
    if (number == value) { ... }

On ARM64, the compiler is generating this assembler to convert and compare:

    frintz  d0, d8                                 # Round d8 towards zero
    fcmp    d8, d0                                 # Compare result with d8
    b.ne    floating_point_case
    fcvtzs  w0, d8                                 # Convert d8 to 32-bit int
    add     x8, sp, #64
    bl      _ZN5blink14JSONBasicValue6CreateEi

which is doing a 64-bit rounding of the floating point value towards 0, and then comparing it to the original value. Since the value is integral to start with, the comparison succeeds, and so it *then* converts to a short int with fcvtzs, and calls JSONBasicValue::Create(int).

If we instead try to use the value before the comparison, it uses fcvtzs to convert to int, and then promotes that back to a float to compare against the original value:

    fcvtzs  w22, d8                                # Convert d8 to 32-bit int

    # Intervening code goes here

    scvtf   d0, w22                                # Convert back to float for comparison
    add     x8, sp, #64
    fcmp    d8, d0                                 # Compare against original
    b.ne    floating_point_case
    mov     w0, w22
    bl      _ZN5blink14JSONBasicValue6CreateEi

And this works correctly.
People have confirmed that that pattern is invoking the undefined behaviour demons -- the compiler is allowed to assume that the result of that static_cast fits within the bounds of an int; you can't use the result of that cast to check whether it was valid or not.

I'll switch that to a proper check.
Project Member

Comment 4 by bugdroid1@chromium.org, Jun 12 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7eea97e871e519038a325a54510524d872fde74e

commit 7eea97e871e519038a325a54510524d872fde74e
Author: Ian Clelland <iclelland@chromium.org>
Date: Tue Jun 12 04:39:08 2018

Fix double->int conversion in ParseJSON

The previous code was triggering undefined behaviour by casting a double
to an int without testing whether it actually fit within the limits of an int.
This was causing incorrect code to be emitted by recent versions of
Clang on ARM64.

Bug:  851415 
Change-Id: Ie40ded6d11377f05c92713165441324f33ac6db3
Reviewed-on: https://chromium-review.googlesource.com/1096028
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Ian Clelland <iclelland@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566314}
[modify] https://crrev.com/7eea97e871e519038a325a54510524d872fde74e/third_party/blink/renderer/platform/json/json_parser.cc

Status: Fixed (was: Assigned)

Sign in to add a comment