buffer: fix buffer alignment restriction by ofrobots · Pull Request #5752 · nodejs/node · GitHub

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)

buffer

Description of change

@addaleax recently reported that recent phantom weakness changes (ebbbc5a) to buffer break node-ffi and possibly other modules that use create unaligned buffers.

It turns out there is a simpler solution possible that doesn't introduce an alignment restriction. This also eliminates the need for Node-core to reserve the first internal field on buffers.

Ref: #5204
R=@bnoordhuis, @trevnorris

Marking with dont-land-on-* as the pre-requisite change is also marked likewise.

Recent phantom weakness API changes to buffer, ebbbc5a, ending up introducing an alignment restriction on the native buffer pointers. It turns out that there are uses in the modules ecosystem that rely on the ability to create buffers with unaligned pointers (e.g. node-ffi). It turns out there is a simpler solution possible here. As a side effect this also removes the need to have to reserve the first internal field on buffers.

Old style SetWeak is now deprecated, and weakness now works like phantom references. This means we no longer have a reference to the object in the weak callback. We use a kInternalFields style weak callback which provides us with the contents of 2 internal fields where we can squirrel away the native buffer pointer. We can no longer neuter the buffer in the weak callback, but that should be unnecessary as the object is going to be GC'd during the current gc cycle. PR-URL: #5204 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: indutny - Fedor Indutny <fedor.indutny@gmail.com>

@ofrobots If you don’t happen to be working on it – mind if I try and write a test for this?

I understand it may be difficult, but is there a way to add a test on this?
(@addaleax.. jinx!)

I guess test/addons/buffer-free-callback would be a good place for that? One could have something like binding.alloc(size, alignment, offset) which returns a Buffer based on a pointer that’s offset bytes from a alignment boundary and try that for a few combinations.

@addaleax @jasnell Added a test already 😄.

@ofrobots Your test doesn’t check that the alignment can be off, only the size. The buf constant in the binding.cc is always aligned the same.

(Not that testing the size too would be a bad idea or anything!)

@addaleax you're right. I will fix tomorrow, or if you're inclined, I would be happy if you want to contribute the test.

sure, shouldn’t take too long :)

What do you think of addaleax/node@63649a5a59fec3cd8ce?

@addaleax Thanks for the test! I have added your commit to this PR (with a slightly modified commit message.)

@ofrobots Sure – I assumed everything here ends up in a single commit anyway, but I guess you can assess that better than I do 😄

@addaleax These commits don't need to be squashed because they can pass testing individually. If this PR lands, the test commit will be correctly attributed as being your contribution. Thanks again for contributing the test, and for raising the issue!

@ofrobots Ah, that makes sense then. Good to know, I’ll think of that next time!

Style nits: don't align the RHS and use snake_case for locals.

Also, maybe drop the want prefix, it suggests the variable is a boolean.

Done.

Buffers instances can have arbitrary alignment. `node-ffi` depends on this. Add some regression tests to ensure we don't break this in the future. PR-URL: nodejs#5752

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

LGTM

Recent phantom weakness API changes to buffer, ebbbc5a, ending up introducing an alignment restriction on the native buffer pointers. It turns out that there are uses in the modules ecosystem that rely on the ability to create buffers with unaligned pointers (e.g. node-ffi). It turns out there is a simpler solution possible here. As a side effect this also removes the need to have to reserve the first internal field on buffers. PR-URL: #5752 Reviewed-By: trevnorris - Trevor Norris <trev.norris@gmail.com> Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>

Buffers instances can have arbitrary alignment. `node-ffi` depends on this. Add some regression tests to ensure we don't break this in the future. PR-URL: #5752 Reviewed-By: trevnorris - Trevor Norris <trev.norris@gmail.com> Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>

Thanks. Landed as 73fc440, be97db9.

Awesome, thanks!

ofrobots added buffer Issues and PRs related to the buffer subsystem. v8 engine Issues and PRs related to the V8 dependency. dont-land-on-v5.x labels Mar 17, 2016

ofrobots force-pushed the fix-buffer-alignment-restriction branch 2 times, most recently from a74951a to ecb3a97 Compare March 17, 2016 04:13

bnoordhuis reviewed Mar 18, 2016
View reviewed changes

ofrobots force-pushed the fix-buffer-alignment-restriction branch from ecb3a97 to fc24288 Compare March 18, 2016 13:12

test: add buffer alignment regression tests

662dad4

Buffers instances can have arbitrary alignment. `node-ffi` depends on this. Add some regression tests to ensure we don't break this in the future. PR-URL: nodejs#5752

ofrobots force-pushed the fix-buffer-alignment-restriction branch from fc24288 to 662dad4 Compare March 18, 2016 13:14

ofrobots closed this Mar 19, 2016

ofrobots deleted the fix-buffer-alignment-restriction branch March 19, 2016 14:14

buffer: fix buffer alignment restriction by ofrobots · Pull Request #5752 · nodejs/node · GitHub

Uh oh!

Uh oh!

buffer: fix buffer alignment restriction - #5752

buffer: fix buffer alignment restriction#5752ofrobots wants to merge 2 commits intonodejs:masternodejs/node:masterfrom ofrobots:fix-buffer-alignment-restrictionofrobots/node:fix-buffer-alignment-restrictionCopy head branch name to clipboard

Conversation

ofrobots commented Mar 17, 2016

Pull Request check-list

Affected core subsystem(s)

Description of change

Uh oh!

addaleax commented Mar 17, 2016

Uh oh!

jasnell commented Mar 17, 2016

Uh oh!

addaleax commented Mar 17, 2016

Uh oh!

ofrobots commented Mar 17, 2016

Uh oh!

addaleax commented Mar 17, 2016

Uh oh!

addaleax commented Mar 17, 2016

Uh oh!

ofrobots commented Mar 17, 2016

Uh oh!

addaleax commented Mar 17, 2016

Uh oh!

addaleax commented Mar 17, 2016

Uh oh!

ofrobots commented Mar 17, 2016

Uh oh!

addaleax commented Mar 17, 2016

Uh oh!

ofrobots commented Mar 17, 2016

Uh oh!

addaleax commented Mar 17, 2016

Uh oh!

bnoordhuis Mar 18, 2016

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Mar 18, 2016

Choose a reason for hiding this comment

Uh oh!

ofrobots Mar 18, 2016

Choose a reason for hiding this comment

Uh oh!

ofrobots commented Mar 18, 2016

Uh oh!

trevnorris commented Mar 18, 2016

Uh oh!

bnoordhuis commented Mar 19, 2016

Uh oh!

ofrobots commented Mar 19, 2016

Uh oh!

addaleax commented Mar 19, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

buffer: fix buffer alignment restriction#5752
ofrobots wants to merge 2 commits into
nodejs:masterfrom
ofrobots:fix-buffer-alignment-restriction