jedisct1 / libsodium

A modern, portable, easy to use crypto library.
https://libsodium.org
Other
12.06k stars 1.72k forks source link

Do not redefine _mm_roti_epi64 if xop feature set enabled #1305

Closed Scr3amer closed 10 months ago

Scr3amer commented 10 months ago

Hello,

Me again with another PR to fix some warnings when compiling the library. This time it is about macro redefinition.

My toolchain has already _mm_roti_epi64 that returns __builtin_ia32_vprotqi. However it fails if I don't enable the feature set XOP via -mxop but I am targeting a hardware that doesn't have it so instead I decided to change the code as follow:

I just have one question: is this redefinition due to security reasons ? There is a flaw with the builtin rotation ? Or it is just to make it work on all targets ? I assumed the latter but I could be wrong, not an expert.

That's all. Cheers, Scr3am

PS: Should we put _mm_roti_epi64 in its own header file and include it in the 3 headers ? Instead of copy-pasting it 3 times ?

jedisct1 commented 10 months ago

Why not, thanks!

But how did you end up with _mm_roti_epi64 being defined?

That requires <x86intrin.h> to be included, which we don't do.

What compiler did you use?

Scr3amer commented 10 months ago

So this is the include chain I have that leads to x86intrin.h:

. ./include/sodium/private/common.h .. my_toolchain/lib/clang/16/include/intrin.h ... my_toolchain/include/intrin.h .... my_toolchain/lib/clang/16/include/x86intrin.h

I use this toolchain https://github.com/mstorsjo/llvm-mingw/releases, version 16.0.6