cpp/openttd-patchpack/source Changeset - r24754:d4186fe759b7

Changeset - r24754:d4186fe759b7

Parent rev.

Child rev.

[Not reviewed]

master

0 2 0

Michael Lutz - 4 years ago 2020-12-13 23:11:39
michi@icosahedron.de

Codechange: Don't use cpp_offsetof in the TTO/TTD savegame loader.

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.

2 files changed with 14 insertions and 15 deletions:

src/saveload/oldloader.cpp

src/saveload/oldloader.h

0 comments (0 inline, 0 general)

src/saveload/oldloader.cpp

➞

Show inline comments

@@ @@ -108,8 +108,6 @@ byte ReadByte(LoadgameState *ls) @@
  */
 bool LoadChunk(LoadgameState *ls, void *base, const OldChunks *chunks)
+{
 	byte *base_ptr = (byte*)base;
 	for (const OldChunks *chunk = chunks; chunk->type != OC_END; chunk++) {
 		if (((chunk->type & OC_TTD) && _savegame_type == SGT_TTO) ||
 				((chunk->type & OC_TTO) && _savegame_type != SGT_TTO)) {
@@ @@ -134,8 +132,8 @@ bool LoadChunk(LoadgameState *ls, void * @@
 						break;
 					case OC_ASSERT:
 						DEBUG(oldloader, 4, "Assert point: 0x%X / 0x%X", ls->total_read, chunk->offset + _bump_assert_value);
 						if (ls->total_read != chunk->offset + _bump_assert_value) throw std::exception();
 						DEBUG(oldloader, 4, "Assert point: 0x%X / 0x%X", ls->total_read, (uint)(size_t)chunk->ptr + _bump_assert_value);
 						if (ls->total_read != (size_t)chunk->ptr + _bump_assert_value) throw std::exception();
 					default: break;
+				}
 			} else {
@@ @@ -153,10 +151,10 @@ bool LoadChunk(LoadgameState *ls, void * @@
+				}
 				/* When both pointers are nullptr, we are just skipping data */
-				if (base_ptr == nullptr && chunk->ptr == nullptr) continue;
+				if (base == nullptr && chunk->ptr == nullptr) continue;
 				/* Writing to the var: bits 8 to 15 have the VAR type */
 				if (chunk->ptr == nullptr) ptr = base_ptr + chunk->offset;
 				/* Chunk refers to a struct member, get address in base. */
 				if (chunk->ptr == nullptr) ptr = (byte *)chunk->offset(base);
 				/* Write the data */
 				switch (GetOldChunkVarType(chunk->type)) {

src/saveload/oldloader.h

➞

Show inline comments

@@ @@ -82,13 +82,14 @@ enum OldChunkType { @@
 DECLARE_ENUM_AS_BIT_SET(OldChunkType)
 typedef bool OldChunkProc(LoadgameState *ls, int num);
 typedef void *OffsetProc(void *base);
 struct OldChunks {
 	OldChunkType type;   ///< Type of field
 	uint32 amount;       ///< Amount of fields
 	void *ptr;           ///< Pointer where to save the data (may only be set if offset is 0)
 	uint offset;         ///< Offset from basepointer (may only be set if ptr is nullptr)
 	void *ptr;           ///< Pointer where to save the data (takes precedence over #offset)
 	OffsetProc *offset;  ///< Pointer to function that returns the actual memory address of a member (ignored if #ptr is not nullptr)
 	OldChunkProc *proc;  ///< Pointer to function that is called with OC_CHUNK
 };
@@ @@ -123,12 +124,12 @@ static inline uint32 ReadUint32(Loadgame @@
  *  - OCL_CHUNK: load another proc to load a part of the savegame, 'amount' times
  *  - OCL_ASSERT: to check if we are really at the place we expect to be.. because old savegames are too binary to be sure ;)
  */
 #define OCL_SVAR(type, base, offset)         { type,                 1, nullptr, (uint)cpp_offsetof(base, offset), nullptr }
 #define OCL_VAR(type, amount, pointer)       { type,            amount, pointer,    0,                             nullptr }
 #define OCL_END()                            { OC_END,               0, nullptr,    0,                             nullptr }
 #define OCL_CNULL(type, amount)              { OC_NULL | type,  amount, nullptr,    0,                             nullptr }
 #define OCL_CCHUNK(type, amount, proc)       { OC_CHUNK | type, amount, nullptr,    0,                             proc }
 #define OCL_ASSERT(type, size)               { OC_ASSERT | type,     1, nullptr, size,                             nullptr }
 #define OCL_SVAR(type, base, offset)         { type,                 1, nullptr, [] (void *b) -> void * { return std::addressof(static_cast<base *>(b)->offset); }, nullptr }
 #define OCL_VAR(type, amount, pointer)       { type,            amount, pointer, nullptr, nullptr }
 #define OCL_END()                            { OC_END,               0, nullptr, nullptr, nullptr }
 #define OCL_CNULL(type, amount)              { OC_NULL | type,  amount, nullptr, nullptr, nullptr }
 #define OCL_CCHUNK(type, amount, proc)       { OC_CHUNK | type, amount, nullptr, nullptr, proc }
 #define OCL_ASSERT(type, size)               { OC_ASSERT | type,     1, (void *)(size_t)size, nullptr, nullptr }
 #define OCL_NULL(amount)        OCL_CNULL((OldChunkType)0, amount)
 #define OCL_CHUNK(amount, proc) OCL_CCHUNK((OldChunkType)0, amount, proc)

0 comments (0 inline, 0 general)