Reenable JSONParserTest.Reading on Android |
||
Issue descriptionTest 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
,
Jun 11 2018
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.
,
Jun 11 2018
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.
,
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
,
Jun 12 2018
|
||
►
Sign in to add a comment |
||
Comment 1 by iclell...@chromium.org
, Jun 11 2018