Skip to content

Conversation

@addaleax
Copy link
Member

The double fields in performance_state could previously have
been aligned at 4-byte instead of 8-byte boundaries, which would
have made creating an Float64Array them as a array buffer view
for an ArrayBuffer extending over the entire struct an invalid
operation.

Ref: 67269fd

Would suggest fast-tracking to unbreak CI, assuming this works.

/cc @jasnell

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Aug 24, 2017
@jasnell
Copy link
Member

jasnell commented Aug 24, 2017

+1 on fast tracking :-) ... assuming we can get CI to stay up long enough to do an actual test.

The `double` fields in `performance_state` could previously have
been aligned at 4-byte instead of 8-byte boundaries, which would
have made creating an Float64Array them as a array buffer view
for an ArrayBuffer extending over the entire struct an invalid
operation.

Ref: 67269fd
@addaleax addaleax changed the title src: fix 32-bit platform build src: fix build on certain platforms Aug 24, 2017
@addaleax
Copy link
Member Author

Updated the PR & did the same thing for HTTP2 where this worked out by accident because there was an even number of uint32_t fields

@jasnell
Copy link
Member

jasnell commented Aug 24, 2017

Yep, it was a bug waiting to happen in the http2 code too. Those fields are quite likely to change. Thank you for including those.

@addaleax
Copy link
Member Author

CI: https://ci.nodejs.org/job/node-test-commit/11990/

Already got some buildbot failures so we might be trying again soon

@jasnell
Copy link
Member

jasnell commented Aug 24, 2017

The other run is almost complete, starting a new one see if we can get past the buildbot failures https://ci.nodejs.org/job/node-test-pull-request/9814/

@refack
Copy link
Contributor

refack commented Aug 24, 2017

+1 for fast tracking.

The other run is almost complete, starting a new one see if we can get past the buildbot failures https://ci.nodejs.org/job/node-test-pull-request/9814/

BTW: You can stop the job then hit "rebuild", that will only retry the incomplete/failed subjobs.

@jasnell
Copy link
Member

jasnell commented Aug 24, 2017

This is looking good so far. I did push a commit in to comment out a flaky precision comparison in one of the tests (it's the first time I've seen it.. and I'll revisit it in a separate PR.). Assuming this run is clear I'll land shortly.

@jasnell
Copy link
Member

jasnell commented Aug 24, 2017

Yes i know we can "rebuild" on the CI. Given the build bot failures I wanted to do a complete fresh rerun.

jasnell pushed a commit that referenced this pull request Aug 24, 2017
The `double` fields in `performance_state` could previously have
been aligned at 4-byte instead of 8-byte boundaries, which would
have made creating an Float64Array them as a array buffer view
for an ArrayBuffer extending over the entire struct an invalid
operation.

Ref: 67269fd

Comments out related flaky failure

PR-URL: #14996
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@jasnell
Copy link
Member

jasnell commented Aug 24, 2017

Landed in 86c4655

@jasnell jasnell closed this Aug 24, 2017
addaleax added a commit to addaleax/ayo that referenced this pull request Aug 25, 2017
The `double` fields in `performance_state` could previously have
been aligned at 4-byte instead of 8-byte boundaries, which would
have made creating an Float64Array them as a array buffer view
for an ArrayBuffer extending over the entire struct an invalid
operation.

Ref: 67269fd

Comments out related flaky failure

PR-URL: nodejs/node#14996
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
addaleax added a commit to ayojs/ayo that referenced this pull request Aug 28, 2017
The `double` fields in `performance_state` could previously have
been aligned at 4-byte instead of 8-byte boundaries, which would
have made creating an Float64Array them as a array buffer view
for an ArrayBuffer extending over the entire struct an invalid
operation.

Ref: 67269fd

Comments out related flaky failure

PR-URL: nodejs/node#14996
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 10, 2017
The `double` fields in `performance_state` could previously have
been aligned at 4-byte instead of 8-byte boundaries, which would
have made creating an Float64Array them as a array buffer view
for an ArrayBuffer extending over the entire struct an invalid
operation.

Ref: 67269fd

Comments out related flaky failure

PR-URL: #14996
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Sep 10, 2017
MylesBorins pushed a commit that referenced this pull request Sep 11, 2017
The `double` fields in `performance_state` could previously have
been aligned at 4-byte instead of 8-byte boundaries, which would
have made creating an Float64Array them as a array buffer view
for an ArrayBuffer extending over the entire struct an invalid
operation.

Ref: 67269fd

Comments out related flaky failure

PR-URL: #14996
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 12, 2017
The `double` fields in `performance_state` could previously have
been aligned at 4-byte instead of 8-byte boundaries, which would
have made creating an Float64Array them as a array buffer view
for an ArrayBuffer extending over the entire struct an invalid
operation.

Ref: 67269fd

Comments out related flaky failure

PR-URL: #14996
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants