Reliably serialize NaN values.
AbandonedPublic
Actions

Authored by wraitii on Dec 10 2020, 5:52 PM.

Details

Reviewers: None
Trac Tickets: #1879

Summary

NaN values can take a number of different balues depending on the architecture and how they came to be. This can OOS spuriously.

This does two things:

make the float/double 'put' functions protected, so they can't be called accidentally from sim code, potentially preventing issues.
make the script-serializer detect NaN and serialize them specially. I add a boolean for whether the nan is the Number Object or just a primitive number, since that seems more straightforward.

Test Plan

Serialize NaN values.

Diff Detail

Repository

rP 0 A.D. Public Repository

Branch

temp

Lint

Lint OK

Unit

No Unit Test Coverage

Build Status

Buildable 14280
Build 30061: Vulcan Build	Jenkins
Build 30060: Vulcan Build (macOS)	Jenkins
Build 30059: Vulcan Build (Windows)	Jenkins
Build 30058: arc lint + arc unit

Unit TestsFailed
View All

	Time	Test
	0 ms	Jenkins > cxxtest_debug.xml::[failed-to-read] Failed to read test report file E:\Jenkins\workspace\vs2015-differential\cxxtest_debug.xml org.dom4j.DocumentException: Error on line 347 of document : Content is not allowed in trailing section. at org.dom4j.io.SAXReader.read(SAXReader.java:511)
	0 ms	Jenkins > TestAllocators::test_da
	0 ms	Jenkins > TestAllocators::test_da
	0 ms	Jenkins > TestAllocators::test_da
	0 ms	Jenkins > TestAtlasObjectXML::test_parse_attributes1
		View Full Test Results (1 Failed · 1,021 Passed)

Event Timeline

wraitii created this revision.Dec 10 2020, 5:52 PM

Build failure - The Moirai have given mortals hearts that can endure.

builderr-debug-macos.txt
fatal error: file '/Users/wfg/Jenkins/workspace/macos-differential/build/workspaces/gcc/../../../libraries/source/spidermonkey/include-unix-debug/mozilla/LinkedList.h' has been modified since the precompiled header 'obj/network_Debug/precompiled.h.gch' was built
note: please rebuild precompiled header 'obj/network_Debug/precompiled.h.gch'
1 error generated.
make[1]: *** [obj/network_Debug/NetMessageSim.o] Error 1
make: *** [network] Error 2

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/2375/display/redirect

Successful build - Chance fights ever on the side of the prudent.

builderr-release-gcc7.txt
In file included from ../../../source/pch/atlas/precompiled.h:26:
../../../source/tools/atlas/GameInterface/Messages.h: In function 'void AtlasMessage::fGetTerrainGroupPreviews(AtlasMessage::qGetTerrainGroupPreviews*)':
../../../source/tools/atlas/GameInterface/Messages.h:315:8: warning: '#'target_mem_ref' not supported by dump_expr#<expression error>' may be used uninitialized in this function [-Wmaybe-uninitialized]
 struct sTerrainTexturePreview
        ^~~~~~~~~~~~~~~~~~~~~~
../../../source/tools/atlas/GameInterface/Messages.h:315:8: warning: '#'target_mem_ref' not supported by dump_expr#<expression error>' may be used uninitialized in this function [-Wmaybe-uninitialized]
../../../source/tools/atlas/GameInterface/Messages.h:315:8: warning: '#'target_mem_ref' not supported by dump_expr#<expression error>' may be used uninitialized in this function [-Wmaybe-uninitialized]
../../../source/tools/atlas/GameInterface/Messages.h:315:8: warning: '#'target_mem_ref' not supported by dump_expr#<expression error>' may be used uninitialized in this function [-Wmaybe-uninitialized]
 struct sTerrainTexturePreview
        ^~~~~~~~~~~~~~~~~~~~~~
../../../source/tools/atlas/GameInterface/Messages.h:315:8: warning: '#'target_mem_ref' not supported by dump_expr#<expression error>' may be used uninitialized in this function [-Wmaybe-uninitialized]
../../../source/tools/atlas/GameInterface/Messages.h:315:8: warning: '#'target_mem_ref' not supported by dump_expr#<expression error>' may be used uninitialized in this function [-Wmaybe-uninitialized]
In file included from ../../../source/lib/os_path.h:26,
                 from ../../../source/lib/self_test.h:28,
                 from ../../../source/simulation2/system/ComponentTest.h:21,
                 from ../../../source/pch/test/precompiled.h:22:
../../../source/lib/path.h: In member function 'virtual void TestMapGenerator::setUp()':
../../../source/lib/path.h:265:68: warning: '<anonymous>.Path::separator' may be used uninitialized in this function [-Wmaybe-uninitialized]
   debug_printf("Path %s, separator %c\n", string8().c_str(), (char)separator);
                                                                    ^~~~~~~~~
In file included from ../../../source/lib/os_path.h:26,
                 from ../../../source/lib/self_test.h:28,
                 from ../../../source/simulation2/system/ComponentTest.h:21,
                 from ../../../source/pch/test/precompiled.h:22:
../../../source/lib/path.h: In member function 'virtual void TestComponentScripts::setUp()':
../../../source/lib/path.h:265:68: warning: '<anonymous>.Path::separator' may be used uninitialized in this function [-Wmaybe-uninitialized]
   debug_printf("Path %s, separator %c\n", string8().c_str(), (char)separator);
                                                                    ^~~~~~~~~
In file included from ../../../source/lib/os_path.h:26,
                 from ../../../source/lib/self_test.h:28,
                 from ../../../source/simulation2/system/ComponentTest.h:21,
                 from ../../../source/pch/test/precompiled.h:22:
../../../source/lib/path.h: In function 'OsPath DataDir()':
../../../source/lib/path.h:265:68: warning: '<anonymous>.Path::separator' may be used uninitialized in this function [-Wmaybe-uninitialized]
   debug_printf("Path %s, separator %c\n", string8().c_str(), (char)separator);
                                                                    ^~~~~~~~~

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/4035/display/redirect

Harbormaster completed remote builds in B14280: Diff 14535.Dec 10 2020, 6:35 PM

wraitii requested review of this revision.Dec 10 2020, 6:35 PM

Does it handle QNaN floats too? I don't know if those exist in JS;

source/simulation2/components/CCmpMotionBall.cpp
58	unsafe?
source/simulation2/serialization/BinarySerializer.h
83	+ in the simulation?
187	Doesn't work?

Don't you get NaN only when stuff went wrong?

In D3205#142369, @Freagarach wrote:

Don't you get NaN only when stuff went wrong?

I would say generally yes, but that remains a confusing source of OOS and it'd be neater to fix it I suppose.

We can't warn/error on NaN?

In D3205#142386, @Freagarach wrote:

We can't warn/error on NaN?

I can add that yes

Should we push the ticket or do you plan to update this?

source/simulation2/serialization/BinarySerializer.cpp
1	2021
source/simulation2/serialization/BinarySerializer.h
184	reorder?
source/simulation2/serialization/ISerializer.h
1	2021
source/simulation2/serialization/SerializedScriptTypes.h
1	2021
source/simulation2/serialization/StdDeserializer.cpp
1	2021
source/simulation2/tests/test_Serializer.h
1	2021