Currently if you try to play in debug mode on windows, an assertion failure will be triggered because it = m_JumpPointCache.end()
In release it doesn't matter cause we don't use the cache anyway.
Details
- Reviewers
wraitii Itms - Group Reviewers
Restricted Owners Package (Owns No Changed Paths) - Commits
- rP22336: Fix a m_JumpPointCache assertion failure in debug mode, refs rP22219.
Check if the fix makes sense
Check if the code couldn't be written differently.
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Lint
Lint Skipped - Unit
Unit Tests Skipped - Build Status
Buildable 7732 Build 12594: Vulcan Build Jenkins
Event Timeline
The example here https://en.cppreference.com/w/cpp/container/map/find gave me the idea for the check.
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/1578/display/redirect
source/simulation2/helpers/LongPathfinder.cpp | ||
---|---|---|
726 | What? |
source/simulation2/helpers/LongPathfinder.cpp | ||
---|---|---|
726 | Nevermind. I can't formulate easily so I must be wrong :D |
source/simulation2/helpers/LongPathfinder.cpp | ||
---|---|---|
726 | No, just find a situation (some values of the variables) where making the change would change the behavior. ? I found one but I think it's unreachable, can you find it and do you agree? |
source/simulation2/helpers/LongPathfinder.cpp | ||
---|---|---|
726 | state.jpc = it->second.get(); If (m_JumpPointCache.end() != it) and get() returned null |
Mh, this is weird. I would gladly add an "ENSURE" there that there is a possibility class for the C++ code, this means we do something differently in release and debug mode and I don't like it.
Can you clarify where you want to add an ensure? To me this looks like the correct fix. I think that the release just doesn't care if methods like get() are called on the end element (it's undefined behavior, not straight segfault, right?) while debug does because the stl adds more guards in debug mode.
source/simulation2/helpers/LongPathfinder.cpp | ||
---|---|---|
726 | You almost had it, you shouldn't guess that, you can check that there is no place in the code where the shared pointer is set to null while still staying in the map. But I suppose such code could be added somewhere else at some point, and others might be confused by the else if, so let's keep things like that. |
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/1587/display/redirect
Alright, then the release code is wrong too imo. UB shouldn't be called, for one thing, but also I worked under the assumption that m_JumpPointCache.find(passClass) would always return a valid, if empty, vector. That's what the code in the Hierarchical Pathfinder does. IMO we should change this code to initialise the jump point cache with the passibility classes, instead of doing two bounds checks.
This patch removes the UB by checking that the element you look for is present before querying it. You know since rP22225 that this assumption is incorrect (you did have an ensure but it was wrong). I don't think it's needed to initialize the cache on object creation, it's a cache, so creating it lazily makes sense. Also if you want to initialize it earlier, you might have issues if passability classes are not loaded yet. Lazily loading it just sounds less complicated.
Maybe I didn't understand what your idea for a better fix is, but it doesn't sound good to me ?
Since the pathfinder gets recomputed and gets possibility classes from there, I don't think it would be much of a problem. The "it's a cache so lazy loading makes sense" argument is fair though I guess, so why not.