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

test: fix tests on mips #20377

Closed
wants to merge 5 commits into from
Closed

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Apr 27, 2018

Mips has a different way of handling NaN. This makes sure the tests
pass on MIPS as well.

This partially fix-es #20336 (the - is there to prevent github to close the issue).

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

Mips has a different way of handling NaN. This makes sure the tests
pass on MIPS as well.
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Apr 27, 2018
@BridgeAR
Copy link
Member Author

@@ -70,6 +70,10 @@ buffer.writeDoubleLE(NaN, 8);
assert.ok(buffer.equals(new Uint8Array([
0x7F, 0xF8, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xF8, 0x7F
// Mips processors use a different NaN.
])) || buffer.equals(new Uint8Array([
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we detect running on mips and only assert the alternative value if so?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

The performance jit logger is not implemented on mips.
@BridgeAR BridgeAR changed the title test: fix buffer writes on mips test: fix tests on mips Apr 28, 2018
@BridgeAR
Copy link
Member Author

Comment addressd. I also added another commit to fix the flag issue.

CI https://ci.nodejs.org/job/node-test-pull-request/14558/

0x7F, 0xF7, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF,
0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xF7, 0x7F
])));
if (process.arch === 'mips') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the docs for process.arch :

The current possible values are: 'arm', 'arm64', 'ia32', 'mips', 'mipsel', 'ppc', 'ppc64', 's390', 's390x', 'x32', and 'x64'.

so we probably need to be checking for mipsel as well. Maybe mips64el too. Perhaps @kapouer or anyone else with access to mips can check?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, actually, it seems like we either have to use the first approach or we can just check one byte.

https://sourceware.org/binutils/docs/as/MIPS-NaN-Encodings.html

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we go back to the first approach, maybe add a comment Allow both quiet and signalling NaNs?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not built nodejs 10 on mipsel yet, but nodejs 8.11 gives the same result as mips on mipsel.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I check one byte now and the check will then verify if the whole entry is correct and added a comment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So now there is no need to be specific about any architecture anymore.

@BridgeAR
Copy link
Member Author

BridgeAR commented Apr 28, 2018

assert.ok(
buffer.equals(new Uint8Array(
[ 0x7F, 0xC0, 0x00, 0x00, 0x00, 0x00, 0xC0, 0x7F ])));
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

buffer[1] === 0xF7 isn't true for either assert?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was late ^^. Fixed.

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 28, 2018
BridgeAR added a commit to BridgeAR/node that referenced this pull request May 7, 2018
Mips has a different way of handling NaN. This makes sure the tests
pass on MIPS as well.

PR-URL: nodejs#20377
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
BridgeAR added a commit to BridgeAR/node that referenced this pull request May 7, 2018
The performance jit logger is not implemented on mips.

PR-URL: nodejs#20377
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@BridgeAR
Copy link
Member Author

BridgeAR commented May 7, 2018

Landed in df2d73d and 8444bba

@BridgeAR BridgeAR closed this May 7, 2018
MylesBorins pushed a commit that referenced this pull request May 8, 2018
Mips has a different way of handling NaN. This makes sure the tests
pass on MIPS as well.

PR-URL: #20377
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
MylesBorins pushed a commit that referenced this pull request May 8, 2018
The performance jit logger is not implemented on mips.

PR-URL: #20377
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@MylesBorins MylesBorins mentioned this pull request May 8, 2018
MylesBorins pushed a commit that referenced this pull request May 8, 2018
Mips has a different way of handling NaN. This makes sure the tests
pass on MIPS as well.

PR-URL: #20377
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
MylesBorins pushed a commit that referenced this pull request May 8, 2018
The performance jit logger is not implemented on mips.

PR-URL: #20377
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
MylesBorins pushed a commit that referenced this pull request May 9, 2018
Mips has a different way of handling NaN. This makes sure the tests
pass on MIPS as well.

PR-URL: #20377
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
MylesBorins pushed a commit that referenced this pull request May 9, 2018
The performance jit logger is not implemented on mips.

PR-URL: #20377
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@BridgeAR BridgeAR deleted the fix-tests-on-mips branch January 20, 2020 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants