[Linux]: Fix other crash in Garbage #1432

sobkas · 2023-08-27T14:43:37Z

For some reason game sometimes crashes while in Garbage.

Xottab-DUTY · 2023-08-27T16:33:05Z

src/xrGame/quadtree.h

+            if (m_free == NULL)
+            {
+              return NULL;
+            }


Codestyle, plus it's better to leave VERIFY here, because it's not normal when m_free is null.

Suggested change

if (m_free == NULL)

{

return NULL;

}

VERIFY(m_free);

if (!m_free)

return nullptr;

Partially fixed, but not fully because:

We always use nullptr, NULL is not used (and is replaced to nullptr during code refactoring)

We don't check for nullptr explicitly, where it's possible to not do it.

Partially fixed, but not fully because:

1. We always use nullptr, NULL is not used (and is replaced to nullptr during code refactoring) 2. We don't check for nullptr explicitly, where it's possible to not do it.

Well there is need to check if we have nullptr, otherwise there is a crash.

Partially fixed, but not fully because:

1. We always use nullptr, NULL is not used (and is replaced to nullptr during code refactoring) 2. We don't check for nullptr explicitly, where it's possible to not do it.

Well there is need to check if we have nullptr, otherwise there is a crash.

I mean, we use if (!ptr) of if (ptr) checks rather than directly comparing to nullptr.

Xottab-DUTY · 2023-08-27T16:33:26Z

src/xrGame/quadtree_inline.h

+
+            if (list_item == NULL)
+            {
+                return;
+            }


Suggested change

if (list_item == NULL)

{

return;

}

if (!list_item)

return;

Xottab-DUTY · 2023-08-27T16:34:04Z

src/xrGame/quadtree_inline.h

+    if (node == NULL)
+    {
+        return NULL;
+    }


Suggested change

if (node == NULL)

{

return NULL;

}

VERIFY(node);

if (!node)

return nullptr;

Xottab-DUTY · 2023-08-27T16:36:24Z

src/xrGame/quadtree_inline.h

@@ -102,6 +114,11 @@ IC void CSQuadTree::insert(_object_type* object)
        if (!*node)
            *node = m_nodes->get_object();

+        if (node == NULL)


node gets dereferenced 3 lines above :D
Also, we are in the cycle here. I'm not sure at the moment if we should return or continue the cycle.

Now get_object() can return null, needs to check for it.

Based on code already there return is ok, continue causes crashes.

Now get_object() can return null, needs to check for it.

But here we check if node is null, while get_object assigns to *node, not node 🤔

Now get_object() can return null, needs to check for it.

But here we check if node is null, while get_object assigns to *node, not node 🤔

Now get_object() can return null, needs to check for it.

But here we check if node is null, while get_object assigns to *node, not node 🤔

node is CQuadNode** node so *node is pointer I think? Otherwise it wouldn't compile.

Xottab-DUTY

In the Files tab of a pull request, you can just press Add suggestion to batch and merge all my suggestions as one commit instead of porting them and recommiting manually.

So, I'm again here, with these suggestions. :)
We still need to leave those VERIFY in the code, because original devs assumed that it is not normal if m_free is nullptr.
Since QuadTree is a generic container it's good to keep it robust with nullptr checks, but it's also good to keep these assertions (VERIFYs) for debugging purposes.

src/xrGame/quadtree.h

Xottab-DUTY · 2023-08-28T15:20:36Z

src/xrGame/quadtree_inline.h

