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

Does not work with subarrays #5

Closed
mateogianolio opened this issue Sep 7, 2016 · 7 comments
Closed

Does not work with subarrays #5

mateogianolio opened this issue Sep 7, 2016 · 7 comments

Comments

@mateogianolio
Copy link
Member

mateogianolio commented Sep 7, 2016

A bug with Node.js is causing the offsets of subarrays to not be registered when using the following pattern Below code does not work with subarrays because the ArrayBuffer offset is not taken into account:

v8::Local<v8::Float64Array> array = argument.As<v8::Float64Array>();
void *data = array->Buffer()->GetContents().Data();
double *x = static_cast<double*>(data);

This seems to be fixed by using the following hack:

v8::Local<v8::Float64Array> array = argument.As<v8::Float64Array>();
void *data = array->Buffer()->GetContents().Data();

size_t offset = array->ByteOffset();
size_t data_ptr = reinterpret_cast<size_t>(data);
data_ptr += offset;

double *x = reinterpret_cast<double*>(data_ptr);

However, it appears to be undefined behaviour in C++. Not sure if I should wait for Node.js to patch the bug or if I should incorporate above into the code.

Ping @metabench.

@metabench
Copy link

I'd hesitate to call the first example a 'pattern', no offence to its authors intended. It looks like it gets the contents of the underlying buffer, and may not be the right code to match the intention. It's a general area I'm unsure about though, and am asking others, including those who run node.js to consider, and to establish and document what the patterns are.

See nodejs/node#8269

I'm hopeful that those with a thorough understanding of V8 and C++ can agree on what the best practice for this is. I've not yet tried the provided hack. If no-one else has done this yet, I suppose that by default it is best practice as well as being a hack.

@mateogianolio
Copy link
Member Author

mateogianolio commented Sep 7, 2016

Searched the node repo for GetContents() occurrences and found a few interesting results, e.g. this test binding for zlib:

auto buffer = view->Buffer();
auto contents = buffer->GetContents();
auto data = static_cast<unsigned char*>(contents.Data()) + byte_offset;

They cast the underlying buffer to unsigned char* and then add offset.

@mateogianolio
Copy link
Member Author

mateogianolio commented Sep 7, 2016

I got this to work:

void *data = array->Buffer()->GetContents().Data();
size_t offset = array->ByteOffset();
unsigned char *data_offset = static_cast<unsigned char*>(data) + offset;
double *x = reinterpret_cast<double*>(data_offset);

@metabench
Copy link

The first line of that example does not have matching brackets. Having removed an extra bracket at the end, I also get a compilation error:

error C2036: 'void *' : unknown size

on the line that corresponds with:

unsigned char* data_offset = static_cast<unsigned char*>(data + offset);

@mateogianolio
Copy link
Member Author

Wow that was sloppy pasting on my part, the + offset should be outside of the cast. Edited the code above.

@metabench
Copy link

I took the last two lines of your corrected code and condensed it into:

double *x = reinterpret_cast<double*>(static_cast<unsigned char*>(data) + offset);

I suggest that would be better as it does not leave the unsigned char *data_offset data structure in memory, what's your opinion on this, @mateogianolio?

@mateogianolio
Copy link
Member Author

Looks good!

I'm going to update the code in this repo with the fix and submit a pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants