noloader / cryptopp-cmake

CMake files for Crypto++ project
BSD 3-Clause "New" or "Revised" License
92 stars 67 forks source link

Setting DISABLE_ASM to ON breaks Visual Studio 2008 and lower builds #44

Closed shmuelie closed 5 years ago

shmuelie commented 5 years ago

When building using Visual Studio 2008 or lower, ASM is required. See cpu.cpp:59.

noloader commented 5 years ago

Thanks @SamuelEnglard,

Is this still a problem?

Also, you should only need DISABLE_ASM as a last resort. I am interested in learning the reason you need it. We should be able to fix it in the library.

shmuelie commented 5 years ago

I'm not the one who disabled it so I'll have to ask why. The problem is still there even with my fix.

noloader commented 5 years ago

@SamuelEnglard,

Please provide the verbose output from Cmake. We need to see the command line used and the error.

@MarcelRaad, @abdes, (other Windows folks?)

Can someone check on this problem and advise?

I'd like to clear this issue if possible (before tagging the CRYPTOPP_8_0_0 release).

MarcelRaad commented 5 years ago

As VS 2008 doesn't support inline assembly in 64-bit mode and obviously doesn't provide the required intrinsic, I don't think there's anything that can be done about that. Except maybe rejecting DISABLE_ASM for 64-bit with a nicer error message. To be clear, the problem doesn't exist when compiling in 32-bit mode as inline assembly is used then.

noloader commented 5 years ago

As VS 2008 doesn't support inline assembly in 64-bit mode and obviously doesn't provide the required intrinsic, I don't think there's anything that can be done about that.

Right, but we don't have the issue on VS2010 or above. Or I don't believe we do. Here's what I see when using MSBuild from VS2012 (I don't have a CMake machine for this test):

C:\Users\Jeff\cryptopp>msbuild /t:Build /p:Configuration=Debug;Platform=x64 cryptlib.vcxproj
Microsoft (R) Build Engine version 4.7.3062.0
[Microsoft .NET Framework, version 4.0.30319.42000]
Copyright (C) Microsoft Corporation. All rights reserved.

Build started 12/28/2018 10:03:16 AM.
Project "C:\Users\Jeff\Desktop\cryptopp\cryptlib.vcxproj" on node 1 (Build targ
et(s)).
PrepareForBuild:
  Creating directory "x64\cryptlib\Debug\".
  Creating directory "x64\Output\Debug\".
...

  cryptlib.vcxproj -> C:\Users\Jeff\cryptopp\x64\Output\Debug\cryptlib.lib
FinalizeBuildStatus:
  Deleting file "x64\cryptlib\Debug\cryptlib.unsuccessfulbuild".
  Touching "x64\cryptlib\Debug\cryptlib.lastbuildstate".
Done Building Project "C:\Users\Jeff\cryptopp\cryptlib.vcxproj" (Build target(s)).

Build succeeded.
    0 Warning(s)
    0 Error(s)

Time Elapsed 00:00:51.07

I don't see any cross-pollination of inline ASM. And x64dll.asm is assembled by MASM, so it should be OK:

    ALIGN   8
ExtendedControlRegister PROC
;; First paramter is RCX, and xgetbv expects the CTRL in ECX
;; http://www.agner.org/optimize/vectorclass/read.php?i=65
DB      0fh, 01h, 0d0h
;; xcr = (EDX << 32) | EAX
and     rax, 0ffffffffh
shl     rdx, 32
or      rax, rdx
ret
ExtendedControlRegister ENDP

I wonder if CMake is not finding the 64-bit version of MASM. I think it was called masm64.exe back then. Didin't Microsoft use an unusual path for the 64-bit build tools for a while?

MarcelRaad commented 5 years ago

I'm not sure I understand the problem, but with DISABLE_ASM, neither x64dll.asm nor MASM are used: https://github.com/noloader/cryptopp-cmake/blob/b97d72f083fefa249e46ae3c15a2c294e615fca2/CMakeLists.txt#L567 This leads to linker errors about missing ExtendedControlRegister definition.

noloader commented 5 years ago

I'm not sure I understand the problem

Yeah, me too.

@SamuelEnglard, we need the command that is producing the error, and the exact error message. We can revisit this if you can provide the information.

shmuelie commented 5 years ago

@MarcelRaad summed up the issue perfectly. If DISABLE_ASM is set for a 64bit build you get linking errors for ExtendedControlRegister. I honestly don't think there is a way to really fix this on Cryptopp's side. I think the CMake just needs to not allow the combination of DISABLE_ASM when using the Visual Studio 9 2008 generator targeting x64. It's simply an invalid configuration.