[C++17] Use ::byte instead of byte

[Romain-Geissler-1A]

Contains the commit of #437. All the rest is the conversion from "byte" to "::byte" to allow C++17 build even with "using namespace std".

[noloader]

@Romain-Geissler-1A,

I may need to test this change more completely.

Did you happen to test it against -std=c++03 and friends?

[Romain-Geissler-1A]

Mmmh I didn't thought it would be an issue, but apparently the build with -std=gnu++03 is broken.

Will fix that.

[Romain-Geissler-1A]

I have just updated my previous commit using this additional sed:

sed -e 's/<::byte\>/< ::byte/g' -i $(git grep -l '<::byte\>')

Now it seems to build fine with -std=gnu++03. Will try with more standards (03, 11, 14, 17, gnu++ vs c++).

[noloader]

Thanks @Romain-Geissler-1A.

I opened a thread on the mailing list about it: PR 438: Use ::byte instead of byte. This seems like one of things we want to see all of the options to minimize pain points for everyone.

[Romain-Geissler-1A]

The current version of the pull request with all version of standard (tested with gcc 7 only).

I may give it a try to your alternative (define byte as std::byte). I didn't do it for two reasons:

• I am not sure your byte type behaves exactly like std::byte, so it might require changes more complex than what I did, on a codebase I absolutely don't know
• this is not binary compatible (there may be issues if you don't built libcryptopp and the user lib with the same version of the standard. However you might argue that this is usually not adviced (other things might break independantly of std::byte).

[Romain-Geissler-1A]

Here is the kind of errors you have:

misc.h:1118:12: error: no match for ‘operator++’ (operand type is ‘byte {aka std::byte}’)
   carry = !++inout[i];
            ^~~~~~~~~~

misc.h: In function ‘void CryptoPP::IncrementCounterByOne(byte*, const byte*, unsigned int)’:
misc.h:1134:33: error: no match for ‘operator+’ (operand types are ‘const byte {aka const std::byte}’ and ‘int’)
   carry = ((output[i] = input[i]+1) == 0);
                         ~~~~~~~~^~

misc.h:1847:22: error: no match for ‘operator&’ (operand types are ‘byte {aka std::byte}’ and ‘int’)
  value = byte((value & 0xAA) >> 1) | byte((value & 0x55) << 1);
                ~~~~~~^~~~~~

(this one might be solved with a cast of "value" into a "byte")

It appears you do things (like arithmetic) on your "byte" type that is not allowed on "std::byte". So do you still would like to use it ?

[noloader]

@Romain-Geissler-1A,

It appears you do things (like arithmetic) on your "byte" type that is not allowed on "std::byte". So do you still would like to use it ?

Yeah, this is the kind of question we need to find answers for. I think its unnatural to not be able to do math on std::byte. We kind of want them to behave like 8-bit unsigned integers, complete with wrap and overflow. For example, from misc.h : 1110 (which I believe you cited above):

inline void IncrementCounterByOne(byte *inout, unsigned int size)
{
	CRYPTOPP_ASSERT(inout != NULLPTR); CRYPTOPP_ASSERT(size < INT_MAX);
	for (int i=int(size-1), carry=1; i>=0 && carry; i--)
		carry = !++inout[i];
}

In some instances, we depend on the wrap to help with constant time operations.

[noloader]

@Romain-Geissler-1A, @MarcelRaad, @DevJPM, @mouse07410, @denisbider,

We setup a wiki page to explain the upcoming change; see std::byte. I wanted a wiki page for it because we need to tell people how to migrate their code for C++17 and the presence of std::byte.

We have not decided (yet) what the change is. @Romain-Geissler-1A, would you have some time to test moving Crypto++'s byte (global scope) to CryptoPP::byte (namespace scope). Effectively here is the change:

$ cat byte.diff
diff --git a/config.h b/config.h
index 291b148..a4e47c8 100644
--- a/config.h
+++ b/config.h
@@ -194,10 +194,11 @@ namespace CryptoPP { }
 #      define __USE_W32_SOCKETS
 #endif

-typedef unsigned char byte;            // put in global namespace to avoid ambiguity with other byte typedefs
+// typedef unsigned char byte;         // put in global namespace to avoid ambiguity with other byte typedefs

 NAMESPACE_BEGIN(CryptoPP)

+typedef unsigned char byte;
 typedef unsigned short word16;
 typedef unsigned int word32;

diff --git a/kalyna.cpp b/kalyna.cpp
index f4c3cc0..fe448a4 100644
--- a/kalyna.cpp
+++ b/kalyna.cpp
@@ -31,6 +31,7 @@ NAMESPACE_END

 ANONYMOUS_NAMESPACE_BEGIN

+using CryptoPP::byte;
 using CryptoPP::word64;
 using CryptoPP::KalynaTab::T;
 using CryptoPP::KalynaTab::S;

I favor this approach since it seems to be most consistent with Herb Sutters position on namespaces and symbols. Also see Migrating to Namespaces in Dr. Dobbs Journal.

Once you have your test results, I'm going to see where consensus lies and make the change. We will also update the wiki artcle and fill in the values for XXX and YYY:

The compromise to accommodate C++17 std::byte was to XXX. The change occurred at check-in YYY.

[Romain-Geissler-1A]

The option that consist in saying to your users that there code is wrong and shall not use "using namespace std" is a bit extreme. While I definitely agree with this rule and apply it myself in my own code, you shall not tell the others which coding style they should adopt in their own project. Also, I work in a company with thousands of C++ developers, you know how things work in a company: such a change when you have millions and millions of existing C++ file is costly, which you can hardly sell to your client as added value.

For me the sanest long term approach is to migrate ::byte to CryptoPP::byte. Anything declared in a CyrptoPP header shall follow this rule, since it's far too easy to conflict with someone else's names. Yet this has the drawback of being non source compatible, yet still binary compatible for your users (gcc mangles the char type, not the name CryptoPP::byte in function names). Being source incompatible is for example an issue in my company, but this is my own problem, I can handle that.

You might want to ease migration this way (this is usually how we do it in my company): for people using C++17, then they have to migrate their user code from ::byte to CryptoPP::byte. People using C++17 are bleeding edge people, who should have no issue with changing their code. In C++ <= 14 I would declare ::byte as deprecated and advice people to replace ::byte by CryptoPP::byte to prepare for the upcoming C++17. That will break only people using -Werror, but these people are asking for such breakages, so I expect them to migrate their code to fix it. Others will only see deprecations warnings, but their code will still build and run fine.

[noloader]

Thanks again @Romain-Geissler-1A.

The option that consist in saying to your users that there code is wrong and shall not use "using namespace std" is a bit extreme. While I definitely agree with this rule and apply it myself in my own code, you shall not tell the others which coding style they should adopt in their own project. Also, I work in a company with thousands of C++ developers, you know how things work in a company: such a change when you have millions and millions of existing C++ file is costly, which you can hardly sell to your client as added value.

Regarding Paragraph 1, I agree we should avoid setting policy for others (i.e., telling them what they should do). How about if we give them a choice. If CRYPTOPP_BYTE_IS_GLOBAL is defined, then its ::byte (i.e., nothing changes). If CRYPTOPP_BYTE_IN_NAMESPACE is defined, then its CryptoPP::byte (i.e., byte moved to CryptoPP). After a couple of years, we completely remove CRYPTOPP_BYTE_IS_GLOBAL and CRYPTOPP_BYTE_IN_NAMESPACE.

Regarding CRYPTOPP_BYTE_IS_GLOBAL and CRYPTOPP_BYTE_IN_NAMESPACE, maybe we should forgo them and tie it to CRYPTOPP_MAINTAIN_BACKWARDS_COMPATIBILITY. We recently cleared all the former stuff tied to CRYPTOPP_MAINTAIN_BACKWARDS_COMPATIBILITY, so we have a clean slate to work with.

And your observation "... [colliding] with someone else's names" is really the crux of the problem here. We had been running between the cracks by placing byte in the global namespace. Everything was OK if someone else (like user code or another library) either (1) did not provide a typedef for byte; or (2) provided the same typedef for byte. Things worked great for 25 years or so, and then C++17 came along and std::byte completely broke things by changing the semantics of the 8-bit type.

In my mind's eye, "[colliding] with someone else's names" is the root cause of the failure. We should stop running between the cracks, and use namespaces the way they were intended to avoid these collisions.

---

For me the sanest long term approach is to migrate ::byte to CryptoPP::byte. Anything declared in a CyrptoPP header shall follow this rule, since it's far too easy to conflict with someone else's names. Yet this has the drawback of being non source compatible, yet still binary compatible for your users (gcc mangles the char type, not the name CryptoPP::byte in function names). Being source incompatible is for example an issue in my company, but this is my own problem, I can handle that.

Regarding Paragraph 2, I agree. The library proper already works with either choice, modulo fixing Kalyna. We might be able to independently fix Kalyna by adding a typedef unsigned char byte or using an unsigned char* directly in the anonymous namespace.

---

You might want to ease migration this way (this is usually how we do it in my company): for people using C++17, then they have to migrate their user code from ::byte to CryptoPP::byte. People using C++17 are bleeding edge people, who should have no issue with changing their code. In C++ <= 14 I would declare ::byte as deprecated and advice people to replace ::byte by CryptoPP::byte to prepare for the upcoming C++17. That will break only people using -Werror, but these people are asking for such breakages, so I expect them to migrate their code to fix it. Others will only see deprecations warnings, but their code will still build and run fine.

Paragraph 3 has two topics if I am parsing correctly. First is the actual migration or change, and second is a deprecated warning.

Regarding the migration or change, I prefer to make it all or nothing (modulo the choice from Paragraph 1). I gave some thought to using CRYPTOPP_CXX14 (or CRYPTOPP_CXX17) for one definition of byte and CRYPTOPP_CXX17 for another definition of byte. I fear that will lead to breaks as follows: a distro builds with GCC 7, but then users comes along and builds with Clang 3.8.

We see this sort of break more often then expected. It often comes from mixing compilers and/or options. Here was a big pain point in the category of unexpected breaks: GCC5 and the C++11 ABI. A bunch of other ones are listed at GNUmakefile | CXX and CXXFLAGS on the wiki.

Regarding the deprecated warning, I think that path is going to be tricky. We placed a deprecated warning on DefaultEncryptor, DefaultDecryptor (and friends) because they were using 3DES and SHA1, and we had months of list discussions and a couple of bug reports about it. It kept turning up like a bad penny. I cringe to think what a deprecated byte would cause.

To be clear about the issue on the deprecated DefaultEncryptor, DefaultDecryptor (and friends): folks did not complain about moving away from 3DES and SHA. They were effectively reporting the warning back to us :)

