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

assert: Check typed array view type in deepEqual #5910

Closed
wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

Pull Request check-list

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to CONTRIBUTING.md?
  • If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included?

Affected core subsystem(s)

assert

Description of change

Do not convert typed arrays to Buffer for deepEqual since their values may not be accurately represented by 8-bit ints. Instead perform binary comparison of underlying ArrayBuffers, but only when the array types match.

Never apply any kind of optimization for floating-point typed arrays since bit pattern equality is not the right kind of check for them.

Fixes: #5907 (introduced in #4330)

@jasnell
Copy link
Member

jasnell commented Mar 26, 2016

/cc @trevnorris @bnoordhuis

@jasnell jasnell added the assert Issues and PRs related to the assert subsystem. label Mar 26, 2016
@benjamingr
Copy link
Member

LGTM for both fixes.

@trevnorris
Copy link
Contributor

If the only concerning value is -0, this can be checked by

if (a === 0 && b === 0)
  if (a === 1/-0) return b === 1/-0;
  else b !== 1/-0;

Think that's it, but you get the idea.

@addaleax
Copy link
Member Author

@trevnorris Not 100 % sure what you’re saying here. Yes, that is how you check for -0 vs +0, but:

  • I don’t think assert.deepEqual(-0, +0) should throw (it currently does not), so I think neither should assert.deepEqual(new Float32Array([-0]), new Float32Array([+0])).
  • -0 and +0 are not the only values where bit-pattern comparison and === have different results (think of NaN – would that be a better example for the source code comment?).

Also, I’d assume checking each member of the typed arrays individually would defeat the whole purpose of speeding things up by comparing the underlying ArrayBuffers.

@addaleax
Copy link
Member Author

Don’t know whether you had a look at #5907, but maybe writing the problem with the current code down helps a bit:

Right now, when both actual and expected are typed arrays, they are cast to Buffer instances using Buffer.from(actual). This leads to false positives, though, because it is (potentially) a narrowing conversion of each value in the arrays to an unsigned 8-bit integer, i.e. 256 -> 0, NaN -> 0, -42 -> 214 and so on. As a result, new Uint32Array([256]) and new Float64Array([NaN]) (for example) are considered equal because they both get converted to <Buffer 00>, which I would deem obviously unintended behaviour.

So, this patch changes the following things:

  • Use actual.buffer instead of actual (and likewise for expected) so that the actual binary data of both arrays is inspected, rather than having each value cast to an uint8 (This also removes unnecessary copying of the arrays).
  • Bitwise comparison only applies when both typed arrays are the same kind of typed array (e.g. don’t compare Int8Array with Uint8ClampedArray).
  • Since comparing floating-point values bitwise leads to different results than comparing with ===, they should not be subject to this optimization. It was never the case that any of the equality checks in assert told +0 from -0 (and I fully agree with that behaviour), but looking at their binary representations would lead to that; so I added the checks that remove the floating-point typed arrays from this optimization.

Let me know if this cleared things up a bit, and if so, what parts of it you would like to see included in the commit message. 😄

@trevnorris
Copy link
Contributor

Ah. Misread your comment. Thought you wanted to.

As for the only thing affected by bit-pattern. ±0 is the only case I believe exists. NaN doesn't suffer from the same and is only ever a qNaN:

var dv1 = new DataView(new ArrayBuffer(8));
var dv2 = new DataView(new ArrayBuffer(8));
dv1.setFloat64(0, NaN);                  // bytes: 7f fc 0 0 0 0 0 0
// write sNaN
dv1.writeUint8(1, 0xfc);
dv2.writeFloat64(0, dv1.getFloat64(0));  // bytes: 7f fc 0 0 0 0 0 0

Now notice how the same doesn't hold true for 0:

dv1.setFloat64(0, -0);                 // bytes: 80 0 0 0 0 0 0 0
dv2.setFloat64(0, dv1.getFloat64(0));  // bytes: 80 0 0 0 0 0 0 0

