It seems modern compilers don't realize that all invocations of operator""_mst can be evaluated at compile time, despite the constexpr keyword.
Since C++20, we can force them to evaluate at compile time using consteval, turning all the miniscript type constants into actual compile-time constants.
This should give a nice but not very important speedup for miniscript logic, but it's also a way to start testing C++20 features.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For detailed information about the code coverage, see the test coverage report.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
25 | @@ -26,6 +26,12 @@ 26 | #include <util/string.h> 27 | #include <util/vector.h> 28 | 29 | +#if __cplusplus >= 201811L
Maybe wait until next Monday and then remove the if-else macro?
Wow, you're saying the future is next week?
Heh, maybe too optimistic, given that it is still waiting on macOS (https://github.com/bitcoin/bitcoin/pull/28622) and also Windows (see the apparent CI failure here).
and also Windows (see the apparent CI failure here).
For reference:
D:\a\bitcoin\bitcoin\src\script\miniscript.h(160,13): error C2248: 'miniscript::Type::Type': cannot access private member declared in class 'miniscript::Type' [D:\a\bitcoin\bitcoin\build_msvc\libbitcoin_common\libbitcoin_common.vcxproj]
D:\a\bitcoin\bitcoin\src\script\miniscript.h(133,5): message : see declaration of 'miniscript::Type::Type' [D:\a\bitcoin\bitcoin\build_msvc\libbitcoin_common\libbitcoin_common.vcxproj]
D:\a\bitcoin\bitcoin\src\script\miniscript.h(128,7): message : see declaration of 'miniscript::Type' [D:\a\bitcoin\bitcoin\build_msvc\libbitcoin_common\libbitcoin_common.vcxproj]
D:\a\bitcoin\bitcoin\src\script\miniscript.cpp(17,27): message : while evaluating consteval function 'miniscript::operator ""_mst' [D:\a\bitcoin\bitcoin\build_msvc\libbitcoin_common\libbitcoin_common.vcxproj]
D:\a\bitcoin\bitcoin\src\script\miniscript.cpp(234,33): error C7595: 'miniscript::operator ""_mst': call to immediate function is not a constant expression [D:\a\bitcoin\bitcoin\build_msvc\libbitcoin_common\libbitcoin_common.vcxproj]
D:\a\bitcoin\bitcoin\src\script\miniscript.cpp(234,33): fatal error C1003: error count exceeds 100; stopping compilation [D:\a\bitcoin\bitcoin\build_msvc\libbitcoin_common\libbitcoin_common.vcxproj]
Note that the failing Windows CI is also already using MSVCs C++20 mode.
Now it is
miniscript.cpp
D:\a\bitcoin\bitcoin\src\script\miniscript.cpp(234,33): error C7595: 'miniscript::operator ""_mst': call to immediate function is not a constant expression [D:\a\bitcoin\bitcoin\build_msvc\libbitcoin_common\libbitcoin_common.vcxproj]
D:\a\bitcoin\bitcoin\src\script\miniscript.cpp(234,45): error C7595: 'miniscript::operator ""_mst': call to immediate function is not a constant expression [D:\a\bitcoin\bitcoin\build_msvc\libbitcoin_common\libbitcoin_common.vcxproj]
Go home MSVC, you're drunk.
Somehow it thinks that calls to ""_mst are immediate, unless they occur inside ternary operators!?
Yeah, not sure if we are using the latest msvc, and if this is a bug that is fixed in a newer release. cc @hebasto @sipsorcery
Yeah, not sure if we are using the latest msvc, and if this is a bug that is fixed in a newer release. cc @hebasto @sipsorcery
MSBuild version 17.7.2+d6990bcfa for .NET Framework
It is the latest one.
I don't think so; MSVC is only complaining about consteval literals inside ( ? : ) ternaries. I've pushed another attempt to avoid it by moving the subexpressions out of the ternaries.
Ok that appears to have worked.
I don't think so; MSVC is only complaining about consteval literals inside
( ? : )ternaries. I've pushed another attempt to avoid it by moving the subexpressions out of the ternaries.
FWIW, this bug was fixed in VS 17.9.
See https://developercommunity.visualstudio.com/t/Ternary-operator-breaks-consteval/10150913.
Concept ACK. It's annoying that we have to refactor this code just to accomodate MSVC. I agree that we should just wait until branch-off, and use conteval directly. Put this on 27.x.
Rebased.
CI is failing with
/usr/bin/ld: /usr/bin/ld: DWARF error: invalid or unhandled FORM value: 0x23
libbitcoin_common.a(libbitcoin_common_a-descriptor.o): in function `(anonymous namespace)::ParseScript(unsigned int&, Span<char const>&, (anonymous namespace)::ParseScriptContext, FlatSigningProvider&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&)':
descriptor.cpp:(.text+0x390f): undefined reference to `miniscript::operator"" _mst(char const*, unsigned int)'
/usr/bin/ld: descriptor.cpp:(.text+0x396c): undefined reference to `miniscript::operator"" _mst(char const*, unsigned int)'
/usr/bin/ld: descriptor.cpp:(.text+0x39ed): undefined reference to `miniscript::operator"" _mst(char const*, unsigned int)'
/usr/bin/ld: libbitcoin_common.a(libbitcoin_common_a-descriptor.o): in function `std::shared_ptr<miniscript::Node<unsigned int> const> miniscript::internal::Parse<unsigned int, (anonymous namespace)::KeyParser>(Span<char const>, (anonymous namespace)::KeyParser const&)':
descriptor.cpp:(.text+0x1df07): undefined reference to `miniscript::operator"" _mst(char const*, unsigned int)'
/usr/bin/ld: libbitcoin_common.a(libbitcoin_common_a-descriptor.o): in function `(anonymous namespace)::MiniscriptDescriptor::MaxSatisfactionElems() const':
descriptor.cpp:(.text+0x2d316): undefined reference to `miniscript::operator"" _mst(char const*, unsigned int)'
/usr/bin/ld: libbitcoin_common.a(libbitcoin_common_a-descriptor.o):descriptor.cpp:(.text+0x2d333): more undefined references to `miniscript::operator"" _mst(char const*, unsigned int)' follow
clang: error: linker command failed with exit code 1 (use -v to see invocation)
Concept ACK.
Locally, it works, starting with clang++-15.
Confirmed that this cuts a third of the runtime off of the fuzz input from #28832 (comment)
Maybe we should also retry the straight constexpr -> consteval change, without having to do all the refactoring for MSVC? Microsoft might have fixed their compiler.
Could rebase, as CI on master should now be passing with clang-15 in.
Could rebase, as CI on master should now be passing with clang-15 in.
As we now build the fuzz.exe on MSVC, this PR seems require a little adjustment while rebasing:
--- a/src/test/fuzz/miniscript.cpp
+++ b/src/test/fuzz/miniscript.cpp
@@ -391,6 +391,7 @@ std::optional<NodeInfo> ConsumeNodeStable(MsCtx script_ctx, FuzzedDataProvider&
bool allow_K = (type_needed == ""_mst) || (type_needed << "K"_mst);
bool allow_V = (type_needed == ""_mst) || (type_needed << "V"_mst);
bool allow_W = (type_needed == ""_mst) || (type_needed << "W"_mst);
+ static constexpr auto B{"B"_mst}, K{"K"_mst}, V{"V"_mst}, W{"W"_mst};
switch (provider.ConsumeIntegral<uint8_t>()) {
case 0:
@@ -440,22 +441,22 @@ std::optional<NodeInfo> ConsumeNodeStable(MsCtx script_ctx, FuzzedDataProvider&
}
case 11:
if (!(allow_B || allow_K || allow_V)) return {};
- return {{{"B"_mst, type_needed, type_needed}, Fragment::ANDOR}};
+ return {{{B, type_needed, type_needed}, Fragment::ANDOR}};
case 12:
if (!(allow_B || allow_K || allow_V)) return {};
- return {{{"V"_mst, type_needed}, Fragment::AND_V}};
+ return {{{V, type_needed}, Fragment::AND_V}};
case 13:
if (!allow_B) return {};
- return {{{"B"_mst, "W"_mst}, Fragment::AND_B}};
+ return {{{B, W}, Fragment::AND_B}};
case 15:
if (!allow_B) return {};
- return {{{"B"_mst, "W"_mst}, Fragment::OR_B}};
+ return {{{B, W}, Fragment::OR_B}};
case 16:
if (!allow_V) return {};
- return {{{"B"_mst, "V"_mst}, Fragment::OR_C}};
+ return {{{B, V}, Fragment::OR_C}};
case 17:
if (!allow_B) return {};
- return {{{"B"_mst, "B"_mst}, Fragment::OR_D}};
+ return {{{B, B}, Fragment::OR_D}};
case 18:
if (!(allow_B || allow_K || allow_V)) return {};
return {{{type_needed, type_needed}, Fragment::OR_I}};
@@ -472,25 +473,25 @@ std::optional<NodeInfo> ConsumeNodeStable(MsCtx script_ctx, FuzzedDataProvider&
}
case 20:
if (!allow_W) return {};
- return {{{"B"_mst}, Fragment::WRAP_A}};
+ return {{{B}, Fragment::WRAP_A}};
case 21:
if (!allow_W) return {};
- return {{{"B"_mst}, Fragment::WRAP_S}};
+ return {{{B}, Fragment::WRAP_S}};
case 22:
if (!allow_B) return {};
- return {{{"K"_mst}, Fragment::WRAP_C}};
+ return {{{K}, Fragment::WRAP_C}};
case 23:
if (!allow_B) return {};
- return {{{"V"_mst}, Fragment::WRAP_D}};
+ return {{{V}, Fragment::WRAP_D}};
case 24:
if (!allow_V) return {};
- return {{{"B"_mst}, Fragment::WRAP_V}};
+ return {{{B}, Fragment::WRAP_V}};
case 25:
if (!allow_B) return {};
- return {{{"B"_mst}, Fragment::WRAP_J}};
+ return {{{B}, Fragment::WRAP_J}};
case 26:
if (!allow_B) return {};
- return {{{"B"_mst}, Fragment::WRAP_N}};
+ return {{{B}, Fragment::WRAP_N}};
case 27: {
if (!allow_B || !IsTapscript(script_ctx)) return {};
const auto k = provider.ConsumeIntegral<uint16_t>();
ACK 73619b819c37e98bd9a100bea14a3366aebfabb8.
I apologise for messing up.
The #30031, which is built on top of this PR, succeeds. It includes additional code changes in src/test/fuzz/miniscript.cpp (with some inconsistency in local variable naming).
@sipa feel free to pull changes from #30031 into this PR.
It seems modern compilers don't realize that all invocations of operator""_mst
can be evaluated at compile time, despite the constexpr keyword.
Since C++20, we can force them to evaluate at compile time, turning all the
miniscript type constants into actual compile-time constants.
It appears that MSVC does not support consteval operator"" when used inside
certain expressions. For the few places where this happens, define a
constant outside the operator call.
Co-Authored-By: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
re-ACK 63317103c9f2b0635558da814567bb79c17ae851.
utACK 63317103c9f2b0635558da814567bb79c17ae851