---

Our next release is going to be Crypto++ 6.0 (and not Crypto++ 5.6.6), so this is one of those times when we can break an ABI. I don't like the idea of doing it quickly without fair warning, but I don't think its too egregious in this case.

Fedora 26 accelerated things when it shipped a compiler capable of C++17. I'm guessing GCC will be switching from -std=gnu++14 to -std=gnu++17 soon as it's default. We might see it as early as Fedora 27. I'd like to be out ahead of things.

I also pinged László Böszörményi, who is our Debian maintainer and packager. László is hands on, and he often feels the pressure and blow-back of our missteps. I want to get him involved to ensure distros are well represented, too.

[noloader]

Our next release is going to be Crypto++ 6.0 (and not Crypto++ 5.6.6), so this is one of those times when we can break an ABI. I don't like the idea of doing it quickly without fair warning, but I don't think its too egregious in this case.

Regarding this statement, and hindsight being 20/20, the library probably should have done the following sometime between 5.6.3 to 5.6.5. Its too bad we did not have an orbuculum to help us along.

• provided ::byte for compatibility
• added CryptoPP::Byte
• warned ::byte was going away

On the upside, we have the problem reduced to one corner case. The only problem we have not been able to contain or help with is when a user program employs both (1) using namespace std; and (2) using namespace CryptoPP; in the same program.

[gcsideal]

Thanks for the ping @noloader, much appreciated. Indeed, I've bitten by some Crypto++ issues. At the moment I can't upload version 5.6.5 as it breaks the ABI without a soname bump. Still, it has some functionality that the users request to have. I do wait for a new release, let it be 6.0.0 or an other.
On the technical side, Debian has GCC 7.1.0 in its archives, but not yet as default compiler. We are just released our actual stable release, Stretch and busy with its first point release which will happen next week as I know. As such, only a warning was issued until yet, we should test our packages with GCC 7.x as it's going to be the default sometime soon.
I'm open for any testing if required or it helps.

[noloader]

@Romain-Geissler-1A,

Thanks for your early help in identifying the issue, thanks for your help in fixing dlltest.cpp, and thank you very much for the patch.

We checked in a change to address the issue. The change was to move byte into the Crypto++ namespace. The check-in occurred at Commit 00f9818b5d8e5aeb.

The reason we did not use PR 438 was, it did not address the underlying problem of placing byte in a namespace other than the Crypto++ namespace. I believe that was the root cause of the failure here. Put another way, I think your patch addressed a symptom, and not the problem.

We added a wiki page on the matter at std::byte. Under the section Fixing User Programs, we offer your patch as one of the alternatives.

[Romain-Geissler-1A]

Hi,

The reason we did not use PR 438 was, it did not address the underlying problem of placing byte in a namespace other than the Crypto++ namespace. I believe that was the root cause of the failure here. Put another way, I think your patch addressed a symptom, and not the problem.

Agreed.

Thank you for fixing this. Do you have any rough plans for an official release 6.0 ?

[noloader]

@Romain-Geissler-1A,

Thank you for fixing this. Do you have any rough plans for an official release 6.0 ?

I'd like to release in four to six weeks. I think we should wait at least a month to see what happens with the byte change. I can't imagine we need to revisit it, but just in case...

I also need to clear Issues 427 - 430. This is important for distros like Debian and Fedora.

[Romain-Geissler]

Hi,

Do you have any new plans for the release 6.0 ?

Cheers,
Romain

[noloader]

@Romain-Geissler ,

Do you have any new plans for the release 6.0 ?

Two task remain. First is OCB mode, and second is ed25519 signatures.

I'm just about finished OCB mode. I might add SIV mode since they are similar and the changes made to the library for OCB mode accommodate SIV mode, too.

I'm not sure about ed25519 at the moment. I started on it last year but I did not like the way the development was proceeding. I'm thinking it might not make 6.0, but I need a little more time with it before the decision is made. (I really want some safe curve support).