We actually ran into this a few years back (see nodejs/node-v0.x-archive#4648) where clang32 was forcing sNaN and causing tests to fail (see https://llvm.org/bugs/show_bug.cgi?id=15054)

But since you're not checking the exact bit-wise pattern none of the above matters.

@addaleax
Copy link
Member Author

Ah, cool – didn’t know that. And yep, this gives just even more justification for not checking the bit pattern.

@benjamingr
Copy link
Member

@benjamingr
Copy link
Member

@addaleax hey, CI is green - I was going to land it but seeing you're up for addition as collaborator I figured I'd give you a chance to touch up your commit message yourself.

Currently, it's missing the PR-URL: field (linking to this PR) and the Reviewed-By field, basically, it stays the same except the bottom part changes from:

Fixes: https://github.com/nodejs/node/issues/5907

To

PR-URL: https://github.com/nodejs/node/pull/5910
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Fixes: https://github.com/nodejs/node/issues/5907

All the lines should be under 70 characters but that is already the case in this PR. Please let me know when you've amended the commit message or alternatively if you don't feel like it I don't mind landing this and doing it for you as we usually do for non-collaborators.

Do not convert typed arrays to `Buffer` for deepEqual since
their values may not be accurately represented by 8-bit ints.
Instead perform binary comparison of underlying `ArrayBuffer`s,
but only when the array types match.

Never apply any kind of optimization for floating-point typed
arrays since bit pattern equality is not the right kind of check
for them.

PR-URL: nodejs#5910
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Fixes: nodejs#5907
@addaleax
Copy link
Member Author

@benjamingr Thanks! I think I got this right. I rebased against master; if that’s something you don’t like around here after having run the CI, I can undo that, but I don’t think that would make a lot of sense?

@evanlucas
Copy link
Contributor

@benjamingr whoever lands the commit generally adds the metadata like PR-URL and Reviewed-By (or I've always thought so :]).

@benjamingr
Copy link
Member

@evanlucas yes, I wondered if addaleax would like the practice :)

I don't offer/ask this normally.

@evanlucas
Copy link
Contributor

@benjamingr ah apologies. I should have kept up with the conversation better :]

benjamingr pushed a commit that referenced this pull request Mar 31, 2016
Do not convert typed arrays to `Buffer` for deepEqual since
their values may not be accurately represented by 8-bit ints.
Instead perform binary comparison of underlying `ArrayBuffer`s,
but only when the array types match.

Never apply any kind of optimization for floating-point typed
arrays since bit pattern equality is not the right kind of check
for them.

PR-URL: #5910
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Fixes: #5907
@benjamingr
Copy link
Member

Landed in cf94929 - thanks for the surgical fix :)

@benjamingr benjamingr closed this Mar 31, 2016
@addaleax addaleax deleted the assert-typed-array-types branch March 31, 2016 19:11
@addaleax
Copy link
Member Author

addaleax commented Apr 2, 2016

If I understand correctly, this should probably be marked lts-watch-v4.x since the original PR which introduced this bug (#4330) was labelled the same way?

MylesBorins pushed a commit that referenced this pull request Apr 5, 2016
Do not convert typed arrays to `Buffer` for deepEqual since
their values may not be accurately represented by 8-bit ints.
Instead perform binary comparison of underlying `ArrayBuffer`s,
but only when the array types match.

Never apply any kind of optimization for floating-point typed
arrays since bit pattern equality is not the right kind of check
for them.

PR-URL: #5910
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Fixes: #5907
This was referenced Apr 5, 2016
@MylesBorins
Copy link
Contributor

@addaleax this is not landing cleanly as it looks like there have been some semver major changes to assert. Would you be able to manually back port this to v4.x-staging and send a PR?

addaleax added a commit to addaleax/node that referenced this pull request Apr 11, 2016
Do not convert typed arrays to `Buffer` for deepEqual since
their values may not be accurately represented by 8-bit ints.
Instead perform binary comparison of underlying `ArrayBuffer`s,
but only when the array types match.

Never apply any kind of optimization for floating-point typed
arrays since bit pattern equality is not the right kind of check
for them.

PR-URL: nodejs#5910
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Fixes: nodejs#5907
@addaleax
Copy link
Member Author

@thealphanerd Yup, here it is: #6147 :)

MylesBorins pushed a commit that referenced this pull request Apr 11, 2016
Do not convert typed arrays to `Buffer` for deepEqual since
their values may not be accurately represented by 8-bit ints.
Instead perform binary comparison of underlying `ArrayBuffer`s,
but only when the array types match.

Never apply any kind of optimization for floating-point typed
arrays since bit pattern equality is not the right kind of check
for them.

PR-URL: #5910
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Fixes: #5907
@MylesBorins MylesBorins mentioned this pull request Apr 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert Issues and PRs related to the assert subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

assert.deepEqual is broken?
7 participants