Samsung / walrus

WebAssembly Lightweight RUntime
Apache License 2.0
40 stars 10 forks source link

Bugfix to real size #90

Closed GorogPeter closed 11 months ago

GorogPeter commented 1 year ago

I changed the condition to check the correct size.

zherczeg commented 1 year ago

This looks like a better size detection. @ksh8281 what do you think about this? There are lost of places in the code where valueSizeInStack is used to detect the transfer size, but I think it is incorrect on big endian machines.

clover2123 commented 1 year ago

@zherczeg Is there any potential defects in valueSizeInStack on big-endian? Would you please elaborate on this?

@GorogPeter BTW this patch seems not that different from origin. It still calls valueSizeInStack method to get a size of value but check the type instead of size. Why did you change the condition?

zherczeg commented 1 year ago

@zherczeg Is there any potential defects in valueSizeInStack on big-endian? Would you please elaborate on this?

On 64 bit system, 32 bit values are "rounded up" to 64 bit, and sometimes copied as 64 bit values (e.g. when it is placed in global values). On little endian systems, 32 bit values are mapped to the lower part of the 64 bit value, but on 64 bit systems it is in the upper 32 bit. This caused me all kinds of unexpected issues in the past, and always worry about it.

Btw is there a reason for allocating 8 byte for 32 bit values?

ksh8281 commented 1 year ago

Btw is there a reason for allocating 8 byte for 32 bit values? -> I want to allocate 8 byte only 64-bit machine. In 32-bit system, int32 would be use 4 byte

I want to align every variables on stack by cpu bus size. It may use more stack, but r/w performance is better I think.(especially arm cpu)

If there is issue on big endian system, we can add ci for test it.

zherczeg commented 11 months ago

Is this still valid?

GorogPeter commented 11 months ago

I think it's not valid anymore.