Fix regressions in 32bit systems

[berarma]

See #96.

[zakk4223]

That seems to work on the arm.

However, what's the best way to test if it broke this: 2b865af ?
Seems like the same bug will arise

[berarma]

I don't understand the question. If this PR works, I guess the issue will be multiplying unsigned with signed values.

You can write a small test program doing the multiplication and showing the reult combining different types, signed and unsigned.

[zakk4223]

I'm saying that previously, you'd fixed 32-bit issues by casting both of those values to (int). Then you changed it in the commit I referenced to only cast the parameters[0].level to (long). I assume you did this because long is wider than int, but that's not the case on 32bit arm, they are the same size.

Casting to s32 is no different than casting to int, so whatever bug you fixed with the change to long cast will be reintroduced with this change.

[berarma]

I'm saying that previously, you'd fixed 32-bit issues by casting both of those values to (int). Then you changed it in the commit I referenced to only cast the parameters[0].level to (long). I assume you did this because long is wider than int, but that's not the case on 32bit arm, they are the same size.

Casting to s32 is no different than casting to int, so whatever bug you fixed with the change to long cast will be reintroduced with this change.

Aha, I get it. My intention when casting to a long was to be sure that at least a 32bit type was used. In retrospect, this wasn't the best way to do it since long can be different sizes on different architectures. The macro s32 is safer because it guarantees the cast to a 32bit type.

Although most probably this wasn't the issue. Instead multiplying a 32bit signed value with a 32bit unsigned value and storing it in a 32bit signed variable was. I could have casted both to long to the same effect, but I'd rather use s32 this time.

I'll test this PR on my 64bit system, and if it works correctly here and in your 32bit system, I think it's a safer cast than it was.

[berarma]

This isn't working well for me on a 64bits system. I'll take a closer look and see if there's really a need for 64bits types for this calculation.

[berarma]

I've reviewed the code and a 64bit cast is definitely needed. One question though about the proposed solution in #96 (comment), why use div_s64 instead of relying on the compiler to do the division?

[zakk4223]

Compiler will detect 64-bit divide on 32-bit arch and insert a helper function (for gcc it comes from libgcc). Kernel isn't linked with the compiler library so you will get a link error.

[berarma]

I think I've finally nailed it down. It was a case of me badly fixing 32bits support, then breaking it again trying to fix it properly.

Your solution of using 64bits operations was good I guess, as it correctly pointed to the real problem, but I've preferred to avoid using 64bits arithmetic for performance reasons. I've scaled down the gain from 16bits to 8bits so that it all fits into 32bits integers. It shouldn't matter since the hardware only has 255 force levels anyway.

If you could test it before merging it so I'm sure this is really fixed now. I've added some comments so hopefully this doesn't bite me again.

Thank you for your help.

[zakk4223]

The (s64) cast (line 886 to 888) will still cause the compiler to generate a 64bit divide. Linking fails with unknown symbol due to the ldivmod inserted by the compiler.

[berarma]

I'll need to do more tests. Apparently, this isn't enough to make it work with 32bits operations.

I'll be away from the wheel for most of the summer so it might take a while before I can do more tests. Please, use your own fix in the mean time.