Page MenuHomeWildfire Games

Fix a m_JumpPointCache assertion failure in debug mode
ClosedPublic

Authored by Stan on Sat, Jun 1, 3:30 PM.

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

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.

Test Plan

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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Stan created this revision.Sat, Jun 1, 3:30 PM

The example here https://en.cppreference.com/w/cpp/container/map/find gave me the idea for the check.

Vulcan added a comment.Sat, Jun 1, 3:33 PM

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

Link to build: https://jenkins.wildfiregames.com/job/differential/1578/display/redirect

elexis retitled this revision from Fix an assertion failure in debug mode to Fix a m_JumpPointCache assertion failure in debug mode.Sat, Jun 1, 5:10 PM
Itms requested changes to this revision.Sun, Jun 2, 3:02 PM

Well well well fixes always need more fixes it seems. Dead code is the worst.

source/simulation2/helpers/LongPathfinder.cpp
723 ↗(On Diff #8272)

We usually test it != ... it's funny to see it the other way around :D

726 ↗(On Diff #8272)

That can now become

else if (m_UseJPSCache)
This revision now requires changes to proceed.Sun, Jun 2, 3:02 PM
Stan added inline comments.Sun, Jun 2, 3:17 PM
source/simulation2/helpers/LongPathfinder.cpp
723 ↗(On Diff #8272)

Sure i'll fix that :)

726 ↗(On Diff #8272)

You sure ? That changes the behavior. It used to initialize anyway and/or fail then if it had failed create a new cache.

Itms added inline comments.Sun, Jun 2, 3:23 PM
source/simulation2/helpers/LongPathfinder.cpp
726 ↗(On Diff #8272)

What?

Stan added inline comments.Sun, Jun 2, 3:38 PM
source/simulation2/helpers/LongPathfinder.cpp
726 ↗(On Diff #8272)

Nevermind. I can't formulate easily so I must be wrong :D

Itms added inline comments.Sun, Jun 2, 3:44 PM
source/simulation2/helpers/LongPathfinder.cpp
726 ↗(On Diff #8272)

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?

Stan planned changes to this revision.Sun, Jun 2, 4:24 PM
Stan added inline comments.
source/simulation2/helpers/LongPathfinder.cpp
726 ↗(On Diff #8272)

state.jpc = it->second.get();

If (m_JumpPointCache.end() != it) and get() returned null
If merge the if block then state.jpc would not correctly be initialized. But I guess that if std::find has an item then it->second shouldn't be 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.

Itms added a comment.Sun, Jun 2, 10:11 PM

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 ↗(On Diff #8272)

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.

Stan marked 5 inline comments as done.Sun, Jun 2, 10:38 PM

In rP22219 there was

ENSURE(it != m_JumpPointCache.end());
Stan updated this revision to Diff 8283.Sun, Jun 2, 10:38 PM
  • Use else if,
  • Reverse condition members

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

Link to build: https://jenkins.wildfiregames.com/job/differential/1587/display/redirect

In D1942#80658, @Itms wrote:

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.

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.

Itms added a comment.Mon, Jun 3, 10:08 AM

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.

Itms accepted this revision.Tue, Jun 4, 10:27 AM
This revision is now accepted and ready to land.Tue, Jun 4, 10:27 AM
This revision was automatically updated to reflect the committed changes.
Owners added a subscriber: Restricted Owners Package.Tue, Jun 4, 10:30 AM