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.

@TrueBrain TrueBrain added needs review: C++ Review requested from a C++ expert candidate: yes This Pull Request is a candidate for being merged size: large This Pull Request is large in size; review will take a while labels Dec 14, 2020
@michicc michicc force-pushed the pr/cpp_offsetof branch 2 times, most recently from cc777cf to c8f8996 Compare December 14, 2020 21:40
@LordAro
Copy link
Member

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 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.

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
src/newgrf_debug_gui.cpp Outdated Show resolved Hide resolved
src/newgrf_debug_gui.cpp Outdated Show resolved Hide resolved
Copy link
Member

@TrueBrain TrueBrain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
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.
src/newgrf_debug_gui.cpp Outdated Show resolved Hide resolved
@michicc michicc merged commit 84636fc into OpenTTD:master Feb 13, 2021
@michicc michicc deleted the pr/cpp_offsetof branch February 13, 2021 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
candidate: yes This Pull Request is a candidate for being merged needs review: C++ Review requested from a C++ expert size: large This Pull Request is large in size; review will take a while
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants