HDF-NI / hdf5.node

A node module for reading/writing the HDF5 file format.
MIT License
123 stars 40 forks source link

Alignment trap #66

Closed NINI1988 closed 6 years ago

NINI1988 commented 6 years ago

Code Example:

const table = new Array();

const tempModeCol = new Int8Array(1);
tempModeCol.name = 'temperatureMode';
tempModeCol[0] = -1;
table.push(tempModeCol);

const tempSollCol = new Float64Array(1);
tempSollCol.name = 'temperatureSoll';
tempSollCol[0] =-1;
table.push(tempSollCol);

h5tb.makeTable(id, 'infos', table);

Error in dmesg:

[ 3721.859828] Alignment trap: not handling instruction ed840b00 at [<b46d5486>]
[ 3721.866958] Unhandled fault: alignment exception (0x801) at 0x019c6fc1
[ 3721.873457] pgd = dc398000
[ 3721.876169] [019c6fc1] *pgd=17166831, *pte=140d875f, *ppte=140d8c7f

On windows it works, but under linux on arm makeTable failed with the above error.

It happens on line https://github.com/HDF-NI/hdf5.node/blob/master/src/h5_tb.hpp#L165 i:1, j: 0, type_size:9, field_offsets[i]:1,

Maybe we need to add padding like in this description https://support.hdfgroup.org/HDF5/Tutor/h5table.html

rimmartin commented 6 years ago

It wasn't at any hdf5 method

does the double need to be a double?

rimmartin commented 6 years ago

does https://hdf-ni.github.io/hdf5.node/tut/table-tutorial.html work?

rimmartin commented 6 years ago

I'm working thru the unit tests. lotta breaks. Then I'll add this one

NINI1988 commented 6 years ago

The solution is to use memcopy instead of assignment: Replace:

((double*)&data[j * type_size + field_offsets[i]])[0] = field->Get(j)->NumberValue();

With:

double value = field->Get(j)->NumberValue();
std::memcpy(&data[j * type_size + field_offsets[i]], &value, H5Tget_size(field_types[i]));

I make a pull request.

rimmartin commented 6 years ago

NumberValue might not have been a double

rimmartin commented 6 years ago

that doesn't make sense in my brain.

A double primitive should always go to a double

rimmartin commented 6 years ago

equals sign makes a copy of a primitive

rimmartin commented 6 years ago

be a bit till I get thru my current changes and getting all the unit tests happy again. Then I'll add this as another table unit test

NINI1988 commented 6 years ago

NumberValue is a double.

The problem is the memory access. When you create a double variable, it is always at a 8 byte boundary alignment. (Depending on CPU and compiler) With the pointer arithmetic, we place this double variable on address 1 (not 0 or 8). (Address 1 because before is one byte of Int8Array). Some cpus dont like this.

rimmartin commented 6 years ago

? they can't be in the same array

rimmartin commented 6 years ago

Each column is to it's own type. [ed.] oh you're looking at the data structure being made up for going to hdf5 call

NINI1988 commented 6 years ago

The offset is not in the array. It happens on writing the value into the data char buffer.

rimmartin commented 6 years ago

are int 64's now available in javascript? and which versions of nodejs?

rimmartin commented 6 years ago

I made a wrapper for Int64 and Uint64 because javascript as I know it doesn't have a long long primitive. The number will actually get truncated

NINI1988 commented 6 years ago

Ohh you are right. I used the NumberValue which is double not Uint64. Or maybe use IntegerValue which is int64? https://v8docs.nodesource.com/node-8.9/dc/d0a/classv8_1_1_value.html

rimmartin commented 6 years ago

When hdf5 went to 64 bit integers I had to make the id's Int64 as a wrapper. then on the javascript side most of the time no need to work with them albeit there is a string representation possible provide for javascript side.

then someone had h5 attributes as int 64 and I added using this wrapper there too. But on the javascript side they visually go back and forth from strings. I added no math operators bceause I didn't know how

I'm hoping javascript adds these sometime

rimmartin commented 6 years ago

IIRC underneath a double is used and truncates long long types. We can try printing out again but I remember the 64 int id's from hdf5 getting beat up

NINI1988 commented 6 years ago

Javascript does not support 64 bit integers, because the native number type is a 64-bit double, giving only 53 bits of integer range.

(https://stackoverflow.com/a/24037917).

That's still better than 32 bits ;-)

rimmartin commented 6 years ago

but just doesn't reach 64

rimmartin commented 6 years ago

Are you on a 32 bit machine?

std::cout<<" sizeof long long "<<sizeof(long long) <<std::endl;

put out sizeof long long 8

8 bytes * 8 bits is 64 bits

NINI1988 commented 6 years ago

I know it doesn't reach 64bit. But 53bits are enough for me. I develop on win64 and deploy on linux armv7 (32bit).

The new version of v8 implements BigInts. This could be helpfull. I dont know when it reachs nodejs.

Do you have more questions or something I miss to answer?

rimmartin commented 6 years ago

in the code somewhere can you try std::cout<<" sizeof long long "<<sizeof(long long) <<std::endl; ? I temporarily

 #include <iostream>

to test then rip it out again

BigInt is interesting

rimmartin commented 6 years ago

Oh interesting deployment:-)

We can easily make it provide this long long as long type for 32 bit machines but have it as not supported on 64 bit where it would fail at the current state of javascript

I will study the BigInt

NINI1988 commented 6 years ago

On armv7 long long size is 8 byte.

rimmartin commented 6 years ago

ah then a dilemma

Some users won't know a priori their use will stay within 53 bits of the mantissa & sign bit https://en.wikipedia.org/wiki/Double-precision_floating-point_format

this might require a compiler definition so you can get what you want until a better solution comes along

NINI1988 commented 6 years ago

OK, you mean disable 64bit arrays, because other users don't know that's it's only 53bit?

Or maybe add a hint for this limitation? So other user can use 53bits.

rimmartin commented 6 years ago

yes,

I'll add a

-DLONGLONG53bit

to compiling definitions in binding.gyp to allow this to be turned on for a compile

NINI1988 commented 6 years ago

Thanks

rimmartin commented 6 years ago

LONGLONG53BITS controls this. working on tests for this. And how it works on a windows box

rimmartin commented 6 years ago

adding a switch

--longlong_type=LONGLONG53BITS

which will turn on this support. Default LONGLONG64BITS will fail

close to commit

rimmartin commented 6 years ago
npm install --longlong_type=LONGLONG53BITS  --hdf5_home_linux=$HDF5_HOME

should work for you; I will run unit tests on windows when I get to work. The critical untested line is

https://github.com/HDF-NI/hdf5.node/blob/master/binding.gyp#L247

rimmartin commented 6 years ago

--longlong_type=LONGLONG53BITS works on win64 tests

NINI1988 commented 6 years ago

Great. I'll test also tomorrow.

NINI1988 commented 6 years ago

Works for me on win64 too. :+1: