Minimal code changes to allow msvc compilation #11558

sipsorcery · 2017-10-25T12:48:42Z

These changes are required to allow the Bitcoin source to build with Microsoft's C++ compiler (#11562 is also required).

I looked around for a better place for the typedef of ssize_t which is in random.h. The best candidate looks like src/compat.h but I figured including that header in random.h is a bigger change than the typedef. Note that the same typedef is in at least two other places including the OpenSSL and Berkeley DB headers so some of the Bitcoin code already picks it up.

theuni · 2017-10-25T15:50:44Z

src/bench/bench.cpp

+    tp->tv_usec = (long)(system_time.wMilliseconds * 1000);
+    return 0;
+}
+#endif


Let's just drop gettimedouble() and switch this to std::chrono::steady_clock. That should eliminate the non-portable code.

theuni · 2017-10-25T15:51:53Z

src/random.h

+typedef int32_t ssize_t;
+#endif
+#endif
+


I think just making MAX_TRIES an int should remove the need for this?

I think just making MAX_TRIES an int should remove the need for this?

Yes this would be better than adding a top-level definition that could conflict with other libraries. (Also, if we did need a top-level definition, compat.h would probably be a better place to put it.)

@ryanofsky I agree it's very clunky to add yet another ssize_t typedef. I did consider adding to compat.h, see the main PR comment. Happy to go with whatever the consensus is.

Happy to go with whatever the consensus is.

I think you should just make MAX_TRIES an int like Cory suggested.

theuni · 2017-10-25T16:03:56Z

src/support/cleanse.cpp

@@ -32,7 +36,7 @@ void memory_cleanse(void *ptr, size_t len)
       might try to eliminate "superfluous" memsets. If there's an easy way to
       detect memset_s, it would be better to use that. */
 #if defined(_MSC_VER)
-    __asm;


See the discussion here #11196 for context here. Now would be a good time to reassess.

Note that __asm is not supported with msvc when targeting x64, that's the only reason for this change.

theuni · 2017-10-25T16:08:02Z

The rest of the changes look good to me.

ryanofsky

Looks good if ssize_t, gettimeofday, and tab character comments are addressed.

ryanofsky · 2017-10-25T18:31:52Z

src/random.h

+typedef int32_t ssize_t;
+#endif
+#endif
+


I think just making MAX_TRIES an int should remove the need for this?

Yes this would be better than adding a top-level definition that could conflict with other libraries. (Also, if we did need a top-level definition, compat.h would probably be a better place to put it.)

ryanofsky · 2017-10-25T18:34:19Z

src/support/cleanse.cpp

@@ -7,6 +7,10 @@

 #include <cstring>

+#if defined(_MSC_VER)
+#include <Windows.h>	// For SecureZeroMemory.


Need to get replace the tab on this line with spaces to fix travis error.

theuni · 2017-10-25T21:10:43Z

See #11562, which should eliminate the need for the gettimeofday() patch.

ryanofsky

utACK a80e0c591b5b56111bdb1d42ff0449dede61810b. Seems fine now without gettimeofday implementation. Would be nice to get rid of ssize_t too.

ryanofsky · 2017-10-31T20:08:12Z

src/random.h

+typedef int32_t ssize_t;
+#endif
+#endif
+


Happy to go with whatever the consensus is.

I think you should just make MAX_TRIES an int like Cory suggested.

sipsorcery · 2017-11-01T10:36:19Z

Working around the use of the non-portable ssize_t definition is proving to be a bit tricky. Adding a typedef to random.h allows the code to build but as some reviewers pointed out it was a clunky fix.

Switching from ssize_t to int in random.h and random.cpp is not sufficient. There are also multiple uses of ssize_t in netbase.cpp and test/util_tests.cpp. A side effect of the definition in random.h was that those two classes picked up the definition.

Adding a typedef to compat.h and also adding an include for compat.h to random.h gets the definition to the missing spots BUT it introduces a re-definition compilation error because the db.h header from BerkeleyDB already has the same typedef.

After some gymnastics I have managed to come up with a set of directives that allow successful compilation, see below, however if portable code is a goal for the future it may be worth steering away from non-C++ standard types, such as ssize_t, outside of any platform specific classes?

#ifdef _MSC_VER
#if !defined(ssize_t)
#ifdef _WIN64
typedef int64_t ssize_t;
#else
typedef int32_t ssize_t;
#endif
#endif
#endif

24a0bdd bench: prefer a steady clock if the resolution is no worse (Cory Fields) c515d26 bench: switch to std::chrono for time measurements (Cory Fields) Pull request description: gettimeofday has portability issues, see for example #11558. Regardless of large-scale clock refactors in the future, I think it's fine for bench to just use std::chrono itself. Note that this may slightly improve bench accuracy and changes the display from tiny floats to nanosecond counts instead. Tree-SHA512: 122355456d01ec6cfcf6867991715cf3a95eabbf5a4f2adc26a059b50382ffb318b7639cdd575197fc4ee5be8b967c0404f1f920d6f5bd4ddd0bd63b5e5c5632

laanwj · 2017-11-09T17:04:21Z

however if portable code is a goal for the future it may be worth steering away from non-C++ standard types, such as ssize_t, outside of any platform specific classes?

Agree, ssize_t should be used in POSIX code, as some of the POSIX functions such as read() return it. But outside it it's better not to.

utACK, changes look sane (or at least harmless) to me now. af82415

ryanofsky · 2017-11-09T17:13:44Z


EDIT: Never mind. I see there were other uses of ssize_t outside of random.h/cpp. Still would be good to stop using ssize_t in inappropriate places.

theuni · 2017-11-09T18:54:20Z

src/random.h

@@ -9,6 +9,7 @@
 #include "crypto/chacha20.h"
 #include "crypto/common.h"
 #include "uint256.h"
+#include "compat.h"


Please just make NUM_OS_RANDOM_BYTES an int to keep the compat.h include out of this header. I realize it's still needed in the cpp, but let's try not to spread the use further than necessary.

fbf327b Minimal code changes to allow msvc compilation. (Aaron Clauson) Pull request description: These changes are required to allow the Bitcoin source to build with Microsoft's C++ compiler (#11562 is also required). I looked around for a better place for the typedef of ssize_t which is in random.h. The best candidate looks like src/compat.h but I figured including that header in random.h is a bigger change than the typedef. Note that the same typedef is in at least two other places including the OpenSSL and Berkeley DB headers so some of the Bitcoin code already picks it up. Tree-SHA512: aa6cc6283015e08ab074641f9abdc116c4dc58574dc90f75e7a5af4cc82946d3052370e5cbe855fb6180c00f8dc66997d3724ff0412e4b7417e51b6602154825

maflcko · 2017-12-13T15:04:55Z

Post merge Concept ACK fbf327b

ef7beae Visual Studio build configuration for Bitcoin Core (Aaron Clauson) Pull request description: This PR allows Bitcoin Core to be relatively easily built with Visual Studio 2017. It's anticipated that it could be useful for devs familiar with Visual Studio and Microsoft's tooling. In particular the ability to use the VS debugger is a big benefit. ~~Caveats:~~ - ~~There are some minor code changes required on Bitcoin Core in order for msvc to be able to successfully compile. I'll submit them in a separate PR, The code changes are available in bitcoin#11528 bitcoin#11558 and bitcoin#11562~~. - ~~The vcpkg for SECP256K1 has not yet been accepted by Microsoft. The files are available from this [PR](microsoft/vcpkg#2005) and should be copied into a vcpkg/ports/secp256k1 directory prior to vcpkg install steps.~~ **Update:** For anyone wishing to test out the Visual Studio build with the various open pull requests the steps are: - Clone and build [Vcpkg](https://github.com/Microsoft/vcpkg) (Microsoft's new open source C/C++ package manager) - git clone https://github.com/Microsoft/vcpkg - .\bootstrap-vcpkg.bat - Set up Visual Studio to automatically reference vcpkg installs: .\vcpkg integrate install - Install the required packages (replace x86 with x64 as required): - vcpkg install boost:x86-windows-static - vcpkg install libevent:x86-windows-static - vcpkg install openssl:x86-windows-static - vcpkg install zeromq:x86-windows-static - vcpkg install berkeleydb:x86-windows-static - vcpkg install secp256k1:x86-windows-static - vcpkg install leveldb:x86-windows-static - git clone https://github.com/bitcoin/bitcoin.git - git checkout -b testbuild - git pull origin pull/11526/head # Visual Studio build configuration for Bitcoin Core - ~~git pull origin pull/11558/head # Minimal code changes to allow msvc compilation~~ - ~~git pull origin pull/11562/head # bench: use std::chrono rather than gettimeofday~~ - ~~Copy and unzip attached bitcoin-config.h to src/config, edit as required [bitcoin-config.zip](https://github.com/bitcoin/bitcoin/files/1429484/bitcoin-config.zip)~~ - ~~git pull origin pull/13031/head # gmtime fix for msvc~~ - Build the Visual Studio solution which, if successful, will result in all but the Qt dependent libraries/programs being built. If the build fails please add a comment. Tree-SHA512: 5cd17273d33a09c35d8534c9f49123dec60ec05383669c67674b2cac88ada177bf94d7731c2a827759444f18d4b67085b91b02458124d0c32ab3a8f72ba5dac9

24a0bdd bench: prefer a steady clock if the resolution is no worse (Cory Fields) c515d26 bench: switch to std::chrono for time measurements (Cory Fields) Pull request description: gettimeofday has portability issues, see for example bitcoin#11558. Regardless of large-scale clock refactors in the future, I think it's fine for bench to just use std::chrono itself. Note that this may slightly improve bench accuracy and changes the display from tiny floats to nanosecond counts instead. Tree-SHA512: 122355456d01ec6cfcf6867991715cf3a95eabbf5a4f2adc26a059b50382ffb318b7639cdd575197fc4ee5be8b967c0404f1f920d6f5bd4ddd0bd63b5e5c5632

fbf327b Minimal code changes to allow msvc compilation. (Aaron Clauson) Pull request description: These changes are required to allow the Bitcoin source to build with Microsoft's C++ compiler (bitcoin#11562 is also required). I looked around for a better place for the typedef of ssize_t which is in random.h. The best candidate looks like src/compat.h but I figured including that header in random.h is a bigger change than the typedef. Note that the same typedef is in at least two other places including the OpenSSL and Berkeley DB headers so some of the Bitcoin code already picks it up. Tree-SHA512: aa6cc6283015e08ab074641f9abdc116c4dc58574dc90f75e7a5af4cc82946d3052370e5cbe855fb6180c00f8dc66997d3724ff0412e4b7417e51b6602154825

Memory cleanse backports Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#10308 - bitcoin/bitcoin#11196 - bitcoin/bitcoin#11558 - Only the changes that did not conflict. - bitcoin/bitcoin#16158 Part of #145.

memory_cleanse backports Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#10308 - bitcoin/bitcoin#11196 - bitcoin/bitcoin#11558 - Only the changes that did not conflict. - bitcoin/bitcoin#16158 Part of #145.

Summary: fbf327b Minimal code changes to allow msvc compilation. (Aaron Clauson) Pull request description: These changes are required to allow the Bitcoin source to build with Microsoft's C++ compiler (#11562 is also required). I looked around for a better place for the typedef of ssize_t which is in random.h. The best candidate looks like src/compat.h but I figured including that header in random.h is a bigger change than the typedef. Note that the same typedef is in at least two other places including the OpenSSL and Berkeley DB headers so some of the Bitcoin code already picks it up. --- Backport of Core [[bitcoin/bitcoin#11558 | PR11558]] Test Plan: ninja all check check-functional Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8379

ef7beae Visual Studio build configuration for Bitcoin Core (Aaron Clauson) Pull request description: This PR allows Bitcoin Core to be relatively easily built with Visual Studio 2017. It's anticipated that it could be useful for devs familiar with Visual Studio and Microsoft's tooling. In particular the ability to use the VS debugger is a big benefit. ~~Caveats:~~ - ~~There are some minor code changes required on Bitcoin Core in order for msvc to be able to successfully compile. I'll submit them in a separate PR, The code changes are available in bitcoin#11528 bitcoin#11558 and bitcoin#11562~~. - ~~The vcpkg for SECP256K1 has not yet been accepted by Microsoft. The files are available from this [PR](microsoft/vcpkg#2005) and should be copied into a vcpkg/ports/secp256k1 directory prior to vcpkg install steps.~~ **Update:** For anyone wishing to test out the Visual Studio build with the various open pull requests the steps are: - Clone and build [Vcpkg](https://github.com/Microsoft/vcpkg) (Microsoft's new open source C/C++ package manager) - git clone https://github.com/Microsoft/vcpkg - .\bootstrap-vcpkg.bat - Set up Visual Studio to automatically reference vcpkg installs: .\vcpkg integrate install - Install the required packages (replace x86 with x64 as required): - vcpkg install boost:x86-windows-static - vcpkg install libevent:x86-windows-static - vcpkg install openssl:x86-windows-static - vcpkg install zeromq:x86-windows-static - vcpkg install berkeleydb:x86-windows-static - vcpkg install secp256k1:x86-windows-static - vcpkg install leveldb:x86-windows-static - git clone https://github.com/bitcoin/bitcoin.git - git checkout -b testbuild - git pull origin pull/11526/head # Visual Studio build configuration for Bitcoin Core - ~~git pull origin pull/11558/head # Minimal code changes to allow msvc compilation~~ - ~~git pull origin pull/11562/head # bench: use std::chrono rather than gettimeofday~~ - ~~Copy and unzip attached bitcoin-config.h to src/config, edit as required [bitcoin-config.zip](https://github.com/bitcoin/bitcoin/files/1429484/bitcoin-config.zip)~~ - ~~git pull origin pull/13031/head # gmtime fix for msvc~~ - Build the Visual Studio solution which, if successful, will result in all but the Qt dependent libraries/programs being built. If the build fails please add a comment. Tree-SHA512: 5cd17273d33a09c35d8534c9f49123dec60ec05383669c67674b2cac88ada177bf94d7731c2a827759444f18d4b67085b91b02458124d0c32ab3a8f72ba5dac9

24a0bdd bench: prefer a steady clock if the resolution is no worse (Cory Fields) c515d26 bench: switch to std::chrono for time measurements (Cory Fields) Pull request description: gettimeofday has portability issues, see for example bitcoin#11558. Regardless of large-scale clock refactors in the future, I think it's fine for bench to just use std::chrono itself. Note that this may slightly improve bench accuracy and changes the display from tiny floats to nanosecond counts instead. Tree-SHA512: 122355456d01ec6cfcf6867991715cf3a95eabbf5a4f2adc26a059b50382ffb318b7639cdd575197fc4ee5be8b967c0404f1f920d6f5bd4ddd0bd63b5e5c5632

ef7beae Visual Studio build configuration for Bitcoin Core (Aaron Clauson) Pull request description: This PR allows Bitcoin Core to be relatively easily built with Visual Studio 2017. It's anticipated that it could be useful for devs familiar with Visual Studio and Microsoft's tooling. In particular the ability to use the VS debugger is a big benefit. ~~Caveats:~~ - ~~There are some minor code changes required on Bitcoin Core in order for msvc to be able to successfully compile. I'll submit them in a separate PR, The code changes are available in bitcoin#11528 bitcoin#11558 and bitcoin#11562~~. - ~~The vcpkg for SECP256K1 has not yet been accepted by Microsoft. The files are available from this [PR](microsoft/vcpkg#2005) and should be copied into a vcpkg/ports/secp256k1 directory prior to vcpkg install steps.~~ **Update:** For anyone wishing to test out the Visual Studio build with the various open pull requests the steps are: - Clone and build [Vcpkg](https://github.com/Microsoft/vcpkg) (Microsoft's new open source C/C++ package manager) - git clone https://github.com/Microsoft/vcpkg - .\bootstrap-vcpkg.bat - Set up Visual Studio to automatically reference vcpkg installs: .\vcpkg integrate install - Install the required packages (replace x86 with x64 as required): - vcpkg install boost:x86-windows-static - vcpkg install libevent:x86-windows-static - vcpkg install openssl:x86-windows-static - vcpkg install zeromq:x86-windows-static - vcpkg install berkeleydb:x86-windows-static - vcpkg install secp256k1:x86-windows-static - vcpkg install leveldb:x86-windows-static - git clone https://github.com/bitcoin/bitcoin.git - git checkout -b testbuild - git pull origin pull/11526/head # Visual Studio build configuration for Bitcoin Core - ~~git pull origin pull/11558/head # Minimal code changes to allow msvc compilation~~ - ~~git pull origin pull/11562/head # bench: use std::chrono rather than gettimeofday~~ - ~~Copy and unzip attached bitcoin-config.h to src/config, edit as required [bitcoin-config.zip](https://github.com/bitcoin/bitcoin/files/1429484/bitcoin-config.zip)~~ - ~~git pull origin pull/13031/head # gmtime fix for msvc~~ - Build the Visual Studio solution which, if successful, will result in all but the Qt dependent libraries/programs being built. If the build fails please add a comment. Tree-SHA512: 5cd17273d33a09c35d8534c9f49123dec60ec05383669c67674b2cac88ada177bf94d7731c2a827759444f18d4b67085b91b02458124d0c32ab3a8f72ba5dac9

…up docs f53a70c Improve documentation of memory_cleanse() (Tim Ruffing) cac30a4 Clean up logic in memory_cleanse() for MSVC (Tim Ruffing) Pull request description: When working on bitcoin-core/secp256k1#185, I noticed that the logic in memory_cleanse(), which is supposed to clear memory securely, is weird on MSVC. While it's correct, it's at least a code smell because the code clears the memory twice on MSVC. This weirdness was introduced by bitcoin#11558. This PR fixes the logic on MSVC and also improves the docs around this function. Best reviewed in individual commits, see the commit messages for more rationale. The second commit touches only comments. ACKs for top commit: practicalswift: utACK f53a70c :-) laanwj: code review ACK f53a70c Tree-SHA512: 1c2fd98ae62b34b3e6e59d1178b293af969a9e06cbb7df02a699ce8802f145a336f72edb178c520e3ecec81f7e8083828f90a5ba6367d966a2c7d7c0dd6c0475