HaxeFoundation / haxe

Haxe - The Cross-Platform Toolkit
https://haxe.org
6.14k stars 656 forks source link

[php] Wrong bitwise shift left #7533

Open flashultra opened 5 years ago

flashultra commented 5 years ago

The following code return -1214 on php target and correct one 1534352194 on macro and js target :

        var x = -344720798+1480740470+1886350957;
        var m =  (x << 12) | (x >>> 20 );
        trace("m = "+m);

Or here : https://try.haxe.org/#50f85

RealyUniqueName commented 5 years ago

Right shift is not the problem here. Reduced (without using Haxe):

3022370629 << 12

This yields 12379630096384 on PHP and 1534349312 on JS without Haxe involved. But python yields 12379630096384 too. And Python has unlimited size integers. So, it looks like an overflow on JS and Eval. And since Haxe doesn't handle overflow for Int, there is nothing to do here. @Simn ?

flashultra commented 5 years ago

Yes, the problem is with right shift , not left. It was wrong assumption from my side at the first place. In php there is not existing unsign shift and possible fix is to to add & ~(1<<(8*PHP_INT_SIZE-1)>>($right-1)) ) as I mention here https://github.com/HaxeFoundation/haxe/pull/7565 At example : ‭B425B745‬ >> 20 will give ‭ FFFFFB42‬ , not ‭B42‬ , because B is 11 ( in bits ) and shift will start with 1.

flashultra commented 5 years ago

This is the generated php code from Haxe, if you call shiftRightUnsigned(‭3022370629‬, 20) , the result will be wrong.

    static public function shiftRightUnsigned ($left, $right) {
        #C:\HaxeToolkit\haxe\std/php/Boot.hx:479: lines 479-485
        if ($right === 0) {
            #C:\HaxeToolkit\haxe\std/php/Boot.hx:480: characters 4-15
            return $left;
        } else if ($left >= 0) {
            #C:\HaxeToolkit\haxe\std/php/Boot.hx:482: characters 4-26
            return $left >> $right;
        } else {
            #C:\HaxeToolkit\haxe\std/php/Boot.hx:484: characters 4-56
            return ($left >> $right) & (2147483647 >> ($right - 1));
        }
    }
RealyUniqueName commented 5 years ago

Ok. Looks like you're right. Could you please fix your PR and add a test case?

flashultra commented 5 years ago

Sorry. There is problem with left shift also. In javascript Int is 32 bit and for that reason the result is different from php ( where I think int is 64 bit) . At example : B425B745‬ will produce: 5B745000‬ in javascript , but ‭B425B745000‬ in php, so the problem is with left shift also. I think in Haxe int is 32 bits, so php should apply mask for left shift bitwise operation. I just want to mention something about javascript. If you do , at example 1 >> 31 , the result will be 0 , but if you do 1 >> 32 , result will be 1 ( i.e. if shift > 31 , javascript preserve int vvalue ) I understand that, but it's a little but awkward , when write some code in Haxe and expected to work same on all platforms ( at least for base operators such as shift right and left ) . So for me javascript also should be fixed

RealyUniqueName commented 5 years ago

php should apply mask for left shift bitwise operation

I'm not sure about that. https://haxe.org/manual/types-overflow.html What's our spec here? @ncannasse @Simn

flashultra commented 5 years ago

Just to mention something about javascript. In Javascript numbers are 64 bits floats, but for bitwise operations JS treat numbers as 32 bits. Here is info https://www.w3schools.com/js/js_bitwise.asp ( section "JavaScript Uses 32 bits Bitwise Operands" ) . I think in Haxe bitwise operators are also applied on Int32 ( https://haxe.org/manual/types-numeric-operators.html , operand1 = Int , operand2 = Int ) . It should be different for Int64

ncannasse commented 5 years ago

@RealyUniqueName all bitwise operations (<< >> & | ^ ) should properly overflow on 32 bits.

ncannasse commented 5 years ago

Note : we should specify this in the doc. Overflows are mostly for * and +/-

Simn commented 5 years ago

Is this a regression? Otherwise we can move it to 4.1.

RealyUniqueName commented 5 years ago

It's not a regression

RealyUniqueName commented 5 years ago

@RealyUniqueName all bitwise operations (<< >> & | ^ ) should properly overflow on 32 bits.

@ncannasse Isn't it what haxe.Int32 was invented for? Making all bitwise correctly overflow would also make them much slower.

ncannasse commented 5 years ago

@RealyUniqueName haxe.Int32 was mostly because when doing double double you would lose significant digits and have a different overflow result than when doing a real int32 int32 multiply.

What's the actual solution for overflow on bitwise operators in PHP and how would it be slower than it is now?

RealyUniqueName commented 5 years ago

I've just measured. And applying a mask to left shift makes it slower by 13%

haxe.Timer.measure(() -> {
    for(i in 0...0xFFFFFF) {
        tmp = (i << 2 | php.Syntax.code('0xFFFFFFFF'));
    }
});

reports ~0.455s on each run, while without a mask it's ~0.4s on each run.

Simn commented 10 months ago

FWIW I don't understand why our Int should specify 32bit overflow, given that it's not specified to be 32bit in the first place.