Add uint32_t i32 field to Value union #517

chfast · 2020-09-02T22:25:02Z

Problems this introduces

stack.top() = 0 only zeros the .i32 now.
FizzyValue{1} only inits the .i32 part. This is excessively used in C API tests. And cannot be disabled. In C++20 you can do FizzyValue{.i64 = 1}.

chfast · 2020-09-03T09:50:11Z

This ultimately need typed execution API because Result(0) now tests i32 result instead of i64. It is necessary to also check if we are testing correct types.

axic · 2020-10-05T15:48:59Z

What was the blocker on this? It would make the Rust API a tiny bit nicer too.

chfast · 2020-10-05T16:51:47Z

What was the blocker on this? It would make the Rust API a tiny bit nicer too.

Unit tests expectations update.

lib/fizzy/value.hpp

codecov · 2020-11-17T21:44:56Z

Codecov Report

Merging #517 (0fd8f0d) into master (76a3b2a) will decrease coverage by 0.01%.
The diff coverage is 99.29%.

@@            Coverage Diff             @@
##           master     #517      +/-   ##
==========================================
- Coverage   99.31%   99.30%   -0.02%     
==========================================
  Files          72       72              
  Lines       10257    10304      +47     
==========================================
+ Hits        10187    10232      +45     
- Misses         70       72       +2

