Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove undefined behaviour from save/load code #8378

Merged
merged 3 commits into from Feb 13, 2021

Conversation

@michicc
Copy link
Member

@michicc michicc commented Dec 13, 2020

Our save/load code is totally depending on undefined behaviour via the
use of the cpp_offsetof macro.

This macro tries to get the address offset of member variables in a
non-portable way. Dereferencing a casted null pointer to get the member offset
can fail, as the value of the casted null pointer is not required to be numeric 0.
Tricks like the one used here by not using 0 and subtracting two pointers
are still undefined behaviour as you still might end up with a null pointer.

C++ has the offsetof function to work around this limitation. Unfortunatly,
it is only defined on types that are standard layout types and undefined behaviour
otherwise. Most types used during save/load are of course not standard
layout types. C++17 declares offsetof for other types as "conditionally
supported", which means that compilers may or may not implement it.
And even then using it for individual array elements is syntactically
not supported for the standard offsetof function.

Of course, undefined behaviour does not mean random behaviour, and all common
compilers that we use do define the behaviour in question, as evident by the
fact that the save/load system has been working fine for many years.

Still, this PR offers an alternative that does not rely on undefined behaviour
and only looks moderately offensive. Whether this is worth the effort is
left to the discretion of the reader.

@michicc michicc force-pushed the michicc:pr/cpp_offsetof branch from ced9e67 to 57ae078 Dec 13, 2020
@michicc michicc force-pushed the michicc:pr/cpp_offsetof branch 2 times, most recently from cc777cf to c8f8996 Dec 14, 2020
@LordAro
Copy link
Member

@LordAro LordAro commented Dec 19, 2020

Code looks fine, I just can't help but wonder if it's actually the right solution at all. Why does the saveload code rely on looking inside various objects at various offsets? Can it not use the fields of the object properly?

@michicc
Copy link
Member Author

@michicc michicc commented Dec 20, 2020

This is exactly what this PR does, it removes the byte offsets in the saveload code and replaces them by lambda functions that return specific pointers to each field.

If you are instead asking, why have a generic interface that doesn't really know anything about what it is loading? Well, I'm not sure the resulting C++ template madness (and probably dynamically allocated lists and stuff) would be considered any improvement at all.

@michicc michicc force-pushed the michicc:pr/cpp_offsetof branch from c8f8996 to 82a2096 Jan 9, 2021
src/saveload/saveload.h Show resolved Hide resolved
src/saveload/saveload.h Show resolved Hide resolved
src/newgrf_debug_gui.cpp Outdated Show resolved Hide resolved
src/newgrf_debug_gui.cpp Outdated Show resolved Hide resolved
@michicc michicc force-pushed the michicc:pr/cpp_offsetof branch from 82a2096 to dd61ac1 Jan 9, 2021
@michicc michicc force-pushed the michicc:pr/cpp_offsetof branch from dd61ac1 to d4ebd2f Jan 11, 2021
src/newgrf_debug_gui.cpp Outdated Show resolved Hide resolved
src/newgrf_debug_gui.cpp Outdated Show resolved Hide resolved
@michicc michicc force-pushed the michicc:pr/cpp_offsetof branch from d4ebd2f to 3054a4f Jan 11, 2021
@michicc michicc force-pushed the michicc:pr/cpp_offsetof branch from 3054a4f to 667a673 Feb 12, 2021
Copy link
Member

@TrueBrain TrueBrain left a comment

I did my best understanding this patch .. but honestly, I am mostly: I am pretty sure you know what you are doing. So that only leaves some questions about things that stood out :D

src/saveload/oldloader.h Outdated Show resolved Hide resolved
src/saveload/saveload.h Outdated Show resolved Hide resolved
src/newgrf_debug_gui.cpp Outdated Show resolved Hide resolved
src/saveload/oldloader.cpp Show resolved Hide resolved
michicc added 2 commits Dec 13, 2020
Many of the member variables that are used in the oldloader are inside types
that are not standard layout types. Using pointer arithmetics to determine
addresses of members inside types that are not standard layout is generally
undefined behaviour. If we'd use C++17, it is conditionally supported, which means
each compiler may or may not support it. And even then using it for individual
array elements is syntactically not supported the the standard offsetof function.
Many of the member variables that are used in save/load are inside types
that are not standard layout types. Using pointer arithmetics to determine
addresses of members inside types that are not standard layout is generally
undefined behaviour. If we'd use C++17, it is conditionally supported, which means
each compiler may or may not support it. And even then using it for individual
array elements is syntactically not supported the the standard offsetof function.

Unfortunately, the trickery employed for saving linkgraph settings causes quite some
clutter in the settings ini files.
@michicc michicc force-pushed the michicc:pr/cpp_offsetof branch from 667a673 to b42cb85 Feb 13, 2021
src/newgrf_debug_gui.cpp Outdated Show resolved Hide resolved
@michicc michicc force-pushed the michicc:pr/cpp_offsetof branch from b42cb85 to 06ec5f1 Feb 13, 2021
@michicc michicc merged commit 84636fc into OpenTTD:master Feb 13, 2021
10 checks passed
10 checks passed
Emscripten
Details
Commit checker
Details
Check preview needs update Check preview needs update
Details
Linux (clang, clang++)
Details
Linux (gcc, g++)
Details
Mac OS (x64, x86_64)
Details
Windows (windows-latest, x86)
Details
Windows (windows-latest, x64)
Details
Windows (windows-2016, x86) Windows (windows-2016, x86)
Details
Windows (windows-2016, x64) Windows (windows-2016, x64)
Details
@michicc michicc deleted the michicc:pr/cpp_offsetof branch Feb 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants