Converting OXCE to rapidyaml

[Yankes]

ok,

btw after bulding new version, it crash on Xcom2:

Code: [Select]


[19-11-2024_15-36-13]   [WARN]  scanModDir('./', 'standard'): Bad metadata.yml xcom2, skipping.

Thread 14 "openxcom" received signal SIGABRT, Aborted.


call stack:

Code: [Select]

#7  0x00007ffa6690e12a in free (p=0xa00000000) at /usr/src/debug/cygwin-3.5.4-1/winsup/cygwin/mm/malloc_wrapper.cc:78
#8  0x00007ffa66927adb in _sigfe () at sigfe.s:36
#9  0x0000000100c50aae in c4::yml::Tree::~Tree() ()
#10 0x00000001007be7fa in OpenXcom::YAML::YamlRootNodeReader::~YamlRootNodeReader() ()
#11 0x00000001006a23de in OpenXcom::FileMap::scanModDir(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, bool) ()
#12 0x00000001006e5aa4 in OpenXcom::Options::refreshMods() ()

I did not yet spend time on analyzing it (nor fetch your last changes).
Do you know what could go wrong?


btw2.
I see you move rapid to `Engine`, I do not think is good move. This is not our code and probably better would be not mix it with our.
We only "bundle it" and someone should be able to use different version if he want (on his own responsibility ), `libs` folder would in
my option best place for code like this.

[Delian]

>I do not think is good move
It's just for organizing files in Visual Studio. The actual files are still going to be in the ../libs/ folder. There are also other folders in the Engine folder that aren't our code. And other files lying unorganized in the top folder of the project. The logical location in the project doesn't have any significance other than for file organization. Basically, it shouldn't be mixed with folders in the top level, because that makes it harder to navigate the folders and files which we commonly access.

As for the crash... I'm not sure.
That's basically the 1st time game tries to read yaml. (normally the first time is trying to read options.cfg, but I'm assuming you don't have it created yet).
Reader is created, reads yaml. When it falls out of scope, it destroys itself and the tree, which holds buffers. But, it seems there was already a problem with parsing?
Parsing goes fine for me:

The tree is created with 10 nodes and I don't get a bad metadata report. So the fact that it's reporting bad metadata, something must've gone wrong with the parsing... But if something went wrong, why didn't the parsing crash or report any errors. Weird.
Maybe you forgot to include some rapidyaml files in the build?

I'm assuming the abort() call came from rapidyaml\c4\ext\sg14\inplace_function.h:91, where it would normally throw a std::bad_function_call() exception.
But that call probably came because ~Tree destructor tried to call a "free" function, defined through a macro. Perhaps poorly defined, because it tried to call an non-existent free function.

[Yankes]

ok, if this move is only in VS then this is fine, probably I jump gun too fast

for bug, it could be some combination of different effects, I use cygwin as my env, this mix win with linux and some could conflict.
Probably then this is bug purely on my side then.

[Meridian]

Just a small update, MacOS build was successful, although with many warnings.

[Delian]

Just a small update, MacOS build was successful, although with many warnings.

Question is, does it run hehe.

Probably then this is bug purely on my side then.

No, rapidyaml allows you to provide it with your own "malloc" and "free" callback, so that's what I'm trying to do now.

...

Ok, try with this https://github.com/Delian0/OpenXcom/commit/452522af25541513a9bf26caf9d0f6e7551b4e46

[Yankes]

Code: [Select]

[20-11-2024_01-50-14]   [FATAL] YamlRootNodeReader(char* data, size_t size, std::string fileNameForError)
[20-11-2024_01-50-14]   [FATAL] ~YamlRootNodeReader
[20-11-2024_01-50-14]   [WARN]  scanModDir('./', 'standard'): Bad metadata.yml xcom2, skipping.
[20-11-2024_01-50-14]   [FATAL] ~YamlRootNodeReader
I think we hit bug with compiler, as far I know this code should not compilable because `YamlRootNodeReader` is not copyable type.
Later version of C++ there is concept for NRVO, but type need have at least move constructor.
Its look like MSVC apply NRVO but GCC try but forget to perform NRVO, because of this code call destructor on some garbage memory.

Tomorrow I will see how is easiest way to fix it.

[Delian]

Does memory allocation and freeing work ok now? If you add breakpoints inside s_allocate and s_free, do they get called?

Ok, how about this. Instead of

Code: [Select]

const auto& reader = frec->getYAML();

try

Code: [Select]

YAML::YamlRootNodeReader reader(frec->fullpath);


The code does exactly the same, but without any movement through return values.
I didn't want to add a move constructor to those classes because that allowed me to spot if any copy/move accidentally took place.
The problem is, there's a lot of other places in the code that have a YamlNodeReader as a return value. So if Copy elision doesn't work, I'd most likely have to add a move constructor to all the classes. Maybe you can add a flag to your compiler which applies move elision more aggressively?

[Meridian]

Another update, the Android build was not successful.

What I did so far:
1. merged main branch into android branch -- that was actually pretty easy, only a few merge conflicts
2. updated cmake in the android project -- see screenshot 1
3. build failed -- see screnshot 2
4. I looked at the source code line indicated in the error message, but don't really know what to do -- see screenshot 3

Trying to solve it with Yankes atm.

[Yankes]

Does memory allocation and freeing work ok now? If you add breakpoints inside s_allocate and s_free, do they get called?

Ok, how about this. Instead of

Code: [Select]

const auto& reader = frec->getYAML();

try

Code: [Select]

YAML::YamlRootNodeReader reader(frec->fullpath);


The code does exactly the same, but without any movement through return values.
I didn't want to add a move constructor to those classes because that allowed me to spot if any copy/move accidentally took place.
The problem is, there's a lot of other places in the code that have a YamlNodeReader as a return value. So if Copy elision doesn't work, I'd most likely have to add a move constructor to all the classes. Maybe you can add a flag to your compiler which applies move elision more aggressively?

this is not problem with memory allocation, this is pure compiler clobbering return value object, second destructor is called on garbage where should be returned object. I did not make minimal test case to say what go wrong. IMHO compiler should return error ("you try return unmovable type") instead of creating UB.
One solution is not try use NRVO but RVO that allow return anything when using`return X{ };` and we can make your code work this way.

Another thing is add fake constructor. because NRVO is allowed only when you have one, but whole point is that is never called, this mean never linked too.
This mean if you have only declaration but not definition (aka no body) then every thing will work, but if it try copy object, linking will fail.

I checkout this more today. Maybe I will find some easy solution.

[Delian]

@Yankes

https://github.com/Delian0/OpenXcom/commit/e88396630c8a8d596f906faef42413b4062033c7

Meh, I just added all the missing constructors. Some throw exceptions tho

@Meridian
Researching...
...it seems rapidyaml is trying to forward declare certain std types (like std::string) for some reason. But the macro logic isn't able to determine which standard library it is.

Edit:
"Android NDK uses LLVM's libc++, with support for _LIBCPP_ABI_NAMESPACE inline namespace. Older versions of the Android NDK supported GNU libstdc++, indicated by __GLIBCXX__ or __GLIBCPP__."

You shouldn't have gotten the "unknown standard library" error, because the conditions check for those specific macros. I don't get it...

[Yankes]

@Yankes

https://github.com/Delian0/OpenXcom/commit/e88396630c8a8d596f906faef42413b4062033c7

Meh, I just added all the missing constructors. Some throw exceptions tho

Code: [Select]

YamlNodeReader::YamlNodeReader(const YamlNodeReader&& other) noexceptthis is wrong signature for move constructor. `const&&` is the crazy uncle nobody talks about in C++ community
Proper signature do not have `const` as whole point of move constructor is rip out all internals of other object and leave carcass that only job is
to dispose gracefully.

overall I push some commits that fix bugs:
https://github.com/Yankes/OpenXcom/commits/rapidyam/
first one I reported, I simply fix by using proper RVO, this mean by code like:

Code: [Select]

return YAML::YamlRootNodeReader{data, size, fullpath};
and delete of `data` was moved to constructor.
This is not perfect solution as this "steal" this memory but as far I see this was only one use of this constructor.

Second fix was:

Code: [Select]

str = ryml::csubstr(data, str.find("\n---") + 1); // "safe" when file do not have "---", it will crop out whole file but it should be invalid anyway
your version had three problems:
`\r\n` and line like `foo: bar---` both would break save. Not to mention some random file that do not have any `---` in it.
My version handle all cases (in last one trim whole file but this probably will not be problem).



if you do not want some functions to exists, you can mark them as `= deleted;` when compiler would try call them, it will break compilation.


another thought, how much of runtime of loading files is spend on allocations? because we allocate buffer copy content to it and then read from it and delete it.
we could probably in many cases skip this, or at least reuse this memory.
I had helper class `class RawData` that could be good tool for handling memory like this, `YamlRootNodeReader` could even store it and use to implicate parsing.
As free benefits, your class would stop being copyable and only movable because of `std::unique_ptr`.

Another thing would be make all members of `YamlRootNodeReader` as values, not pointers. With this `Parse` would become delegated constructor.


[ps]
For default copy-construcotrs, is seems I miss remember this:
https://en.cppreference.com/w/cpp/language/copy_constructor#Implicitly-defined_copy_constructor

said this is only "deprecated" when you define destructor, this mean we need go "rule of 5" or "rule of 0"

https://en.cppreference.com/w/cpp/language/rule_of_three

[Meridian]

Some more updates:

1. cross-compilation for windows in MXE was successful... but the game silently crashes without any errors on loading 'standard' mods, i.e. xcom1

http://108.61.157.159/versions/win/Test-Extended-8.0-37400f2d7-2024-11-20-win32.7z

2. linux appimage compilation was successful... but I can't test it yet, will test when I get back home next week

https://github.com/MeridianOXC/OpenXcom/actions/runs/11948727493

3. compilation for ios in xcode: I had some trial and error to do with project settings, but I managed to add rapidyaml and also set up header file search paths so that I don't get "file not found" errors. The compilation started, but failed somewhere in the middle on something that looks like linker errors? Don't know how to continue... see attached screenshot

[Meridian]

As for next steps, I suggest going step by step, solving only one issue/build at a time.

We have VisualStudio build working.
Now I would recommend concentrating on the MXE build... which probably suffers from the same issue that Yankes found and is discussed above.

Then move to Linux native build, Linux appimage build, MacOS native build, Android build and finally iOS build. In that order.

[Delian]

@Yankes
>this is wrong signature for move constructor.
Well, I made them const so that I could more easily determine what was being copied, and what was being moved. That allowed me to find a few places in code which I fixed. But yeah, move constructor with const doesn't make sense.

>your version had three problems:
Nice catch. I should mention that, if a sav file is missing ---, it's corrupted, because the header contains critical data.

>I had helper class `class RawData`
I should probably have made readFileRaw and getYamlSaveHeaderRaw return RawData instead of char*.

YamlRootNodeReader currently copies that data to ryml::Tree's buffer (arena), so there's no need to keep that data.
I could make a version that would use parse_in_place instead of parse_in_arena, but then data would have to outlive YamlRootNodeReader instance, which means it would have to keep the data. It's not worth the effort, because it only makes parsing maybe 1% faster (skips 1 memory allocation + copy).

Anyway, yeah, unique_ptr with RawData would probably be a better solution instead of YamlRootNodeReader calling free itself. Probably also safer, because if there's an exception, the free doesn't get called.

Should I change stuff to use unique_ptr + RawData?

1. cross-compilation for windows in MXE was successful... but the game silently crashes without any errors on loading 'standard' mods, i.e. xcom1

Code: [Select]

(1da8.32e4): Access violation - code c0000005 (first chance)
First chance exceptions are reported before any exception handling.
This exception may be expected and handled.
*** WARNING: Unable to verify timestamp for openxcom.exe
openxcom!ZN2c43yml4TreeD2Ev+0x20:
00007ff7`570540a0 ff5350          call    qword ptr [rbx+50h] ds:000002ad`1cf7dd10=feeefeeefeeefeee
It looks like the same issue that Yankes had and was fixed. The issue with malloc and free.
But, it could also be the same problem with failed NRVO, which he also fixed yesterday.

>3. compilation for ios in xcode
Based on those linker errors my best guess is that the rapidyaml cpp files aren't being compiled. Does a build log contain any rapidyaml hpp file compilation?
It could also be that the #include in Yaml.h (#include "../../libs/rapidyaml/ryml.hpp") is wrong, if the hpp files aren't at the right place.

[psavola]

I can report that the linux appimage crashes immediately at start (I was using XCF master mod):

[21-11-2024_12-45-01]   [INFO]  Scanning standard mods in '/tmp/.mount_oxce_gOsHz9A/usr/bin//../share/openxcom/'...
[21-11-2024_12-45-01]   [FATAL] A fatal error has occurred: Segmentation fault.
[21-11-2024_12-45-01]   [FATAL] ./oxce_git_2024_11_21_0839-x86_64.AppImage(OpenXcom::CrossPlatform::stackTrace(void*)+0x3d) [0x557fb310ccfd]
[21-11-2024_12-45-01]   [FATAL] ./oxce_git_2024_11_21_0839-x86_64.AppImage(OpenXcom::CrossPlatform::crashDump(void*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)+0x3fa) [0x557fb310d6ea]
[21-11-2024_12-45-01]   [FATAL] ./oxce_git_2024_11_21_0839-x86_64.AppImage(signalLogger(int)+0x45) [0x557fb2ebb915]
[21-11-2024_12-45-01]   [FATAL] /lib64/libc.so.6(+0x3e730) [0x7f306883e730]
[21-11-2024_12-45-01]   [FATAL] ./oxce_git_2024_11_21_0839-x86_64.AppImage(OpenXcom::YAML::YamlNodeReader::isMap() const+0x16) [0x557fb32527a6]
[21-11-2024_12-45-01]   [FATAL] ./oxce_git_2024_11_21_0839-x86_64.AppImage(OpenXcom::FileMap::scanModDir(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, bool)+0xb2d) [0x557fb311f3ed]
[21-11-2024_12-45-01]   [FATAL] ./oxce_git_2024_11_21_0839-x86_64.AppImage(OpenXcom::Options::refreshMods()+0x8e5) [0x557fb316cb25]
[21-11-2024_12-45-01]   [FATAL] ./oxce_git_2024_11_21_0839-x86_64.AppImage(OpenXcom::Options::updateMods()+0x103) [0x557fb316e0e3]
[21-11-2024_12-45-01]   [FATAL] ./oxce_git_2024_11_21_0839-x86_64.AppImage(OpenXcom::StartState::load(void*)+0x221) [0x557fb336ae11]
[21-11-2024_12-45-01]   [FATAL] /tmp/.mount_oxce_gOsHz9A/usr/bin/../lib/libSDL-1.2.so.0(+0x15f3c) [0x7f3068fc5f3c]
[21-11-2024_12-45-01]   [FATAL] /tmp/.mount_oxce_gOsHz9A/usr/bin/../lib/libSDL-1.2.so.0(+0x55baf) [0x7f3069005baf]
[21-11-2024_12-45-01]   [FATAL] /lib64/libc.so.6(+0x89d82) [0x7f3068889d82]
[21-11-2024_12-45-01]   [FATAL] /lib64/libc.so.6(+0x10ee20) [0x7f306890ee20]
[21-11-2024_12-45-20]   [FATAL] OpenXcom has crashed: Segmentation fault.