Flag	Coverage Δ
spectests	`91.49% <100.00%> (-0.01%)`	⬇️
unittests	`99.30% <99.29%> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
test/testfloat/testfloat.cpp	`89.51% <0.00%> (-1.27%)`	⬇️
lib/fizzy/execute.cpp	`100.00% <100.00%> (ø)`
lib/fizzy/instantiate.cpp	`100.00% <100.00%> (ø)`
lib/fizzy/stack.hpp	`100.00% <100.00%> (ø)`
lib/fizzy/value.hpp	`100.00% <100.00%> (ø)`
test/unittests/api_test.cpp	`100.00% <100.00%> (ø)`
test/unittests/capi_test.cpp	`100.00% <100.00%> (ø)`
test/unittests/cxx20_span_test.cpp	`100.00% <100.00%> (ø)`
test/unittests/execute_numeric_test.cpp	`100.00% <100.00%> (ø)`
test/unittests/execute_test.cpp	`100.00% <100.00%> (ø)`
... and 7 more

axic · 2020-11-17T22:01:52Z

This is now blocked on explicitly typing number literals in the tests.

lib/fizzy/value.hpp

CMakeLists.txt

axic · 2020-11-18T20:10:29Z

test/utils/asserts.hpp

@@ -96,7 +97,6 @@ namespace fizzy::test
 {
 inline uint32_t as_uint32(fizzy::Value value)


Eventually we should also remove this, if we merge this PR.

@chfast do you want to remove it in a separate commit, or a separate PR? I can do the removal too.

Well, actually shouldn't all the test code be using as<uint32_t>()? We can apply that parallel to this PR.

Well, actually shouldn't all the test code be using as<uint32_t>()? We can apply that parallel to this PR.

The Result() was checking this to some distinct. But I went with additional type checking to solved it for good.

@chfast do you want to remove it in a separate commit, or a separate PR? I can do the removal too.

In #691.

gumb0 · 2020-11-25T15:11:27Z

lib/fizzy/stack.hpp

@@ -115,7 +115,7 @@ class OperandStack
        m_top = m_bottom - 1;

        const auto local_variables = std::copy_n(args, num_args, m_locals);
-        std::fill_n(local_variables, num_local_variables, 0);
+        std::fill_n(local_variables, num_local_variables, Value{});


Hm, I'm confused by union initialization again. Does default initialization like this set the active member of the union?

I'm confused too. This was only found by GCC coverage build.

The 0 is definitely wrong because it calls the special constructor and inits .i32 only.

But the default constructor should be wrong too because it should only init the first field and .i32 is the first one. This works however for current compilers. I wander if MSan can help here. Would be good if you can check the spec yourself.

One solution to solve this is to provide own implementation of the default constructor which inits .i64 (or define i64 = 0;). But with that the Value looses some properties like "is trivially constructable".

Second solution is to move .i64 to the first position. This guarantees the Value{} to be fully zero-initialized. Same for FizzyValue in C API.

But I don't see any sanity solution to init locals with zero without knowing the type.

I don't believe I'm doing it again to myself, but:

The Value{} is value-initialization.

if T is a class type with a default constructor that is neither user-provided nor deleted (that is, it may be a class with an implicitly-defined or defaulted default constructor), the object is zero-initialized.

Zero-initialization: If T is a union type, the first non-static named data member is zero-initialized and all padding is initialized to zero bits.

So I think the Value{} is correct here as it zeros all object's bytes.

It seems to depend on the interpretation of what is "padding" for a union - whether unused bits of inactive members are "padding". I see some people having doubts about this here: https://stackoverflow.com/questions/11812555/how-to-zero-initialize-an-union#comment15704643_11812606 I have not found a definite answer to this yet.

However, I had a concern about something else (perhaps not directly related to this PR): we zero-initialize Values of locals here, which might or might not choose the active member of the union (I have not found the answer to this either). Then function may access them right away with local.get, then some other operation may access some member of it - can be any type member. Then this may or may not be UB.

We can add a test for this, but as I remember sanitizers didn't catch union type punning.

And if we're required to set the right type, then there's no way around needing the types of locals when initializing.

We can add a test for this, but as I remember sanitizers didn't catch union type punning.

Well, the tests you added here (execute.local_init) already check this actually. Because checking the result of a function reads some specific member of Value union.

It seems to depend on the interpretation of what is "padding" for a union - whether unused bits of inactive members are "padding". I see some people having doubts about this here: https://stackoverflow.com/questions/11812555/how-to-zero-initialize-an-union#comment15704643_11812606 I have not found a definite answer to this yet.

Agree. This definition issues would be nice to clear.

However, I had a concern about something else (perhaps not directly related to this PR): we zero-initialize Values of locals here, which might or might not choose the active member of the union (I have not found the answer to this either).

"the object's first non-static named data member is zero initialized and padding is initialized to zero bits"

This indicates that the first member is initialized then the active one.

Then function may access them right away with local.get, then some other operation may access some member of it - can be any type member. Then this may or may not be UB.

We can add a test for this, but as I remember sanitizers didn't catch union type punning.

This is true. I have found multiple notes that GCC explicitly supports union-based type punning as some kind of C++ extension. But I cannot find it in documentation.
It is also said that Clang's UBSan should report all UB's Clang recognizes (otherwise it is a compiler bug), so this can indicate this is also supported in Clang.

And if we're required to set the right type, then there's no way around needing the types of locals when initializing.

That's would be very pedantic way of handling this. I would stay with investigating C++ standard and compiler documentation + have many tests for this.

Another alternative to investigate is to use std::memset() as a way of fixing theoretical UB (similarly to std::memcpy() being a (the only) correct way of doing type punning).

I did some experiments and it looks like GCC and MSVC inits the whole object while Clang only the first element: https://godbolt.org/z/cWscY8.

See also: https://stackoverflow.com/questions/37226837/union-zero-initialization-with-clang-vs-gcc

This may be a good reason to make i64 the first member. Are there any advantages for it to be the second other than aesthetical?

Just esthetics. I'm keeping it though for now to catch issues and add regression tests.

lib/fizzy/execute.cpp

test/unittests/api_test.cpp

test/unittests/capi_test.cpp

test/unittests/execute_numeric_test.cpp

chfast · 2021-01-11T13:26:19Z

I don't see any significant speed difference except for noise. The GCC LTO build is much noisier.

fizzy/execute/blake2b/512_bytes_rounds_1_mean                     +0.0095         +0.0095            80            81            80            81
fizzy/execute/blake2b/512_bytes_rounds_16_mean                    +0.0085         +0.0085          1207          1217          1207          1217
fizzy/execute/ecpairing/onepoint_mean                             +0.0072         +0.0072        404734        407646        404736        407648
fizzy/execute/keccak256/512_bytes_rounds_1_mean                   +0.0201         +0.0201           101           103           101           103
fizzy/execute/keccak256/512_bytes_rounds_16_mean                  +0.0131         +0.0131          1489          1508          1489          1508
fizzy/execute/memset/256_bytes_mean                               -0.0167         -0.0167             7             7             7             7
fizzy/execute/memset/60000_bytes_mean                             -0.0209         -0.0209          1539          1507          1539          1507
fizzy/execute/mul256_opt0/input1_mean                             +0.0347         +0.0347            29            30            29            30
fizzy/execute/ramanujan_pi/33_runs_mean                           -0.0274         -0.0274           114           111           114           111
fizzy/execute/sha1/512_bytes_rounds_1_mean                        +0.0303         +0.0303            88            91            88            91
fizzy/execute/sha1/512_bytes_rounds_16_mean                       +0.0334         +0.0334          1225          1266          1225          1266
fizzy/execute/sha256/512_bytes_rounds_1_mean                      +0.0075         +0.0075            80            81            80            81
fizzy/execute/sha256/512_bytes_rounds_16_mean                     +0.0087         +0.0087          1101          1111          1101          1111
fizzy/execute/taylor_pi/pi_1000000_runs_mean                      -0.0011         -0.0011         40619         40573         40620         40573
fizzy/execute/micro/eli_interpreter/exec105_mean                  -0.0150         -0.0150             5             4             5             4
fizzy/execute/micro/factorial/20_mean                             +0.0207         +0.0206             1             1             1             1
fizzy/execute/micro/fibonacci/24_mean                             +0.0175         +0.0175          5048          5137          5048          5137
fizzy/execute/micro/host_adler32/1_mean                           +0.0211         +0.0211             0             0             0             0
fizzy/execute/micro/host_adler32/1000_mean                        -0.0042         -0.0042            31            31            31            31
fizzy/execute/micro/icall_hash/1000_steps_mean                    +0.0033         +0.0033            69            69            69            69
fizzy/execute/micro/spinner/1_mean                                -0.0603         -0.0603             0             0             0             0
fizzy/execute/micro/spinner/1000_mean                             -0.0134         -0.0134             9             9             9             9

test/unittests/value_test.cpp

gumb0

Looks fine to me

axic · 2021-01-19T12:07:53Z

test/unittests/api_test.cpp

@@ -47,7 +47,7 @@ TEST(api, execution_result_value)
    const ExecutionResult result = Value{1234_u32};
    EXPECT_FALSE(result.trapped);
    EXPECT_TRUE(result.has_value);
-    EXPECT_EQ(result.value.i64, 1234_u32);
+    EXPECT_EQ(result.value.i32, 1234_u32);


Notpick, we do not actually need _u32 here, right?

No. But using _u32 gives higher confidence (i.e. someone has thought what the result type is expected). Still gray area though.

test/utils/fizzy_c_engine.cpp

include/fizzy/fizzy.h

This allows using i32 values directly what is less confusing than previous emulation with storing these as uint64_t.

chfast force-pushed the value_i32 branch from c52ead7 to b54d8a1 Compare September 3, 2020 09:15

axic force-pushed the value_i32 branch from b54d8a1 to 10fff7b Compare October 7, 2020 10:30

axic force-pushed the value_i32 branch from 10fff7b to 96afb93 Compare October 19, 2020 13:41

axic reviewed Oct 19, 2020

View reviewed changes

lib/fizzy/value.hpp Show resolved Hide resolved

axic force-pushed the value_i32 branch from 96afb93 to 4900d0f Compare October 19, 2020 13:44

axic force-pushed the value_i32 branch from 4900d0f to 687a6c3 Compare October 28, 2020 15:17

chfast mentioned this pull request Nov 17, 2020

test: Add additional i64_extend_i32_u test #648

Merged

axic force-pushed the value_i32 branch from 687a6c3 to ba0e42d Compare November 17, 2020 21:15

chfast force-pushed the value_i32 branch from 2d1034e to 6b59700 Compare November 18, 2020 16:07

axic reviewed Nov 18, 2020

View reviewed changes

lib/fizzy/value.hpp Outdated Show resolved Hide resolved

axic reviewed Nov 18, 2020

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

axic reviewed Nov 18, 2020

View reviewed changes

chfast force-pushed the value_i32 branch from 6b59700 to 811dece Compare November 23, 2020 13:31

chfast changed the base branch from master to test_typed_result November 23, 2020 13:32

chfast force-pushed the value_i32 branch 4 times, most recently from 1cfb307 to bf0c745 Compare November 23, 2020 16:39

chfast marked this pull request as ready for review November 24, 2020 12:30

chfast requested review from gumb0 and axic November 24, 2020 12:30

gumb0 reviewed Nov 25, 2020

View reviewed changes

lib/fizzy/execute.cpp Outdated Show resolved Hide resolved

axic reviewed Jan 5, 2021

View reviewed changes

test/unittests/api_test.cpp Show resolved Hide resolved

axic reviewed Jan 5, 2021

View reviewed changes

test/unittests/capi_test.cpp Outdated Show resolved Hide resolved

axic reviewed Jan 5, 2021

View reviewed changes

test/unittests/execute_numeric_test.cpp Show resolved Hide resolved

chfast force-pushed the value_i32 branch 2 times, most recently from 4678dff to 338d46c Compare January 11, 2021 10:24

gumb0 reviewed Jan 12, 2021

View reviewed changes

test/unittests/value_test.cpp Outdated Show resolved Hide resolved

gumb0 reviewed Jan 12, 2021

View reviewed changes

test/unittests/value_test.cpp Show resolved Hide resolved

chfast force-pushed the value_i32 branch from f45c994 to 602278e Compare January 12, 2021 16:03

gumb0 approved these changes Jan 13, 2021

View reviewed changes

chfast force-pushed the value_i32 branch 2 times, most recently from 52786d4 to 37a3470 Compare January 13, 2021 14:34

axic reviewed Jan 19, 2021

View reviewed changes

test/utils/fizzy_c_engine.cpp Show resolved Hide resolved

axic approved these changes Jan 19, 2021

View reviewed changes

axic mentioned this pull request Jan 19, 2021

test: Drop as_uint32() #691

Merged

chfast force-pushed the value_i32 branch from 37a3470 to 8957aab Compare January 20, 2021 10:45

axic approved these changes Jan 20, 2021

View reviewed changes

axic reviewed Jan 20, 2021

View reviewed changes

include/fizzy/fizzy.h Outdated Show resolved Hide resolved

chfast added 2 commits January 20, 2021 12:35

Add uint32_t i32 field to Value union

1f6d589

This allows using i32 values directly what is less confusing than previous emulation with storing these as uint64_t.

test: Assert against FP result types in Fizzy engines

0fd8f0d

chfast force-pushed the value_i32 branch from 8957aab to 0fd8f0d Compare January 20, 2021 11:36

gumb0 approved these changes Jan 20, 2021

View reviewed changes

axic changed the title ~~Value i32~~ Add uint32_t i32 field to Value union Jan 20, 2021

axic merged commit 470bd08 into master Jan 20, 2021

axic deleted the value_i32 branch January 20, 2021 12:41

axic mentioned this pull request Jan 28, 2021

rust: use i32 type in Value #702

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add uint32_t i32 field to Value union #517

Add uint32_t i32 field to Value union #517

chfast commented Sep 2, 2020 •

edited

Loading

chfast commented Sep 3, 2020

axic commented Oct 5, 2020

chfast commented Oct 5, 2020

codecov bot commented Nov 17, 2020 •

edited

Loading

axic commented Nov 17, 2020

axic Nov 18, 2020

axic Jan 5, 2021

axic Jan 5, 2021

chfast Jan 13, 2021

chfast Jan 13, 2021

gumb0 Nov 25, 2020

chfast Nov 25, 2020

chfast Nov 25, 2020

gumb0 Nov 30, 2020 •

edited

Loading

gumb0 Nov 30, 2020

chfast Nov 30, 2020

chfast Dec 1, 2020 •

edited

Loading

gumb0 Dec 1, 2020

chfast Jan 5, 2021

chfast commented Jan 11, 2021

gumb0 left a comment

axic Jan 19, 2021

chfast Jan 20, 2021

		@@ -96,7 +97,6 @@ namespace fizzy::test
		{
		inline uint32_t as_uint32(fizzy::Value value)

Add uint32_t i32 field to Value union #517

Add uint32_t i32 field to Value union #517

Conversation

chfast commented Sep 2, 2020 • edited Loading

chfast commented Sep 3, 2020

axic commented Oct 5, 2020

chfast commented Oct 5, 2020

codecov bot commented Nov 17, 2020 • edited Loading

Codecov Report

axic commented Nov 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gumb0 Nov 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chfast Dec 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chfast commented Jan 11, 2021

gumb0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chfast commented Sep 2, 2020 •

edited

Loading

codecov bot commented Nov 17, 2020 •

edited

Loading

gumb0 Nov 30, 2020 •

edited

Loading

chfast Dec 1, 2020 •

edited

Loading