Page MenuHomeWildfire Games

VS2013 Grid.h memset fallback
Needs ReviewPublic

Authored by elexis on Fri, Jul 26, 2:41 AM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Trac Tickets
#5539
#5379
Summary

As reported by Angen and historic_bruno in #5539, the default initialization of array is not implemented correctly in VS2013 (but VS2015), so rP22511 broke there.
This patch uses the previous memset as we didn't find a less broken implementation than that yet (PathCost ctor is still triggering a warning and it's one Ctor modification away from breaking).

Test Plan

Verify that this introduces the code as it was before. Test whether keeping the correct lines in the repository is the right choice as opposed to removing them (as the code isn't broken but VS2013 is) and that VS2013 will be dropped in the next weeks by looks of things (#5379) which would mean that the C++11 / GCC compliant code can be used again without removing and reintroducing the lines (but only reintroducing and removing the broken lines temporarily).
Figure out whether there is a way to init that array without breaking on VS2013, without triggering a GCC compiler warning, without skipping the PathCost ctor.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 8602
Build 14087: Vulcan BuildJenkins
Build 14086: arc lint + arc unit

Event Timeline

elexis created this revision.Fri, Jul 26, 2:41 AM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/simulation2/helpers/Grid.h
|  37| template<typename·T>
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'template<...' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/236/display/redirect

historic_bruno added a subscriber: historic_bruno.EditedFri, Jul 26, 6:05 AM

Here's the observed behavior in VS2013:

  • pre-rP22511: builds and can play matches (tested last week when I was still using VS2013).
  • rP22511: builds, but instant crashes on match start (I didn't tested this as I had switched to VS2015... was not reported either). Also introduced memleak.
    • This is important because we hadn't introduced placement-new to fix the memleak. That's evidence that it can be ruled out as the problem. More later.
  • rP22545: builds, fixes memleak, but crashes on match start (I also didn't test this on VS2013... but gameboy and Angen did, and reported the crash).
    • Suspicion falls on placement-new, as one might expect, it was all that changed in this commit.
    • I tested after the report from Angen and confirmed the crash. Also suspected placement-new, but while testing things, found that rP22511 crashes too.

So it must be something in rP22511. Three candidates by process of elimination, basically.

  • The following diff builds, but crashes:
Index: Grid.h
===================================================================
--- Grid.h	(revision 22551)
+++ Grid.h	(working copy)
@@ -217,8 +217,7 @@
 		for (size_t i = 0; i < (size_t)(m_BW*m_BH); ++i)
 			delete[] m_Data[i];
 
-		// Reset m_Data by value-constructing in place with placement new.
-		m_Data = new (m_Data) T*[m_BW*m_BH]();
+		memset(m_Data, 0, m_BW*m_BH*sizeof(T*));
 	}
 
 	void set(int i, int j, const T& value)
  • Next, this diff also builds but crashes:
Index: Grid.h
===================================================================
--- Grid.h	(revision 22551)
+++ Grid.h	(working copy)
@@ -203,7 +203,8 @@
 		m_BW = (u16)((m_W + BucketSize-1) >> BucketBits);
 		m_BH = (u16)((m_H + BucketSize-1) >> BucketBits);
 
-		m_Data = new T*[m_BW*m_BH]();
+		m_Data = new T*[m_BW*m_BH];
+		memset(m_Data, 0, m_BW*m_BH*sizeof(T*));
 	}
 
 	~SparseGrid()
@@ -217,8 +218,7 @@
 		for (size_t i = 0; i < (size_t)(m_BW*m_BH); ++i)
 			delete[] m_Data[i];
 
-		// Reset m_Data by value-constructing in place with placement new.
-		m_Data = new (m_Data) T*[m_BW*m_BH]();
+		memset(m_Data, 0, m_BW*m_BH*sizeof(T*));
 	}
 
 	void set(int i, int j, const T& value)

The only changes left from rP22511 are:

Index: Grid.h
===================================================================
--- Grid.h	(revision 22510)
+++ Grid.h	(revision 22511)
@@ -1,4 +1,4 @@
-/* Copyright (C) 2018 Wildfire Games.
+/* Copyright (C) 2019 Wildfire Games.
  * This file is part of 0 A.D.
  *
  * 0 A.D. is free software: you can redistribute it and/or modify
@@ -190,8 +190,7 @@
 		size_t b = (j >> BucketBits) * m_BW + (i >> BucketBits);
 		if (!m_Data[b])
 		{
-			m_Data[b] = new T[BucketSize*BucketSize];
-			memset(m_Data[b], 0, BucketSize*BucketSize*sizeof(T));
+			m_Data[b] = new T[BucketSize*BucketSize]();
 		}
 		return m_Data[b];
 	}

That's why I said it must be something about array initialization and not placement-new, it's the only thing left.

Angen added a subscriber: Angen.Fri, Jul 26, 7:15 AM
Angen added inline comments.
source/simulation2/helpers/Grid.h
30

you could use here visual studio version here to set this value

The diff is actually unsatisfying because it's still triggering the compile warning and there is the possibility that VS2013 is not dropped in the next few weeks.

source/simulation2/helpers/Grid.h
30

I don't know if one can, especially if one could what bag of weirdness one imports

We could replace the memset with a loop, though that's somewhat likely to be slower as it might not be optimised well.

TBH I'd be tempted to just call it a day for VS13 :/
What's the reason for wanting to keep it right now?

Why not to use std::fill? Does it produce the warning?

This comment was removed by wraitii.