C++17 support

[KayEss]

There are a few places where 5.6.5 has problems with the changes in C++17. I've been compiling on a recent (self built from current head) of clang (so using libc++). There are two main problems:

• The addition of std::byte causes some confusion in a couple of places.
• std::bind2nd has been removed

I'm happy to work up a pull request if you want for this. For std::byte I think the first thing is to just fully qualify the names, but in the long run maybe using std::byte is the right approach?

For the second I think using lambdas instead should be fine, maybe only for C++14 or later.

[noloader]

Thanks @KayEss,

There are a few places where 5.6.5 has problems

Yeah, Crypto++ 5.6.5 is starting to show its age. Its about a year old but we need to make another release.

The addition of std::byte causes some confusion in a couple of places

Oh man, std::byte is a pain in the butt. We cleared it in Master already. Also see std::byte on the Crypto++ wiki.

std::bind2nd has been removed

We had a PR for std::bind2nd, also see GH #397. We declined std::bind2nd at the time. If std::bind2nd has been removed in C++17 then we need to revisit it. + @MarcelRaad , @RaptorFactor.

Below are the places std::bind2nd is being used. If you work up a PR, then please be sure it does not break C++03.

$ grep 'std::bind2nd' *.h *.cpp | cut -f 1 -d ':' | sort | uniq
filters.cpp
ida.cpp
oaep.cpp
pssr.cpp
zdeflate.cpp

Can you use CRYPTOPP_CXX11, or do you recommend another define? Because of the fake C++11 libraries (see the comments in config.h), I'm thinking it might be a good idea to use a new define, like CRYPTOPP_CXX14.

Using a lambda at C++14 creates a small gap at C++11. But the fake standard C++ libraries cause a lot of problems. They are the ones written in 2005 or 2008 but advertise C++11 support via __cplusplus. Apple and Android using STLport usually have a lot of problems.

You can also add a CRYPTOPP_CXX11_LAMBDA if needed. We do a fair amount of this in the source code for other C++11 features. Also see config.h around line 940. It kind of looks like this:

// alignof/alignas: MS at VS2015 (19.00); GCC at 4.8; Clang at 3.0; Intel 15.0; SunCC 5.13.
#if (CRYPTOPP_MSC_VERSION >= 1900) || __has_feature(cxx_alignas) || \
    (__INTEL_COMPILER >= 1500) || (CRYPTOPP_GCC_VERSION >= 40800) || (__SUNPRO_CC >= 0x5130)
#  define CRYPTOPP_CXX11_ALIGNAS 1
#endif // alignas

Finally, for testing, please use Master and run cryptest.sh on Linux. It stress tests the library under different configurations, like Debug, Release, C++03 through C++17, etc. This should be enough:

./cryptest.sh fast

[KayEss]

./crypttest.sh

And

./crypttest.sh fast

Both take an awful long time, and I get errors from master.

cryptest-result.txt cryptest-warn.txt

I think the c++11 define should be adequate as I'll just change the binds to lambdas (assuming you're OK with that). I think that should be much clearer than the other PR which used std::bind.

[noloader]

I'll just change the binds to lambdas (assuming you're OK with that)

Yeah, that sounds about right. The big requirement is, it can't break C++03. As long as that requirement is satisfied, you can do pretty much anything within reason.

Lambdas can be tricky, especially with the captures. Botan has had some trouble with them in the past. If you have Windows and Solaris machines then be sure to test it. Also see Botan's Fix lambda capture, MSVC didn't like this and Fixes lambda capture error in MSVC.

Both take an awful long time ...

Yeah, it is painful. I usually start the test on another machine and then move onto something else.

... and I get errors from master.

Yes, this is a GCC UBsan bug. Intel specifically allows unaligned loads and stores using MOVSD. Also see Is UBSan supposed to produce a finding for _mm_load_sd and _mm_store_sd:

Testing SymmetricCipher algorithm SIMON-64/ECB.
.../usr/lib/gcc/x86_64-linux-gnu/7/include/emmintrin.h:144:21: runtime error: load of misaligned address 0x7ffe14b27547 for type 'const double', which requires 8 byte alignment
0x7ffe14b27547: note: pointer points here
 db ff aa 63 95  a8 cc 1d f3 bd 8d 81 aa  b7 2a 3b 42 b7 37 a8 9c  b3 63 49 0d 67 03 14 bd  8a a4 1e

And 92 is the exact number we expect to see:

************************************************
92 errors detected. See cryptest-result.txt for details
************************************************

We are supposed to be able to disable the sanitizer on the functions causing the problem, but that does not work either. That is what this is for:

// https://www.spinics.net/lists/gcchelp/msg47735.html and
// https://www.spinics.net/lists/gcchelp/msg47749.html
#if (CRYPTOPP_GCC_VERSION >= 40900)
# define GCC_NO_UBSAN __attribute__ ((no_sanitize_undefined))
#else
# define GCC_NO_UBSAN
#endif

You can work around it with Clang. It does not mis-report on the loads and stores.

CXX=clang++ ./cryptest.sh fast

[KayEss]

OK, cool. Then I'll get on with the code. It's late here so probably not for a day or two though.

[noloader]

@KayEss,

I added CRYPTOPP_CXX11_LAMBDA at Commit 5ae79afd89c9. It is intended to signal Lambda v1.1, which is C++11 and N2927.

There was not much to it:

// lambdas: MS at VS2012 (17.00); GCC at 4.9; Clang at 3.3; Intel 12.0; SunCC 5.14.
#if (CRYPTOPP_MSC_VERSION >= 1700) || __has_feature(cxx_lambda) || \
    (__INTEL_COMPILER >= 1200) || (CRYPTOPP_GCC_VERSION >= 40900) || (__SUNPRO_CC >= 0x5140)
#  define CRYPTOPP_CXX11_LAMBDA 1
#endif // lambdas

That leaves the modifications for:

$ grep 'std::bind2nd' *.h *.cpp | cut -f 1 -d ':' | sort | uniq
filters.cpp
ida.cpp
oaep.cpp
pssr.cpp
zdeflate.cpp

---

By the way... be sure to use -std=c++11 for Clang. It appears they are still advertising C++98. Below is from Fedora 27 and Clang 4.0. WTF...

$ clang++ -x c++ -dM -E - < /dev/null | egrep '(clang|__cplusplus)'
#define __clang__ 1
#define __clang_major__ 4
#define __clang_minor__ 0
#define __clang_patchlevel__ 1
#define __clang_version__ "4.0.1 (tags/RELEASE_401/final)"
#define __cplusplus 199711L

[zabulus]

Question about bind2nd. Why don't just add "find_if_not" custom function that takes value for not predicate and completely remove this pain point for both prev and future versions.

EDIT: proposed solution to use ifdefs with two different code paths and using lambda in one of them to achieve simple != predicate from my sight is something that adds too much complexity, than fixing anything.

@noloader

[noloader]

@zabulus,

The code in this area is Wei's original code. It looks like it predates Crypto++ 5.6.0. I don't know why Wei wrote it that way. It may have been something that all compilers supported at the time. But that is just speculation on my part.

find_if_not works for me too (as long as it works everywhere).

[KayEss]

Why don't just add "find_if_not" custom function that takes value for not predicate and completely remove this pain point for both prev and future versions.

If support for C++03 might ever be dropped in the future then the lambda for now is probably better. If it'll never be dropped then a library function for this would probably be better.

[KayEss]

Every single one of the bind2nd uses is associated also with a use of not_equal_to so actually adding a find_if_not does look like the best choice. Is there any preference to where that function should live?

There is an implementation in std for C++11,, but I guess we want to avoid the conditional compilation here, or do we strongly prefer to use the library version if available? We can just use the conditional compilation on the definition of find_if_not.

[noloader]

@zabulus, @KayEss,

If support for C++03 might ever be dropped in the future then the lambda for now is probably better. ...

C++03 support will probably be dropped, but there's no telling when that's going to happen. If I had to hazard a guess, I would say in 5 or 10 or 15 years.

There are several reasons, but the most important (in my mind's eye) is, users need a quality library even when an OEM abandons a platform. That applies to Apple, Google, Microsoft and the Unixes and Linuxes. As a user you may not have a platform choice - you may be told you need to support Windows 7 and above, iOS 6.0 and above, Android 4.1 and above, etc. A user in that position does not care it is 2018 and C++11 or C++14 is the norm.

---

Every single one of the bind2nd uses is associated also with a use of not_equal_to so actually adding a find_if_not does look like the best choice...

If you like find_if_not better, then I'm happy with it too. I have not encountered the error yet, so I am glad it is on someone else's plate :)

[noloader]

Cleared via Pull Request 559.