buffer: fix buffer alignment restriction #5752
Conversation
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.
ofrobots
added
buffer
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? |
|
I guess test/addons/buffer-free-callback would be a good place for that? One could have something like |
|
@ofrobots Your test doesn’t check that the alignment can be off, only the size. The |
|
(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? |
ofrobots
force-pushed
the
fix-buffer-alignment-restriction
branch
2 times, most recently
from
a74951a
to
ecb3a97
Compare
|
@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! |
| @@ -13,9 +13,16 @@ static void FreeCallback(char* data, void* hint) { | |||
| void Alloc(const v8::FunctionCallbackInfo<v8::Value>& args) { | |||
| v8::Isolate* isolate = args.GetIsolate(); | |||
| alive++; | |||
|
|
|||
| uintptr_t wantAlignment = args[1]->IntegerValue(); | |||
| uintptr_t wantOffset = args[2]->IntegerValue(); | |||
There was a problem hiding this comment.
Style nits: don't align the RHS and use snake_case for locals.
There was a problem hiding this comment.
Also, maybe drop the want prefix, it suggests the variable is a boolean.
ofrobots
force-pushed
the
fix-buffer-alignment-restriction
branch
from
ecb3a97
to
fc24288
Compare
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
|
LGTM |
1 similar comment
|
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>
|
Awesome, thanks! |
Pull Request check-list
make -j8 test(UNIX) orvcbuild test nosign(Windows) pass withthis change (including linting)?
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-ffiand 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.