12 Nov, 2012, Rarva.Riendf wrote in the 1st comment:
Votes: 0
This code crashes on line number 94 when it tries to access ( *SECT_TABLE[sector_type])[roomIndex])
The thing is that it happens for values that were previously working fine, so obviously the memory has been overwritten between two calls.
So either I have a bug somewhere else, or I did not allocated memory like I should (I think it is the second solution (and hope it is as a matter of fact))

int **SECT_TABLE[SECT_MAX];
int SECT_NBROOMS[SECT_MAX];

void init() {
int i;
for (i = 0;i < SECT_MAX; ++i) {
SECT_NBROOMS[i] = 0;
SECT_TABLE[i] = NULL;
}
}

int *SECT_INSIDE_TABLE = NULL;
int *SECT_CITY_TABLE = NULL;
int *SECT_FIELD_TABLE = NULL;
int *SECT_FOREST_TABLE = NULL;
int *SECT_HILLS_TABLE = NULL;
int *SECT_MOUNTAIN_TABLE = NULL;
int *SECT_WATER_SWIM_TABLE = NULL;
int *SECT_WATER_NOSWIM_TABLE = NULL;
int *SECT_AIR_TABLE = NULL;
int *SECT_DESERT_TABLE = NULL;
int *SECT_FIRE_TABLE = NULL;
int *SECT_SUBZERO_TABLE = NULL;
int *SECT_TORNADO_TABLE = NULL;
int *SECT_QUICKSAND_TABLE = NULL;
int *SECT_ACID_TABLE = NULL;
int *SECT_UWATER_TABLE = NULL;
bool_t initDone = FALSE;

ROOM_INDEX_DATA *get_random_room_sector(short int sector_type) {

ROOM_INDEX_DATA *room = NULL;
int vnum = 0;
//if not already done, build the table for the sector asked
if ( !initDone) {
initDone = TRUE;
int tempSectRoom[SECT_MAX];
for (vnum = 0;vnum < SECT_MAX; vnum++)
tempSectRoom[vnum] = SECT_NBROOMS[vnum];
if (SECT_NBROOMS[SECT_INSIDE] > 0) SECT_INSIDE_TABLE = (int*)malloc(SECT_NBROOMS[SECT_INSIDE] * sizeof(int));
if (SECT_NBROOMS[SECT_CITY] > 0) SECT_CITY_TABLE = (int*)malloc(SECT_NBROOMS[SECT_CITY] * sizeof(int));
if (SECT_NBROOMS[SECT_FIELD] > 0) SECT_FIELD_TABLE = (int*)malloc(SECT_NBROOMS[SECT_FIELD] * sizeof(int));
if (SECT_NBROOMS[SECT_FOREST] > 0) SECT_FOREST_TABLE = (int*)malloc(SECT_NBROOMS[SECT_FOREST] * sizeof(int));
if (SECT_NBROOMS[SECT_HILLS] > 0) SECT_HILLS_TABLE = (int*)malloc(SECT_NBROOMS[SECT_HILLS] * sizeof(int));
if (SECT_NBROOMS[SECT_MOUNTAIN] > 0) SECT_MOUNTAIN_TABLE = (int*)malloc(SECT_NBROOMS[SECT_MOUNTAIN] * sizeof(int));
if (SECT_NBROOMS[SECT_WATER_SWIM] > 0) SECT_WATER_SWIM_TABLE = (int*)malloc(SECT_NBROOMS[SECT_WATER_SWIM] * sizeof(int));
if (SECT_NBROOMS[SECT_WATER_NOSWIM] > 0) SECT_WATER_NOSWIM_TABLE = (int*)malloc(SECT_NBROOMS[SECT_WATER_NOSWIM] * sizeof(int));
if (SECT_NBROOMS[SECT_AIR] > 0) SECT_AIR_TABLE = (int*)malloc(SECT_NBROOMS[SECT_AIR] * sizeof(int));
if (SECT_NBROOMS[SECT_DESERT] > 0) SECT_DESERT_TABLE = (int*)malloc(SECT_NBROOMS[SECT_DESERT] * sizeof(int));
if (SECT_NBROOMS[SECT_FIRE] > 0) SECT_FIRE_TABLE = (int*)malloc(SECT_NBROOMS[SECT_FIRE] * sizeof(int));
if (SECT_NBROOMS[SECT_SUBZERO] > 0) SECT_SUBZERO_TABLE = (int*)malloc(SECT_NBROOMS[SECT_SUBZERO] * sizeof(int));
if (SECT_NBROOMS[SECT_TORNADO] > 0) SECT_TORNADO_TABLE = (int*)malloc(SECT_NBROOMS[SECT_TORNADO] * sizeof(int));
if (SECT_NBROOMS[SECT_QUICKSAND] > 0) SECT_QUICKSAND_TABLE = (int*)malloc(SECT_NBROOMS[SECT_QUICKSAND] * sizeof(int));
if (SECT_NBROOMS[SECT_ACID] > 0) SECT_ACID_TABLE = (int*)malloc(SECT_NBROOMS[SECT_ACID] * sizeof(int));
if (SECT_NBROOMS[SECT_UWATER] > 0) SECT_UWATER_TABLE = (int*)malloc(SECT_NBROOMS[SECT_UWATER] * sizeof(int));

SECT_TABLE[SECT_INSIDE] = SECT_INSIDE_TABLE ? &SECT_INSIDE_TABLE : NULL;
SECT_TABLE[SECT_CITY] = SECT_CITY_TABLE ? &SECT_CITY_TABLE : NULL;
SECT_TABLE[SECT_FIELD] = SECT_FIELD_TABLE ? &SECT_FIELD_TABLE : NULL;
SECT_TABLE[SECT_FOREST] = SECT_FOREST_TABLE ? &SECT_FOREST_TABLE : NULL;
SECT_TABLE[SECT_HILLS] = SECT_HILLS_TABLE ? &SECT_HILLS_TABLE : NULL;
SECT_TABLE[SECT_MOUNTAIN] = SECT_MOUNTAIN_TABLE ? &SECT_MOUNTAIN_TABLE : NULL;
SECT_TABLE[SECT_WATER_SWIM] = SECT_WATER_SWIM_TABLE ? &SECT_WATER_SWIM_TABLE : NULL;
SECT_TABLE[SECT_WATER_NOSWIM] = SECT_WATER_NOSWIM_TABLE ? &SECT_WATER_NOSWIM_TABLE : NULL;
SECT_TABLE[SECT_AIR] = SECT_AIR_TABLE ? &SECT_AIR_TABLE : NULL;
SECT_TABLE[SECT_DESERT] = SECT_DESERT_TABLE ? &SECT_DESERT_TABLE : NULL;
SECT_TABLE[SECT_FIRE] = SECT_FIRE_TABLE ? &SECT_FIRE_TABLE : NULL;
SECT_TABLE[SECT_SUBZERO] = SECT_SUBZERO_TABLE ? &SECT_SUBZERO_TABLE : NULL;
SECT_TABLE[SECT_TORNADO] = SECT_TORNADO_TABLE ? &SECT_TORNADO_TABLE : NULL;
SECT_TABLE[SECT_QUICKSAND] = SECT_QUICKSAND_TABLE ? &SECT_QUICKSAND_TABLE : NULL;
SECT_TABLE[SECT_ACID] = SECT_ACID_TABLE ? &SECT_ACID_TABLE : NULL;
SECT_TABLE[SECT_UWATER] = SECT_UWATER_TABLE ? &SECT_UWATER_TABLE : NULL;

AREA_DATA *pArea;
for (pArea = area_first;pArea;pArea = pArea->next) {
for (vnum = pArea->lvnum;vnum <= pArea->uvnum;vnum ++) {
room = get_room_index(vnum);
if ( !room || room->sectorType == SECT_UNUSED || room->sectorType == SECT_UNUSED2
|| room->sectorType >= SECT_MAX)
continue;
tempSectRoom[room->sectorType] –;
( *SECT_TABLE[room->sectorType])[tempSectRoom[room->sectorType]] = vnum;

}
}
}
vnum = 3;
room = NULL;
while (vnum > 0) {//so it never goes in infinite loop (should not but but it depends on areas loaded)
vnum –;
long roomIndex = number_range(0, SECT_NBROOMS[sector_type] - 1);
if (SECT_NBROOMS[sector_type] > 0 ) {
bugf("sector:%d nb room:%d roomindex:%d", sector_type, SECT_NBROOMS[sector_type], roomIndex);
room = get_room_index(( *SECT_TABLE[sector_type])[roomIndex]);
}
if ( !room || !room->desc || strlen(room->desc) == 0)
continue;
if (IS_SET(room->room_flags, ROOM_NO_RANDOM) || IS_SET(room->room_flags,ROOM_LAW))
continue;
break;
}
return room;
}
13 Nov, 2012, quixadhal wrote in the 2nd comment:
Votes: 0
int **SECT_TABLE[SECT_MAX]; is probably not doing what you think it's doing.

If you're going to use multi-dimensional arrays like this, and dynamically allocate them, you shouldn't mix * and [] notation unless you're 100% sure what the compiler will do with it.

I would just declare it as ***SECT_TABLE; and then allocate in the order you intend to use them.

Of course, once you get this messy, you probably would be better off (for your own sanity) just declaring a new structure type and using that, rather than such a convoluted tangle of arrays.
13 Nov, 2012, Rarva.Riendf wrote in the 3rd comment:
Votes: 0
quixadhal said:
int **SECT_TABLE[SECT_MAX]; is probably not doing what you think it's doing.


Probably not but it seemed to work fine for my use (I took this example from the web).
Sure I could probably do it in another way but I also wish to understand why it does not work as well.


It is supposed to be a table of pointer that points to dynamically allocated arrays.

http://www.lemoda.net/c/cdecl/index.cgi?...
-> declare SECT_TABLE as array 10 of pointer to pointer to int seemed fine for what I intended to use.

Problem is that if I do not know why it bugs, I will not know if this code is really the problem as well. Meaning that if I change it and it works , I am not so sure I will have solved the real problem.
13 Nov, 2012, quixadhal wrote in the 4th comment:
Votes: 0
First off, you were using an extra level of pointers that isn't needed… perhaps you were originally storing room pointers instead of vnums?

I don't see anywhere that you're setting SECT_NBROOMSto a valid value. I assume you're reading this in from somewhere else?
Are you actually using the individual SECT_FOO_TABLE arrays anywhere, or just trying to make sense of how to do the allocations?

You should use calloc() instead of malloc()… ALWAYS… otherwise you have to zero-out the memory you're getting back yourself, and NOT doing that makes it almost impossible to debug.

It LOOKS like you're building an array of room vnums, divided up by terrain type. I'd suggest using linked lists for this, but if you insist on an array, this is probably closer to what you want:

int **SectorTable = NULL;
int *RoomCountBySector = NULL;

int *SectorTable_Inside = NULL;
int *SectorTable_City = NULL;
int *SectorTable_Field = NULL;
int *SectorTable_Forest = NULL;
int *SectorTable_Hills= NULL;
int *SectorTable_Mountain= NULL;
int *SectorTable_Water_swim= NULL;
int *SectorTable_Water_noswim= NULL;
int *SectorTable_Air= NULL;
int *SectorTable_Desert= NULL;
int *SectorTable_Fire= NULL;
int *SectorTable_Subzero= NULL;
int *SectorTable_Tornado= NULL;
int *SectorTable_Quicksand= NULL;
int *SectorTable_Acid= NULL;
int *SectorTable_Uwater= NULL;

bool_t initDone = FALSE;

void init( void ) {
int i;

SectorTable = (int **)calloc(SECT_MAX, sizeof(int *));
RoomCountBySector = (int *)calloc(SECT_MAX, sizeof(int));

/* Not needed, because calloc() already sets memory to zero-filled.
for( i = 0; i < SECT_MAX; i++ ) {
RoomCountBySector[i] = 0;
SectorTable[i] = NULL;
}
*/
}


ROOM_INDEX_DATA *get_random_room_sector(short int sector_type) {
ROOM_INDEX_DATA *room = NULL;
int vnum = 0;

//if not already done, build the table for the sector asked
if(!initDone) {
AREA_DATA *pArea = NULL;
int tempSectRoom[SECT_MAX];
for( vnum = 0; vnum < SECT_MAX; vnum++ )
tempSectRoom[vnum] = RoomCountBySector[vnum];

SectorTable[SECT_INSIDE] = RoomCountBySector[SECT_INSIDE] > 0 ? (int *)calloc(RoomCountBySector[SECT_INSIDE], sizeof(int)) : NULL;
SectorTable[SECT_CITY] = RoomCountBySector[SECT_CITY] > 0 ? (int *)calloc(RoomCountBySector[SECT_CITY], sizeof(int)) : NULL;
SectorTable[SECT_FIELD] = RoomCountBySector[SECT_FIELD] > 0 ? (int *)calloc(RoomCountBySector[SECT_FIELD], sizeof(int)) : NULL;
SectorTable[SECT_FOREST] = RoomCountBySector[SECT_FOREST] > 0 ? (int *)calloc(RoomCountBySector[SECT_FOREST], sizeof(int)) : NULL;
SectorTable[SECT_HILLS] = RoomCountBySector[SECT_HILLS] > 0 ? (int *)calloc(RoomCountBySector[SECT_HILLS], sizeof(int)) : NULL;
SectorTable[SECT_MOUNTAIN] = RoomCountBySector[SECT_MOUNTAIN] > 0 ? (int *)calloc(RoomCountBySector[SECT_MOUNTAIN], sizeof(int)) : NULL;
SectorTable[SECT_WATER_SWIM] = RoomCountBySector[SECT_WATER_SWIM] > 0 ? (int *)calloc(RoomCountBySector[SECT_WATER_SWIM], sizeof(int)) : NULL;
SectorTable[SECT_WATER_NOSWIM] = RoomCountBySector[SECT_WATER_NOSWIM] > 0 ? (int *)calloc(RoomCountBySector[SECT_WATER_NOSWIM], sizeof(int)) : NULL;
SectorTable[SECT_AIR] = RoomCountBySector[SECT_AIR] > 0 ? (int *)calloc(RoomCountBySector[SECT_AIR], sizeof(int)) : NULL;
SectorTable[SECT_DESERT] = RoomCountBySector[SECT_DESERT] > 0 ? (int *)calloc(RoomCountBySector[SECT_DESERT], sizeof(int)) : NULL;
SectorTable[SECT_FIRE] = RoomCountBySector[SECT_FIRE] > 0 ? (int *)calloc(RoomCountBySector[SECT_FIRE], sizeof(int)) : NULL;
SectorTable[SECT_SUBZERO] = RoomCountBySector[SECT_SUBZERO] > 0 ? (int *)calloc(RoomCountBySector[SECT_SUBZERO], sizeof(int)) : NULL;
SectorTable[SECT_TORNADO] = RoomCountBySector[SECT_TORNADO] > 0 ? (int *)calloc(RoomCountBySector[SECT_TORNADO], sizeof(int)) : NULL;
SectorTable[SECT_QUICKSAND] = RoomCountBySector[SECT_QUICKSAND] > 0 ? (int *)calloc(RoomCountBySector[SECT_QUICKSAND], sizeof(int)) : NULL;
SectorTable[SECT_ACID] = RoomCountBySector[SECT_ACID] > 0 ? (int *)calloc(RoomCountBySector[SECT_ACID], sizeof(int)) : NULL;
SectorTable[SECT_UWATER] = RoomCountBySector[SECT_UWATER] > 0 ? (int *)calloc(RoomCountBySector[SECT_UWATER], sizeof(int)) : NULL;
// If you want the seperate tables, here's where you copy them….
SectorTable_Inside = SectorTable[SECT_INSIDE];
SectorTable_City = SectorTable[SECT_CITY];
SectorTable_Field = SectorTable[SECT_FIELD];
SectorTable_Forest = SectorTable[SECT_FOREST];
SectorTable_Hills = SectorTable[SECT_HILLS];
SectorTable_Mountain = SectorTable[SECT_MOUNTAIN];
SectorTable_Water_swim = SectorTable[SECT_WATER_SWIM];
SectorTable_Water_noswim = SectorTable[SECT_WATER_NOSWIM];
SectorTable_Air = SectorTable[SECT_AIR];
SectorTable_Desert = SectorTable[SECT_DESERT];
SectorTable_Fire = SectorTable[SECT_FIRE];
SectorTable_Subzero = SectorTable[SECT_SUBZERO];
SectorTable_Tornado = SectorTable[SECT_TORNADO];
SectorTable_Quicksand = SectorTable[SECT_QUICKSAND];
SectorTable_Acid = SectorTable[SECT_ACID];
SectorTable_Uwater = SectorTable[SECT_UWATER];

for( pArea = area_first; pArea; pArea = pArea->next ) {
for( vnum = pArea->lvnum; vnum <= pArea->uvnum; vnum++ ) {
room = get_room_index(vnum);
if( !room ||
room->sectorType == SECT_UNUSED ||
room->sectorType == SECT_UNUSED2 ||
room->sectorType >= SECT_MAX
)
continue;
tempSectRoom[room->sectorType]–;
SectorTable[room->sectorType][tempSectRoom[room->sectorType]] = vnum;
}
}
}
vnum = 3;
room = NULL;
while( vnum > 0 ) {//so it never goes in infinite loop (should not but but it depends on areas loaded)
long roomIndex;
vnum–;
roomIndex = number_range(0, RoomCountBySector[sector_type] - 1);
if( RoomCountBySector[sector_type] > 0 ) {
bugf("sector:%d nb room:%d roomindex:%d", sector_type, RoomCountBySector[sector_type], roomIndex);
room = get_room_index(SectorTable[sector_type][roomIndex]);
}
if ( !room || !room->desc || strlen(room->desc) == 0)
continue;
if (IS_SET(room->room_flags, ROOM_NO_RANDOM) || IS_SET(room->room_flags,ROOM_LAW))
continue;
break;
}
return room;
}


I renamed some things, because trying to read so many ALL_CAPS variables/constants gives me a headache….
This is, of course, not tested… but look at the differences in how the pointers are used and see if it makes sense.
13 Nov, 2012, Rarva.Riendf wrote in the 5th comment:
Votes: 0
Quote
I don't see anywhere that you're setting SECT_NBROOMS to a valid value.

Yep did not put this code, those are just int and setted elsewhere, so did not really mattered. I just put a false init to show how I initialized values.

Quote
I renamed some things, because trying to read so many ALL_CAPS variables/constants gives me a headache….

I can understand :) but I use all caps for stuff that is considered constant (I do not alter those value in any way once they have been setted.)

Quote
You should use calloc() instead of malloc()… ALWAYS… otherwise you have to zero-out the memory you're getting back yourself, and NOT doing that makes it almost impossible to debug.

Does the compiler not warns you about use of unitialized value in arrays ?(I always compile with all warning on). But don't worry I know about that. So it definitely was not the problem.

Quote
This is, of course, not tested… but look at the differences in how the pointers are used and see if it makes sense.

Well the only differences I see is you removed one array level in sectortable and use * instead of [] for roomcountby sector , will implement it that way and see if it still crashes.

Quote
First off, you were using an extra level of pointers that isn't needed… perhaps you were originally storing room pointers instead of vnums?

Now that you mention it, that was probably the case when I thought of this code.

and yes the point is to get an easy access to all vnum of room that has a perticular sector.

Thanx for the help. will test that now and report if the problem was indeed this extra level of pointer
13 Nov, 2012, Rarva.Riendf wrote in the 6th comment:
Votes: 0
Seems to work perfectly fine now, and now that I look at the code I really dont see why I do not store the room pointer directly.
My guess is I started to do it (hence the table in **SECT_TABLE[SECT_MAX]), but forgot to finish it, and my brain got stuck on this declaration being fine with the rest of the code.

I just got unlucky as it could run for an months without crashing at all (I only trigger the crash when I use a small area list for test purpose)
(and Valgrind does not complain anymore as well when I free the sector tables as well)

Thanx a lot! and code is down to that, as I inded do not use any of the individual table as well…

ROOM_INDEX_DATA *get_random_room_sector(short int sector_type) {

ROOM_INDEX_DATA *room = NULL;
int vnum = 0;
//if not already done, build the table for the sector asked
if ( !initDone) {
SECT_TABLE = (int **)calloc(SECT_MAX, sizeof(int *));
initDone = TRUE;
int tempSectRoom[SECT_MAX];
for (vnum = 0;vnum < SECT_MAX; vnum++) {
tempSectRoom[vnum] = SECT_NBROOMS[vnum];
SECT_TABLE[vnum] = SECT_NBROOMS[vnum] > 0 ? (int*)calloc(SECT_NBROOMS[vnum] , sizeof(int)) : NULL;
}

AREA_DATA *pArea;
for (pArea = area_first;pArea;pArea = pArea->next) {
for (vnum = pArea->lvnum;vnum <= pArea->uvnum;vnum ++) {
room = get_room_index(vnum);
if ( !room || room->sectorType == SECT_UNUSED || room->sectorType == SECT_UNUSED2
|| room->sectorType >= SECT_MAX)
continue;
tempSectRoom[room->sectorType] –;
SECT_TABLE[room->sectorType][tempSectRoom[room->sectorType]] = vnum;

}
}
}
vnum = 3;
room = NULL;
while (vnum > 0) {//so it never goes in infinite loop (should not but but it depends on areas loaded)
vnum –;
long roomIndex = number_range(0, SECT_NBROOMS[sector_type] - 1);
if (SECT_NBROOMS[sector_type] > 0 )
room = get_room_index(SECT_TABLE[sector_type][roomIndex]);
if ( !room || !room->desc || strlen(room->desc) == 0)
continue;
if (IS_SET(room->room_flags, ROOM_NO_RANDOM) || IS_SET(room->room_flags,ROOM_LAW))
continue;
break;
}
return room;
}
13 Nov, 2012, quixadhal wrote in the 7th comment:
Votes: 0
Cool, glad it helped. I know sometimes just having another set of eyes makes you think of something or spot something that you've stared at for months. :)
0.0/7