Remove undefined behaviour from save/load code #8378
Merged
+185
−169
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
michicc
force-pushed
the
pr/cpp_offsetof
branch
from
ced9e67 to
57ae078
Compare
TrueBrain
added
needs review: C++
michicc
force-pushed
the
pr/cpp_offsetof
branch
2 times, most recently
from
cc777cf to
c8f8996
Compare
|
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? |
|
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
force-pushed
the
pr/cpp_offsetof
branch
from
c8f8996 to
82a2096
Compare
glx22
reviewed
Jan 9, 2021
michicc
force-pushed
the
pr/cpp_offsetof
branch
from
82a2096 to
dd61ac1
Compare
TrueBrain
reviewed
Jan 11, 2021
michicc
force-pushed
the
pr/cpp_offsetof
branch
from
dd61ac1 to
d4ebd2f
Compare
Eddi-z
reviewed
Jan 11, 2021
michicc
force-pushed
the
pr/cpp_offsetof
branch
from
d4ebd2f to
3054a4f
Compare
michicc
force-pushed
the
pr/cpp_offsetof
branch
from
3054a4f to
667a673
Compare
TrueBrain
reviewed
Feb 13, 2021
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
force-pushed
the
pr/cpp_offsetof
branch
from
667a673 to
b42cb85
Compare
TrueBrain
reviewed
Feb 13, 2021
michicc
force-pushed
the
pr/cpp_offsetof
branch
from
b42cb85 to
06ec5f1
Compare
TrueBrain
approved these changes
Feb 13, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Our save/load code is totally depending on undefined behaviour via the
use of the
cpp_offsetofmacro.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
offsetoffunction 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
offsetoffor other types as "conditionallysupported", 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
offsetoffunction.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.