Closed manxorist closed 9 months ago
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.
@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.
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.
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.
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++'sCPUID64
.
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
andx64dll.masm
. Both files are algorithm implementations, and you should be able to avoid them whenCRYPTOPP_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 forXGETBV64
andCPUID64
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.
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>
This should be cleared at Commit 121014baf0e8.
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?
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.
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.
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.
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.
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.
The library will be compiled using C++ only.
yeah ... so?! What am I misunderstanding here?!
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.
It looks like we're getting a fix for this...
... and I am already using it (see above / https://github.com/OpenMPT/openmpt/commit/979aece26ac208a7c3a7ff847c3410d70d9cafd1) ...
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 https://github.com/weidai11/cryptopp/commit/49fef81923d9e9ed6706c7ad3a503bfc2aeceedc - this decision IMHO needs far more discussion and approval from other contributors and users.
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 https://github.com/weidai11/cryptopp/commit/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 ?
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
.
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 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".
#define CRYPTOPP_DISABLE_ASM 1
does not disable all assembler / builtins / intrinsics (as documented here: https://github.com/weidai11/cryptopp/blob/CRYPTOPP_8_9_0/config_asm.h#L30).cpu.cpp
on x86-64 Windows MSVC builds still wantsCPUID64
andXGETBV64
which are only implemented inx64dll.asm
(see https://github.com/weidai11/cryptopp/blob/843d74c7c97f9e19a615b8ff3c0ca06599ca501b/cpu.cpp#L451 / https://github.com/weidai11/cryptopp/blob/843d74c7c97f9e19a615b8ff3c0ca06599ca501b/cpu.cpp#L393 / https://github.com/weidai11/cryptopp/blob/843d74c7c97f9e19a615b8ff3c0ca06599ca501b/cpu.cpp#L81). This makes building Crypto++ as pure C++ impossible.@noloader The offending commit is https://github.com/weidai11/cryptopp/commit/e65fa006bec60192112f0a3f23174ef0528f8cc4 which removes the use of the MSVC-specific intrinsics for MSVC versions new enough to actually provide it. I suggest reverting the part of this commit that removes the use of
__cpuidex
and_xgetbv
.For OpenMPT, we are building Crypto++ with our own build system in which we define
CRYPTOPP_DISABLE_ASM 1
. This worked with 8.8.0, however it fails now with 8.9.0. Crypto++ is not performance critical in our case, thus we very much prefer a simple C++-only solution in order to keep the build system simple.We are using Crypto++ to secure our automatic updates on older Windows versions which do not provide the necessary crypto primitives themselves. In the failing case, we use VS2017 to target Windows XP.