@@ -92,8 +92,16 @@ IC void CSQuadTree::insert(_object_type* object)
        if (depth == m_max_depth)
        {
            CListItem* list_item = m_list_items->get_object();
+


Suggested change

Xottab-DUTY · 2023-08-28T15:20:45Z

src/xrGame/quadtree_inline.h

            list_item->m_object = object;
            list_item->m_next = (CListItem*)((void*)(*node));
+
+            if (!list_item->m_next )


Suggested change

if (!list_item->m_next )

if (!list_item->m_next)

For some reason game sometimes crashes while in Garbage.

Xottab-DUTY · 2023-08-28T16:26:24Z

Could you give me the save file with the crash?

sobkas · 2023-09-04T19:33:14Z

Could you give me the save file with the crash?

I started shooting at bandits and it crashed(it might take some time).
a234.sav.gz

Co-authored-by: Xottab_DUTY <[email protected]>

sobkas · 2023-09-04T20:26:42Z

In the Files tab of a pull request, you can just press Add suggestion to batch and merge all my suggestions as one commit instead of porting them and recommiting manually.

I hate that there are two commit then, I prefer fix commits manually.
It's bad habit I took from using gerrit/baremetal git.

sobkas · 2023-09-09T16:14:20Z

It's needed to be merged, without it I have rare but occurring crashes.

Xottab-DUTY · 2023-10-18T14:36:31Z

Superseded by #1439, and then by #1440

Xottab-DUTY · 2023-10-18T14:38:57Z

src/xrGame/quadtree_inline.h

@@ -102,6 +108,9 @@ IC void CSQuadTree::insert(_object_type* object)
        if (!*node)
            *node = m_nodes->get_object();

+        if (!node)


So, back to the old discussion, we can't do *node (dereference) two lines above if node is nullptr, so this if (!node) check is actually late and we need to check for nullptr earlier.

Xottab-DUTY changed the title ~~[Linux]: Fix other crash in Gargage~~ [Linux]: Fix other crash in Garbage Aug 27, 2023

Xottab-DUTY requested changes Aug 27, 2023

View reviewed changes

Xottab-DUTY added Bug The issue in the run-time. Code Quality labels Aug 27, 2023

sobkas force-pushed the other_crash branch 3 times, most recently from 6d41c16 to 49e4e0e Compare August 28, 2023 15:07

Xottab-DUTY reviewed Aug 28, 2023

View reviewed changes

sobkas force-pushed the other_crash branch from 49e4e0e to 17043fd Compare August 28, 2023 15:45

[Linux]: Fix other crash in Gargage

5e68ee8

For some reason game sometimes crashes while in Garbage.

sobkas force-pushed the other_crash branch from 17043fd to 5e68ee8 Compare August 28, 2023 15:50

Apply suggestions from code review

0b710d7

Co-authored-by: Xottab_DUTY <[email protected]>

sobkas closed this Sep 9, 2023

sobkas deleted the other_crash branch September 9, 2023 16:17

Xottab-DUTY reviewed Oct 18, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Linux]: Fix other crash in Garbage #1432

[Linux]: Fix other crash in Garbage #1432

sobkas commented Aug 27, 2023

Xottab-DUTY Aug 27, 2023

sobkas Aug 27, 2023

Xottab-DUTY Aug 27, 2023

sobkas Aug 28, 2023

Xottab-DUTY Aug 28, 2023 •

edited

Loading

Xottab-DUTY Aug 27, 2023

sobkas Aug 27, 2023

Xottab-DUTY Aug 27, 2023

Xottab-DUTY Aug 27, 2023

sobkas Aug 27, 2023

sobkas Aug 27, 2023 •

edited

Loading

Xottab-DUTY Aug 28, 2023

sobkas Sep 4, 2023

Xottab-DUTY left a comment •

edited

Loading

Xottab-DUTY Aug 28, 2023

Xottab-DUTY Aug 28, 2023

Xottab-DUTY commented Aug 28, 2023

sobkas commented Sep 4, 2023

sobkas commented Sep 4, 2023

sobkas commented Sep 9, 2023

Xottab-DUTY commented Oct 18, 2023

Xottab-DUTY Oct 18, 2023

-            if (m_free == NULL)
-            {
-              return NULL;
-            }
+            VERIFY(m_free);
+            if (!m_free)
+                return nullptr;

[Linux]: Fix other crash in Garbage #1432

[Linux]: Fix other crash in Garbage #1432

Conversation

sobkas commented Aug 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xottab-DUTY Aug 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sobkas Aug 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xottab-DUTY left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xottab-DUTY commented Aug 28, 2023

sobkas commented Sep 4, 2023

sobkas commented Sep 4, 2023

sobkas commented Sep 9, 2023

Xottab-DUTY commented Oct 18, 2023

Choose a reason for hiding this comment

Xottab-DUTY Aug 28, 2023 •

edited

Loading

sobkas Aug 27, 2023 •

edited

Loading

Xottab-DUTY left a comment •

edited

Loading