[Runtime Bug/Crash] Linux: audio related crash when trying to play a mission #141
Comments
|
can you also get a backtrace from gdb? (enter |
|
|
Same as #7, corruptions on loading level. |
That could be the reason I couldn't send out the guidebot or open a door on level 2 |
|
This is also present on macOS targeting arm64. |
|
Only Windows is being forced to build in 32bit. For Linux, at least, |
|
One problem with trying to get 32-bit binaries working on a modern version of macOS: It doesn't support 32-bit binaries, either AARCH32 or i386. |
|
The problem here seems to be that in BOA_INDEX() is the following macro: in the end,
I just provoked the crash with starting a new game, specifically the "Pilot Training" mission. No idea why Adding if(sound_seg == -1)
return false;right before the |
I was looking into this earlier tonight, seems I was on the right track after all. It gets initialized at Line 1187 in 86a6c13 Along with that are a few other inits with val |
|
But probably it should be set to a proper value elsewhere? Either way, the solution definitely is not to initialize Either the code only worked by accident on Win32 (by overflowing to an address that's valid) and adding |
|
Well, it's read in at Descent3/Descent3/LoadLevel.cpp Line 1671 in 86a6c13 objp->roomnum = roomnum.
|
|
Yeah, but is that done for all objects, or is this specific object left out for some reason? By the way, I wrote a hacky testprogram (that's not really valid C, the compiler might optimize #include <stdio.h>
struct bla { char data[20]; };
struct bla blas[256*256];
int main()
{
void* p = &blas[0];
void* p2 = &blas[0x7FFFFFFF];
printf("p = %p p2 = %p diff = %td\n", p, p2, p2-p);
return 0;
}When compiled for 32bit ( So my theory that on 32bit platforms one lands 20 bytes before the array seems to hold (didn't test this on Windows with MSVC though - I would expect it to behave the same, but I always forget if some platforms treat pointers as signed or some stupid shit like that) |
sound_seg was -1, which crashes in BOA_INDEX(sound_seg) on 64bit platforms (by pure luck it doesn't seem to crash on 32bit platforms)
|
Created a pull request with the fix mentioned above: #158 |
|
I did some digging around and we are loading objects with negative room numbers. From the training mission: Retribution level 1 has lots of them: (type 7 is a Use this patch along with #157 (once it lands) to see these outputs: index 6434fbd..31361b7 100644
--- a/Descent3/LoadLevel.cpp
+++ b/Descent3/LoadLevel.cpp
@@ -1,5 +1,5 @@
/*
-* Descent 3
+* Descent 3
* Copyright (C) 2024 Parallax Software
*
* This program is free software: you can redistribute it and/or modify
@@ -1572,7 +1572,7 @@ void ConvertObject(int *type, int *id) {
// returns 1 if read ok
int ReadObject(CFILE *ifile, object *objp, int handle, int fileversion) {
int type, id, old_id, i;
- int roomnum;
+ int roomnum = 0;
float door_shields;
char tempname[OBJ_NAME_LEN + 1] = "";
@@ -1650,6 +1650,7 @@ int ReadObject(CFILE *ifile, object *objp, int handle, int fileversion) {
// Get the room number
roomnum = cf_ReadInt(ifile);
+ mprintf((0, "Loaded Object %d type %d in room %d\n", id, type, roomnum));
// For old files, check if this object is on terrain
if (fileversion < 49) { |
|
I think negative room numbers are ok, that encodes if it's an (indoor) room or an (outdoor) cell, as far as I can tell, see ROOMNUM_OUTSIDE(), ROOMNUM_CELLNUM_FLAG (which is 0x80000000 = -2147483648 aka INT32_MIN) etc in Descent3/object_external.h But -1 specifically apparently means "uninitialized", and |
|
This is the problem I was investigating last weekend, and yes inside rooms are positive, and negative are outside except for the case of -1 which means the object has no room assigned, unlinked. I couldn't figure out why in a 64-bit build, unlinked objects are being passed around, so all I could do it make the sanity check to prevent crash when unlinking an object that has already been unlinked. I cannot also play level 2, as the first door just doesn't open for me, it is a sign things are not properly setup. This happens when loading a demo file, and I know the demo file hasn't changed in a long time, so it confirms this strange bug causes chain of strange things to happen. |
|
Seems we could do with both a sanity check for unlinked objects and some additional logging for objects as they're loaded, which will help us diagnose problems with incorrect room assignments. |
|
Doing some more digging and the issue is that Commenting out the bt: patch: index 09b16ec..0bdea95 100644
--- a/Descent3/AImain.cpp
+++ b/Descent3/AImain.cpp
@@ -1,5 +1,5 @@
/*
-* Descent 3
+* Descent 3
* Copyright (C) 2024 Parallax Software
*
* This program is free software: you can redistribute it and/or modify
@@ -3738,8 +3738,8 @@ bool AIInit(object *obj, ubyte ai_class, ubyte ai_type, ubyte ai_movement) {
// Accounts for sound loading
if ((obj->handle & HANDLE_COUNT_MASK) != 0) {
ai_info->last_played_sound_index = Object_info[obj->id].anim[ai_movement].elem[anim].anim_sound_index;
- ai_info->anim_sound_handle = Sound_system.Play3dSound(
- Object_info[obj->id].anim[ai_movement].elem[anim].anim_sound_index, SND_PRIORITY_LOW, obj);
+ // ai_info->anim_sound_handle = Sound_system.Play3dSound(
+ // Object_info[obj->id].anim[ai_movement].elem[anim].anim_sound_index, SND_PRIORITY_LOW, obj);
} else {
ai_info->last_played_sound_index = -1;
} |
Yeah probably be the best to have a modern logging system implemented, eg: debug, notice, warning, error, then use trace level messages on the whole level setup system to track every detail of what is really going on, then logs can be compared between windows and linux. |
So...imgui? |
|
Nothing related to graphics, just using a simple backend logging library like log4c, where you simply log some text with a chosen verbosity level, then there is usually a config file to control the loggers: logger for the command line, logger for file, etc. |
|
Console logging is available. See pr #157. Or add -DMONO to your cmake config. The problem doesn't appear to be incorrect room assignments either (see my comment above). It's that the sound system is trying to play a sound before the object is fully initialized and assigned its room number. I think this is done to play looping sounds but I also can't figure out why this would work on windows and not linux/mac. |
|
I do understand there is some built in logging to the console functionality, but it would better using a robust logging system where things are organized by log level and category. With all these level script going on, and level setup, be nice to turn on trace logs to see what in detail is happening like which source file and function is doing what, and timestamps too. When people file a bug ticket, they can give us better logging information. The built in logger only offers 2 levels and it only logs to console. |
I commented out calls to compute play info, and see issues with the in-game scripting, not even guidebot wants to come out, so wondering if there are other wacky memory accesses going on that just works out for windows. |
If you apply the patch from #160 does guidebot work for you? It fixes the scripts on aarch64 for me. |
|
Yes it does, I get the intro animation now too, and the door opens. I didn't get the guidebot menu at all before. |
What do you mean exactly with pushes onto the sound buffer? Descent3/sndlib/hlsoundlib.cpp Lines 1157 to 1160 in 86a6c13 If Or is some other global state also changed that is not undone? (Short circuiting it probably still is a good idea, because why do all that work if we already know in the first Play3dSound() that we have no valid roomnum..) |
That's what I mean yes. If I'm reading that bit of code correctly (no guarantees I am) it only stops sounds that aren't audible AND aren't looping ( Short circuiting the |
sound_seg was -1, which crashes in BOA_INDEX(sound_seg) on 64bit platforms (by pure luck it doesn't seem to crash on 32bit platforms) It makes sense to catch this problem much earlier in Play3dSound(), to avoid executing all the superfluous
|
You're right, I guess it could happen that we have a looping sound there that's neither properly initialized nor stopped. I just updated my PR to add an early out, see https://github.com/DescentDevelopers/Descent3/pull/158/files |
|
@DanielGibson Are you in the DDN Discord by any chance? |
|
I am now (as caedes), can't promise I'll be too active though |
|
We are in the same boat as life is busy. |
|
I think that issue may be solved with these changes --- a/Descent3/terrain.h
+++ b/Descent3/terrain.h
@@ -213,7 +213,7 @@ extern terrain_tex_segment Terrain_tex_seg[TERRAIN_TEX_WIDTH * TERRAIN_TEX_DEPTH
// first object to render after cell has been rendered (only used for SW renderer)
extern short Terrain_seg_render_objs[];
-#define TERRAIN_REGION(x) ((Terrain_seg[0x7FFFFFFF & x].flags & TFM_REGION_MASK) >> 5)
+#define TERRAIN_REGION(x) ((Terrain_seg[0x7FFF & x].flags & TFM_REGION_MASK) >> 5)
extern terrain_sky Terrain_sky; |
|
Why would that be a solution? That just turns -1 into 0x7FFF, which might be a valid index within the array (and thus wouldn't crash), but that element of the array is still totally unrelated to roomnum -1 which simply is invalid/unassigned/unlinked |
|
just to make note here: the size of the terrain grid is based on 2 defined constants: So I don't think a fixed mask makes sense. |
|
Yes, that's the other side: Terrain_seg[] has 65536 elements, 0x7FFF is 32767, so with the "fix" suggested by winterheart, TERRAIN_REGION(x) would stop working for all indices between 32768 and 65536. Anyway, room number -1 is a special value with the special meaning of "unassigned" and thus has no corresponding entry in |
|
OK, here some explanations. Limits of C array indexes is In code #define ROOMNUM_CELLNUM_FLAG 0x80000000
#define ROOMNUM_CELLNUM_MASK 0x7fffffffis INT_MAX and PTRDIFF_MAX for 32-bit systems. So, to solve this issue, we need just check objp->roomnum to bound limits (0, 65535) and return zero if we out of limits like this: int BOA_INDEX(int x) {
if ((x < 0) || (x > (TERRAIN_WIDTH * TERRAIN_DEPTH))) {
return (Highest_room_index + 1);
}
else {
return x;
}
} |
|
How do you manage to have such a high density of wrong information, are you an LLM?
No. ptrdiff_t is a signed type (so on 32bit systems PTRDIFF_MAX is 0x7FFFFFFF, around 2 million) whoile size_t is unsigned (so on 32bit systems the maximum value, SIZE_MAX, is 0xFFFFFFFF, around 4 million). INT_MAX is the maximum value of
no, it's 0x7FFFFFFFFFFFFFFF, which is (2^63-1)
That arr[PTRDIFF_MAX] ending up being equivalent to arr[-1] is not universally true - it's only true if the element size is a multiple of 2. Furthermore,
No, returning 0 doesn't make it right. If anything, BOA_INDEX() should return -1 when called with -1, because some other functions (like Furthermore, the code you posted doesn't even do what you wrote above (really, are you an LLM?!), and even apart from that is not equivalent to what the BOA_INDEX macro does: |
|
If I were to turn the BOA_INDEX() macro into a function (and I'm not saying it should be done!), I'd turn it into something like this (untested): inline int BOA_INDEX(int x) {
// x == -1 should always be handled before calling BOA_INDEX()
if(x == -1) {
// in debug builds, make x == -1 exit D3 with an error,
// so the problem becomes obvious and the surrounding code can be fixed
ASSERT(x != -1 && "Make sure not to call BOA_INDEX() with -1!");
// otherwise (in release builds), return -1 so hopefully
// the code that uses the result of this function detects
// the invalid value and handles it accordingly.
return -1;
}
if(ROOMNUM_OUTSIDE(x)) {
ASSERT((0x7FFFFFFF & x) < sizeof(Terrain_seg)/sizeof(Terrain_seg[0]
&& "x negative, but not a valid index for Terrain_seg[]!");
return TERRAIN_REGION(x) + Highest_room_index + 1;
}
return x;
} |
|
Daniel, I've tried this case on 32-bit and 64-bit mode, and this is hows code works. If you not comfortable with my explanations, you can attribute this to the fact that before this post I was engaged in continuous debugging until 4 AM, which I could not have done. Still, -1 is valid value for inline int TERRAIN_REGION(int x) {
if (x == -1) return 0;
return ((Terrain_seg[0x7FFFFFFF & x].flags & TFM_REGION_MASK) >> 5);
} |
|
As for your variant, |
|
Looks to me that returning |
|
Which part of "that code was always buggy" did you not understand? The goal is not to emulate the broken behavior on 32bit (that just by chance happened to not crash), but to fix the code (even if it appeared to work on 32bit so far, there is no guarantee that it doesn't break with other compilers or whatever - or that it didn't cause actual bugs that were just more subtle than crashes). And fixing means handling index -1 as one should according to its meaning of "unassigned value", which does not mean "just use whatever is at index 0". |
|
Daniel, please calm your tone. Right after assigning sound_seg and ear_seg in code already has correctness check of values: if (!BOA_IsSoundAudible(sound_seg, ear_seg))
return false;If there incorrect data (and |
As far as I can tell from
and thus, so is this one. |
Fix crash in hlsSystem::ComputePlayInfo(), #141
sound_seg was -1, which crashes in BOA_INDEX(sound_seg) on 64bit platforms (by pure luck it doesn't seem to crash on 32bit platforms) It makes sense to catch this problem much earlier in Play3dSound(), to avoid executing all the superfluous
Fix crash in hlsSystem::ComputePlayInfo(), #141
Build Version
1fd8876
Operating System Environment
CPU Environment
Game Environment
Start a new campaign on any level for Descent 3: Retribution
Description
After starting a mission, you wait at the loading screen, then the game crashes
Regression Status
Present since first commit
Steps to Reproduce
Just start playing any level from Descent 3: Retribution, then it crashes on the loading screen before the actual gameplay starts.
Here are more details provided by GDB:
Thread 1 "Descent3" received signal SIGSEGV, Segmentation fault. 0x000000000067747e in hlsSystem::ComputePlayInfo (this=0x1339f20 <Sound_system>, sound_obj_index=1, virtual_pos=0x7fffffffbd70, virtual_vel=0x7fffffffbd64, adjusted_volume=0x7fffffffbd60) at /home/bperris/base/dev/Descent3/sndlib/hlsoundlib.cpp:925 925 sound_seg = BOA_INDEX(sound_seg); Missing separate debuginfos, use: dnf debuginfo-install xorg-x11-drv-nvidia-libs-550.67-1.fc39.x86_64The text was updated successfully, but these errors were encountered: