CRYPTOPP_DISABLE_ASM is broken in 8.9.0 for MSVC x64 builds about cryptopp HOT 20 CLOSED

I still do not get why removing essential features (like not requiring additional tools over solely a C++ compiler) that your users have been relying upon for multiple years is something I should just swallow without complaining about. Pretty please revert 49fef81 - this decision IMHO needs far more discussion and approval from other contributors and users.

Just making sure we're on the same page here... With 121014baf0e8 you do not need to compile the ASM files anymore. At least, this is what I am doing in my build. Could you confirm that @manxorist ?

Yes, confirmed.

I still take issue with the removed comment for CRYPTOPP_DISABLE_ASM.

from cryptopp.

In the long term, CRYPTOPP_DISABLE_ASM should probably just disable all/most of cpu.cpp and all/most of its uses, but that is a way bigger change and kind of a separate issue, IMHO.

from cryptopp.

@manxorist,

I don't quite understand your use case. But looking at the comment that says, "XGETBV64 and CPUID64 are in x64dll.asm," I'm thinking it should be in x64masm.asm so it is always available to you.

from cryptopp.

CRYPTOPP_DISABLE_ASM claims to disable all assembler. x64dll.asm is assembler. Why do I suddenly need to teach my build system how to build asm files with Crypto++ 8.9.0? This was NOT the case in 8.8.0. This is a very obvious regression and in conflict with documentation of CRYPTOPP_DISABLE_ASM.

from cryptopp.

CRYPTOPP_DISABLE_ASM claims to disable all assembler.

That's not quite correct.

CRYPTOPP_DISABLE_ASM will disable ASM-based and intrinsic-based algorithm implementations. It has never disabled feature detection. Consider, projects were still getting ASM whether it was MS's __cpuidex or Crypto++'s CPUID64.

I think moving forward, I'm going to put XGETBV64 and CPUID64 in its own *.asm file to avoid both x64masm.asm and x64dll.asm. Both files are algorithm implementations, and you should be able to avoid them when CRYPTOPP_DISABLE_ASM is in effect.

Now, when CRYPTOPP_DISABLE_ASM is in effect, maybe we need to provide some stubs for XGETBV64 and CPUID64 that just return 0 values.

from cryptopp.

CRYPTOPP_DISABLE_ASM claims to disable all assembler.

That's not quite correct.

CRYPTOPP_DISABLE_ASM will disable ASM-based and intrinsic-based algorithm implementations. It has never disabled feature detection. Consider, projects were still getting ASM whether it was MS's __cpuidex or Crypto++'s CPUID64.

But feature detection did not required ASM with MSVC before.

I think moving forward, I'm going to put XGETBV64 and CPUID64 in its own *.asm file to avoid both x64masm.masm and x64dll.masm. Both files are algorithm implementations, and you should be able to avoid them when CRYPTOPP_DISABLE_ASM.

I fail to see how that will help. The problem is requiring ASM at all, which was introduced by removing the use of the compiler intrinsics.

Now, when CRYPTOPP_DISABLE_ASM is in effect, maybe we need to provide some stubs for XGETBV64 and CPUID64 that just return 0 values.

That might also be an option.

Let me try to rephrase the problem I am complaining about here: There is currently no way at all disable the dependency of the C++ code on ASM code for MSVC x64. There was in 8.8.0, which used intrinsics instead of requiring ASM. Intrinsics are less of a problem because they are provided by the compiler itself and do not require build system adoption.

from cryptopp.

How does this look to you:

$ git diff
diff --git a/cpu.cpp b/cpu.cpp
index 44b57c4d..ff8c5cee 100644
--- a/cpu.cpp
+++ b/cpu.cpp
@@ -388,9 +388,14 @@ extern bool CPU_ProbeSSE2();
 // https://gcc.gnu.org/bugzilla/show_bug.cgi?id=85684.
 word64 XGetBV(word32 num)
 {
+// Explicitly handle CRYPTOPP_DISABLE_ASM case.^M
+// https://github.com/weidai11/cryptopp/issues/1240^M
+#if defined(CRYPTOPP_DISABLE_ASM)^M
+       return 0;^M
+^M
 // Required by Visual Studio 2008 and below and Clang on Windows.
 // Use it for all MSVC-compatible compilers.
-#if defined(_M_X64) && defined(CRYPTOPP_MS_STYLE_INLINE_ASSEMBLY)
+#elif defined(_M_X64) && defined(CRYPTOPP_MS_STYLE_INLINE_ASSEMBLY)^M
 
        return XGETBV64(num);
 
@@ -446,9 +451,15 @@ word64 XGetBV(word32 num)
 // cpu.cpp (131): E2211 Inline assembly not allowed in inline and template functions
 bool CpuId(word32 func, word32 subfunc, word32 output[4])
 {
+// Explicitly handle CRYPTOPP_DISABLE_ASM case.^M
+// https://github.com/weidai11/cryptopp/issues/1240^M
+#if defined(CRYPTOPP_DISABLE_ASM)^M
+       output[0] = output[1] = output[2] = output[3] = 0;^M
+       return false;^M
+^M
 // Required by Visual Studio 2008 and below and Clang on Windows.
 // Use it for all MSVC-compatible compilers.
-#if defined(_M_X64) && defined(CRYPTOPP_MS_STYLE_INLINE_ASSEMBLY)
+#elif defined(_M_X64) && defined(CRYPTOPP_MS_STYLE_INLINE_ASSEMBLY)^M
 
        CPUID64(func, subfunc, output);
        return true;

A quick smoke test on Linux looks like it does nto break the compile, and the self tests pass. Looking at cpu.o disassembly, it looks like most feature detection was gutted:

$ objdump --disassemble cpu.o | c++filt 

cpu.o:     file format elf64-x86-64

Disassembly of section .text:

0000000000000000 <CryptoPP::XGetBV(unsigned int)>:
   0:   f3 0f 1e fa             endbr64 
   4:   31 c0                   xor    %eax,%eax
   6:   c3                      ret    
   7:   66 0f 1f 84 00 00 00    nopw   0x0(%rax,%rax,1)
   e:   00 00 

0000000000000010 <CryptoPP::CpuId(unsigned int, unsigned int, unsigned int*)>:
  10:   f3 0f 1e fa             endbr64 
  14:   66 0f ef c0             pxor   %xmm0,%xmm0
  18:   31 c0                   xor    %eax,%eax
  1a:   0f 11 02                movups %xmm0,(%rdx)
  1d:   c3                      ret    
  1e:   66 90                   xchg   %ax,%ax

0000000000000020 <CryptoPP::DetectX86Features()>:
  20:   f3 0f 1e fa             endbr64 
  24:   48 83 ec 08             sub    $0x8,%rsp
  28:   bf be 00 00 00          mov    $0xbe,%edi
  2d:   e8 00 00 00 00          call   32 <CryptoPP::DetectX86Features()+0x12>
  32:   48 8b 15 00 00 00 00    mov    0x0(%rip),%rdx        # 39 <CryptoPP::DetectX86Features()+0x19>
  39:   8b 0a                   mov    (%rdx),%ecx
  3b:   85 c0                   test   %eax,%eax
  3d:   7e 1d                   jle    5c <CryptoPP::DetectX86Features()+0x3c>
  3f:   85 c9                   test   %ecx,%ecx
  41:   74 15                   je     58 <CryptoPP::DetectX86Features()+0x38>
  43:   48 8b 05 00 00 00 00    mov    0x0(%rip),%rax        # 4a <CryptoPP::DetectX86Features()+0x2a>
  4a:   c6 00 01                movb   $0x1,(%rax)
  4d:   48 83 c4 08             add    $0x8,%rsp
  51:   c3                      ret    
  52:   66 0f 1f 44 00 00       nopw   0x0(%rax,%rax,1)
  58:   89 02                   mov    %eax,(%rdx)
  5a:   89 c1                   mov    %eax,%ecx
  5c:   85 c9                   test   %ecx,%ecx
  5e:   75 e3                   jne    43 <CryptoPP::DetectX86Features()+0x23>
  60:   48 8b 05 00 00 00 00    mov    0x0(%rip),%rax        # 67 <CryptoPP::DetectX86Features()+0x47>
  67:   c7 02 40 00 00 00       movl   $0x40,(%rdx)
  6d:   c6 00 01                movb   $0x1,(%rax)
  70:   48 83 c4 08             add    $0x8,%rsp
  74:   c3                      ret    

Disassembly of section .text.startup:

0000000000000000 <_GLOBAL__sub_I.00260_cpu.cpp>:
   0:   f3 0f 1e fa             endbr64 
   4:   e9 00 00 00 00          jmp    9 <CryptoPP::g_isP4>

from cryptopp.

This should be cleared at Commit 121014baf0e8.

from cryptopp.

Yeah, that certainly works for me.

I still wonder why you prefer to keep only the ASM CPUID implementation even if the compiler supports intrinsics which do not require the use of an external assembler?

It does not look like Crypto++ currently distinguish at a high level between using ASM or intrinsic implementations for its algorithms, but if it someday does, having the intrinsic CPUID implementation has clear advantages (i.e. not requiring tools other than the compiler itself). At a lower level there are more fine-grained macros (i.e. CRYPTOPP_SSE2_INTRIN_AVAILABLE vs CRYPTOPP_SSE2_ASM_AVAILABLE). Longer term, it might make sense to split some CRYPTOPP_DISABLE_INTRINSICS from CRYPTOPP_DISABLE_ASM, maybe?

from cryptopp.

I still wonder why you prefer to keep only the ASM CPUID implementation even if the compiler supports intrinsics which do not require the use of an external assembler?

A single implementation is advantageous for maintenance. We no longer need to differentiate between versions of Visual Studio or GCC or Clang that added support for something.

from cryptopp.

I still wonder why you prefer to keep only the ASM CPUID implementation even if the compiler supports intrinsics which do not require the use of an external assembler?

A single implementation is advantageous for maintenance. We no longer need to differentiate between versions of Visual Studio that added support for something.

I somewhat doubt that adding a forced dependency on another build tool was advantageous for maintenance or for your users. It made Crypto++ impossible to build with solely a C++ compiler.

from cryptopp.

I somewhat doubt that adding a forced dependency on another build tool was advantageous for maintenance or for your users. I

I told you why we did it. You can take it or leave it.

You happened to get lucky in the past with your practices. That broke at Crypto++ 8.9. That's what happens when you wander off the ranch, and start making up your own rules like not compiling or assembling certain source files.

You're the cause of this problem, not the library.

from cryptopp.

You're the cause of this problem, not the library.

Your C++ library suddenly REQUIRING not only a C++ compiler but also an assembler is the root cause of the problem. CRYPTOPP_DISABLE_ASM explicitly states The library will be compiled using C++ only. (see https://github.com/weidai11/cryptopp/blob/843d74c7c97f9e19a615b8ff3c0ca06599ca501b/config_asm.h#L30C58-L31C29)

This advertised feature was the single main reason for choosing Crypto++ at all in the first place for me.

from cryptopp.

Your C++ library suddenly REQUIRING not only a C++ compiler but also an assembler is the root cause of the problem.

You have always needed a complete toolchain. We have never advertised you did not need one.

You should probably switch to Botan or OpenSSL since you are so dissatisfied at the job we are doing.

from cryptopp.

The library will be compiled using C++ only.

yeah ... so?! What am I misunderstanding here?!

from cryptopp.

It looks like we're getting a fix for this... Just chill for a bit while @noloader is preparing it.
Please be nice to OpenSource project maintainers. You cannot imaging how much time we spend dealing with these special cases that seem benign but use enormous amounts of time from us testing and making sure nothing else is broken.
#peace

from cryptopp.

It looks like we're getting a fix for this...

... and I am already using it (see above / OpenMPT/openmpt@979aece) ...

You cannot imaging how much time we spend dealing with these special cases that seem benign but use enormous amounts of time from us testing and making sure nothing else is broken. #peace

As an Open Source maintainer myself I certainly know that.

I still do not get why removing essential features (like not requiring additional tools over solely a C++ compiler) that your users have been relying upon for multiple years is something I should just swallow without complaining about. Pretty please revert 49fef81 - this decision IMHO needs far more discussion and approval from other contributors and users.

from cryptopp.

I still do not get why removing essential features (like not requiring additional tools over solely a C++ compiler) that your users have been relying upon for multiple years is something I should just swallow without complaining about. Pretty please revert 49fef81 - this decision IMHO needs far more discussion and approval from other contributors and users.

Just making sure we're on the same page here...
With 121014baf0e8 you do not need to compile the ASM files anymore. At least, this is what I am doing in my build. Could you confirm that @manxorist ?

from cryptopp.

I still take issue with the removed comment for CRYPTOPP_DISABLE_ASM.

My understanding is that the revised comment reflects the actual state of the implementation. How would you rephrase it?

from cryptopp.

I still take issue with the removed comment for CRYPTOPP_DISABLE_ASM.

My understanding is that the revised comment reflects the actual state of the implementation. How would you rephrase it?

I would very much prefer if the implementation would stick to its historic promises, as it did in earlier versions.

If Crypto++ really wants to start requiring building with an assembler, it fails to meet my requirements for a "C++ library".

from cryptopp.