Login
User Name:

Password:



Register
Forgot your password?
Vote for Us!
auth_update crash
Dec 23, 2017, 10:15 pm
By Remcon
check_tumble
Dec 18, 2017, 7:21 pm
By Remcon
parse description bug
Dec 15, 2017, 10:08 pm
By Remcon
Couple bugs
Dec 12, 2017, 5:42 pm
By Remcon
Bug in disarm( )
Nov 12, 2017, 6:54 pm
By GatewaySysop
LoP 1.46
Author: Remcon
Submitted by: Remcon
LOP 1.45
Author: Remcon
Submitted by: Remcon
LOP Heroes Edition
Author: Vladaar
Submitted by: Vladaar
Heroes sound extras
Author: Vladaar
Submitted by: Vladaar
6Dragons 4.3
Author: Vladaar
Submitted by: Vladaar
Users Online
CommonCrawl, Sogou, Yahoo!

Members: 0
Guests: 3
Stats
Files
Topics
Posts
Members
Newest Member
478
3,708
19,242
612
Jacki72H
Today's Birthdays
There are no member birthdays today.
Related Links
» SmaugMuds.org » General » Coding » SWR Makeskills and obj std::l...
Forum Rules | Mark all | Recent Posts

SWR Makeskills and obj std::list
< Newer Topic :: Older Topic >

Pages:<< prev 1, 2 next >>
Post is unread #1 Aug 9, 2010, 8:42 am
Go to the top of the page
Go to the bottom of the page

Keirath
Magician
GroupMembers
Posts144
JoinedJan 24, 2008

I've finished the task of converting the object list to use the STL. For the char_data class, I added an std::list called carrying.
Well, when I use the make skills in SWR (makearmor, makeblade etc), the MUD is continually crashing. The first check of a persons inventory succeeds. However, after the timer is activated and comes back to complete the skill, it crashes on the second run through. Particularly, this line:
   for( iobj = ch->carrying.begin(  ); iobj != ch->carrying.end(  ); ++iobj )
   {
**       obj = *iobj;


I'm not sure whats going on - I do know if I change it to do a get_objtype function to return an object it works properly.
       
Post is unread #2 Aug 9, 2010, 10:52 am
Go to the top of the page
Go to the bottom of the page

Caius
Magician
GroupMembers
Posts132
JoinedJan 29, 2006

Are you altering the carrying list inside the body of that loop? Ie, add and/or remove objects.
       
Post is unread #3 Aug 9, 2010, 11:24 am
Go to the top of the page
Go to the bottom of the page

Keirath
Magician
GroupMembers
Posts144
JoinedJan 24, 2008

Yeah. But its crashing far before it gets to that point.
Am I supposed to make it more like?

   for( iobj = ch->carrying.begin(  ); iobj != ch->carrying.end(  ); )
   {
      obj = *iobj;
      ++iobj;
       
Post is unread #4 Aug 9, 2010, 11:36 am
Go to the top of the page
Go to the bottom of the page

David Haley
Sorcerer
GroupMembers
Posts903
JoinedJan 29, 2007

You say that it only crashes on the second run through -- what exactly do you mean? If you are modifying the list and try to use iterators again, bad things can happen. You might need to show more about what you're doing here.

The change you propose should not be necessary.
       
Post is unread #5 Aug 9, 2010, 11:41 am
Go to the top of the page
Go to the bottom of the page

Samson
Black Hand
GroupAdministrators
Posts3,639
JoinedJan 1, 2002

Actually that change he made is generally what has to be done with a sdt::list that removed members from the list. AFKMud has a few of those around for various lists, especially in the ones that free memory during shutdown.
       
Post is unread #6 Aug 9, 2010, 11:49 am
Go to the top of the page
Go to the bottom of the page

Keirath
Magician
GroupMembers
Posts144
JoinedJan 24, 2008

Alright, when you use a makeskill (ie makeblade) the iterator runs through and checks a players inventory. If it has all the needed components, it adds a timer to the character. When the timer comes up, it returns to the function and one of the first things it does is AGAIN run through and checks a players inventory. This time, it might remove items. Here is an example from makeblade.

It crashes at the line with the ** consistently.

   for( iobj = ch->carrying.begin(  ); iobj != ch->carrying.end(  ); ++iobj )
   {
**       obj = *iobj;

      if( obj->item_type == ITEM_TOOLKIT )
         checktool = TRUE;
      if( obj->item_type == ITEM_OVEN )
         checkoven = TRUE;
      if( obj->item_type == ITEM_DURASTEEL && checkdura == FALSE )
      {
         checkdura = TRUE;
         separate_obj( obj );
         obj_from_char( obj );
         extract_obj( obj );
      }
      if( obj->item_type == ITEM_BATTERY && checkbatt == FALSE )
      {
         charge = UMAX( 5, obj->value[0] );
         separate_obj( obj );
         obj_from_char( obj );
         extract_obj( obj );
         checkbatt = TRUE;
      }
   }

       
Post is unread #7 Aug 9, 2010, 12:20 pm
Go to the top of the page
Go to the bottom of the page

Samson
Black Hand
GroupAdministrators
Posts3,639
JoinedJan 1, 2002

You already had a solution that should have worked:
   for( iobj = ch->carrying.begin(  ); iobj != ch->carrying.end(  ); )
   {
      obj = *iobj;
      ++iobj;


If that's not doing it, something else is wrong.

This is the top part of the loop in char_check() for AFKMud, for example:
   for( ich = pclist.begin(  ); ich != pclist.end(  ); )
   {
      char_data *ch = *ich;
      ++ich;
       
Post is unread #8 Aug 9, 2010, 12:47 pm
Go to the top of the page
Go to the bottom of the page

Keirath
Magician
GroupMembers
Posts144
JoinedJan 24, 2008

That solution worked. I was just making sure that was the proper way. It didn't hit me until after Caius posted.
Thanks guys.
       
Post is unread #9 Aug 9, 2010, 2:52 pm
Go to the top of the page
Go to the bottom of the page

David Haley
Sorcerer
GroupMembers
Posts903
JoinedJan 29, 2007

Hence the importance of being precise :) If the list is not being modified, the change is not necessary.

Note also that your solution is not guaranteed not work in the general case; it works in the std::list case by accident of implementation details. It would not work with a vector, for example.

As a general rule, you really don't want to modify STL lists as you traverse them.
       
Post is unread #10 Aug 9, 2010, 3:22 pm
Go to the top of the page
Go to the bottom of the page

Keirath
Magician
GroupMembers
Posts144
JoinedJan 24, 2008

What would you suggest doing then?
       
Post is unread #11 Aug 9, 2010, 3:37 pm
Go to the top of the page
Go to the bottom of the page

David Haley
Sorcerer
GroupMembers
Posts903
JoinedJan 29, 2007

Create a copy of the list before iteration, or keep track of elements you'll want to remove and only remove them after you have finished iteration.

Either solution has ups and downs; a copy of the list has the issue of potentially iterating over killed objects (so you need to deal with validity checks somehow). A list of elements to remove has the issue of being hard to use across code scopes.

If you're removing the objects in basically the same place that you're iterating in, it's probably easier to do deferred removal from the list.
       
Post is unread #12 Aug 10, 2010, 1:46 pm   Last edited Aug 10, 2010, 2:52 pm by Keirath
Go to the top of the page
Go to the bottom of the page

Keirath
Magician
GroupMembers
Posts144
JoinedJan 24, 2008

Well, I've got that working for now. But now I have a new set of problems.

If I kill/slay a player (SWR has the perma-death deal), the MUD crashes immediately. I've added some log strings through the code between the raw_kill call and all the way through the clean extracted char data. GDB is showing me this:
#0  0x00000000004b9eaa in process_input () at comm.c:462
462	            if( d->character && d->character->wait > 0 )
(gdb) list
457	          }
458	            /* check for input from the dns */
459	            if( ( d->connected == CON_PLAYING || d->character != NULL ) && d->ifd != -1 && FD_ISSET( d->ifd, &in_set ) )
460	               process_dns( d );
461	
462	            if( d->character && d->character->wait > 0 )
463	            {
464	               --d->character->wait;
465	               continue;
466	            }
(gdb) bt
#0  0x00000000004b9eaa in process_input () at comm.c:462
#1  0x00000000004ba284 in game_loop () at comm.c:557
#2  0x00000000004b9144 in main (argc=5, argv=0x7ffff9772098) at comm.c:224


Process input is completely stock (aside from using dlist like AFK uses and again, like AFK being split off into its own function - AFK has kinda been where I'm teaching myself this stuff.)

I really am not sure whats going on here. Any ideas?

To further confuse and boggle matters, if you suicide a character it works fine. If you slay/murder one otherwise, not so.
       
Post is unread #13 Aug 10, 2010, 4:42 pm
Go to the top of the page
Go to the bottom of the page

Kayle
Off the Edge of the Map
GroupAdministrators
Posts1,195
JoinedMar 21, 2006

This is one of the reasons I've avoided mass conversion to std::list. You're going to come across issues like this whenever you remove something from a list while iterating over that list.

For the crashing with raw_kill for slay/murder. Can you post your raw_kill, process_input, and the relevant bits of do_slay?
       
Post is unread #14 Aug 10, 2010, 5:55 pm
Go to the top of the page
Go to the bottom of the page

Keirath
Magician
GroupMembers
Posts144
JoinedJan 24, 2008

Raw_kill
obj_data *raw_kill( char_data * ch, char_data * victim )
{
   char_data *victmp;
   obj_data *corpse_to_return;
   SHIP_DATA *ship;
   char buf[MAX_STRING_LENGTH];
   char buf2[MAX_STRING_LENGTH];
   char arg[MAX_STRING_LENGTH];

   if( !victim )
   {
      bug( "%s: null victim!", __FUNCTION__ );
      return NULL;
   }

   strcpy( arg, victim->name );
   stop_fighting( victim, TRUE );

   if( ch && !IS_NPC( ch ) && !IS_NPC( victim ) )
      claim_disintigration( ch, victim );

   /* Take care of polymorphed chars */
   if( IS_NPC( victim ) && IS_SET( victim->act, ACT_POLYMORPHED ) )
   {
      char_from_room( victim->desc->original );
      char_to_room( victim->desc->original, victim->in_room );
      victmp = victim->desc->original;
      do_revert( victim, "" );
      return raw_kill( ch, victmp );
   }

   if( victim->in_room && IS_NPC( victim ) && victim->vip_flags != 0 && victim->in_room->area
       && victim->in_room->area->planet )
   {
      victim->in_room->area->planet->population--;
      victim->in_room->area->planet->population = UMAX( victim->in_room->area->planet->population, 0 );
      victim->in_room->area->planet->pop_support -= ( float )( 1 + 1 / ( victim->in_room->area->planet->population + 1 ) );
      if( victim->in_room->area->planet->pop_support < -100 )
         victim->in_room->area->planet->pop_support = -100;
   }

   if( !IS_NPC( victim ) || !IS_SET( victim->act, ACT_NOKILL ) )
      mprog_death_trigger( ch, victim );
   if( char_died( victim ) )
      return NULL;

   if( !IS_NPC( victim ) || !IS_SET( victim->act, ACT_NOKILL ) )
      rprog_death_trigger( ch, victim );
   if( char_died( victim ) )
      return NULL;

   if( !IS_NPC( victim ) || ( !IS_SET( victim->act, ACT_NOKILL ) && !IS_SET( victim->act, ACT_NOCORPSE ) ) )
      corpse_to_return = make_corpse( victim, ch );
   else
   {
      std::list < obj_data * >::iterator iobj;
      for( iobj = victim->carrying.begin(  ); iobj != victim->carrying.end(  ); )
      {
         obj_data *obj = ( *iobj );
         ++iobj;
         obj_from_char( obj );
         extract_obj( obj );
      }
   }

   if( IS_NPC( victim ) )
   {
      log_string( "Definitely should not be seeing this shit" );
      victim->pIndexData->killed++;
      extract_char( victim, TRUE );
      victim = NULL;
      return corpse_to_return;
   }

   set_char_color( AT_DIEMSG, victim );
   do_help( victim, "_DIEMSG_" );

   /* swreality chnages begin here */

   for( ship = first_ship; ship; ship = ship->next )
   {
      if( !str_cmp( ship->owner, victim->name ) )
      {
         STRFREE( ship->owner );
         ship->owner = STRALLOC( "" );
         STRFREE( ship->pilot );
         ship->pilot = STRALLOC( "" );
         STRFREE( ship->copilot );
         ship->copilot = STRALLOC( "" );

         save_ship( ship );
      }
   }
   log_printf( "raw_kill victim %s", victim->name );
   if( victim->plr_home )
   {
      room_index_data *room = victim->plr_home;

      STRFREE( room->name );
      room->name = STRALLOC( "An Empty Apartment" );

      REMOVE_BIT( room->room_flags, ROOM_PLR_HOME );
      SET_BIT( room->room_flags, ROOM_EMPTY_HOME );

      fold_area( room->area, room->area->filename, FALSE );
   }

   if( victim->pcdata && victim->pcdata->clan )
   {
      if( !str_cmp( victim->name, victim->pcdata->clan->leader ) )
      {
         STRFREE( victim->pcdata->clan->leader );
         if( victim->pcdata->clan->number1 )
         {
            victim->pcdata->clan->leader = STRALLOC( victim->pcdata->clan->number1 );
            STRFREE( victim->pcdata->clan->number1 );
            victim->pcdata->clan->number1 = STRALLOC( "" );
         }
         else if( victim->pcdata->clan->number2 )
         {
            victim->pcdata->clan->leader = STRALLOC( victim->pcdata->clan->number2 );
            STRFREE( victim->pcdata->clan->number2 );
            victim->pcdata->clan->number2 = STRALLOC( "" );
         }
         else
            victim->pcdata->clan->leader = STRALLOC( "" );
      }

      if( !str_cmp( victim->name, victim->pcdata->clan->number1 ) )
      {
         STRFREE( victim->pcdata->clan->number1 );
         if( victim->pcdata->clan->number2 )
         {
            victim->pcdata->clan->number1 = STRALLOC( victim->pcdata->clan->number2 );
            STRFREE( victim->pcdata->clan->number2 );
            victim->pcdata->clan->number2 = STRALLOC( "" );
         }
         else
            victim->pcdata->clan->number1 = STRALLOC( "" );
      }

      if( !str_cmp( victim->name, victim->pcdata->clan->number2 ) )
      {
         STRFREE( victim->pcdata->clan->number2 );
         victim->pcdata->clan->number1 = STRALLOC( "" );
      }
      victim->pcdata->clan->members--;
   }

   if( !victim )
   {
      std::list < descriptor_data * >::iterator ds;
      descriptor_data *d = NULL;
      bool dfound = FALSE;

      /*
       * Make sure they aren't halfway logged in.
       */
      for( ds = dlist.begin(  ); ds != dlist.end(  ); ++ds )
      {
         d = *ds;
         if( ( victim = d->character ) && !IS_NPC( victim ) )
         {
            dfound = FALSE;
            break;
         }
       }
       if( dfound && d )
         close_socket( d, TRUE );
        log_string( "Null victim raw_kill" );
   }
   else
   {
      int x, y;

      quitting_char = victim;
      save_char_obj( victim );
      saving_char = NULL;
      extract_char( victim, TRUE );
      log_string( "Extract char call in raw_kilL" );
      for( x = 0; x < MAX_WEAR; x++ )
         for( y = 0; y < MAX_LAYERS; y++ )
            save_equipment[x][y] = NULL;
   }

   sprintf( buf, "%s%c/%s", PLAYER_DIR, tolower( arg[0] ), capitalize( arg ) );
   sprintf( buf2, "%s%c/%s", BACKUP_DIR, tolower( arg[0] ), capitalize( arg ) );

   rename( buf, buf2 );

   sprintf( buf, "%s%c/%s.clone", PLAYER_DIR, tolower( arg[0] ), capitalize( arg ) );
   sprintf( buf2, "%s%c/%s", PLAYER_DIR, tolower( arg[0] ), capitalize( arg ) );

   rename( buf, buf2 );

   return corpse_to_return;
}


Process_Input
void process_input( void )
{
   char cmdline[MAX_INPUT_LENGTH];
   std::list < descriptor_data * >::iterator ds;
   for( ds = dlist.begin(  ); ds != dlist.end(  ); )
   {
      descriptor_data *d = *ds;
      ++ds;

      ++d->idle;  /* make it so a descriptor can idle out */
      if( FD_ISSET( d->descriptor, &exc_set ) )
      {
         FD_CLR( d->descriptor, &in_set );
         FD_CLR( d->descriptor, &out_set );
         if( d->character && ( d->connected == CON_PLAYING || d->connected == CON_EDITING ) )
            save_char_obj( d->character );
         d->outtop = 0;
         close_socket( d, TRUE );
         continue;
      }
      else if( ( !d->character && d->idle > 360 )  /* 2 mins */
               || ( d->connected != CON_PLAYING && d->idle > 1200 )  /* 5 mins */
               || d->idle > 28800 ) /* 2 hrs  */
      {
         d->write( "Idle timeout... disconnecting.\r\n", 0 );
         d->outtop = 0;
         close_socket( d, TRUE );
         continue;
      }
      else
      {
         d->fcommand = FALSE;
         if( FD_ISSET( d->descriptor, &in_set ) )
         {
            d->idle = 0;
            if( d->character )
                d->character->timer = 0;
            if( !d->read_from(  ) )
            {
               FD_CLR( d->descriptor, &out_set );
               if( d->character && ( d->connected == CON_PLAYING || d->connected == CON_EDITING ) )
                  save_char_obj( d->character );
               d->outtop = 0;
               close_socket( d, FALSE );
               continue;
             }
          }
            /* check for input from the dns */
            if( ( d->connected == CON_PLAYING || d->character != NULL ) && d->ifd != -1 && FD_ISSET( d->ifd, &in_set ) )
               process_dns( d );

            if( d->character )
            {
               if( d->character->wait > 0 )
               {
                  log_printf( "character %s in process_input", d->character->name );
                 --d->character->wait;
                 continue;
               }
            }

            read_from_buffer( d );
            if( d->incomm[0] != '\0' )
            {
               d->fcommand = TRUE;
               stop_idling( d->character );
              strcpy( cmdline, d->incomm );
               d->incomm[0] = '\0';

               if( d->pagepoint )
                  set_pager_input( d, cmdline );
               else
                  switch ( d->connected )
                  {
                     default:
                        d->nanny( cmdline );
                        break;
                     case CON_PLAYING:
                        interpret( d->character, cmdline );
                        break;
                     case CON_EDITING:
                        edit_buffer( d->character, cmdline );
                        break;
                  }
            }
         }
      }
}


Slay merely calls raw_kill.
       
Post is unread #15 Aug 10, 2010, 9:41 pm
Go to the top of the page
Go to the bottom of the page

David Haley
Sorcerer
GroupMembers
Posts903
JoinedJan 29, 2007

You're probably running into the dangers I predicted earlier. The method you are using is not safe for iteration over a container that is being modified.

A simple example: you advance the iterator state, and then as you process the current character you kill the next character. The iterator now points to a removed object. Oops.
       
Post is unread #16 Aug 11, 2010, 4:20 am
Go to the top of the page
Go to the bottom of the page

Keirath
Magician
GroupMembers
Posts144
JoinedJan 24, 2008

Would it be better to make a removelist and remove it at the end of being processed?
       
Post is unread #17 Aug 11, 2010, 7:26 am
Go to the top of the page
Go to the bottom of the page

David Haley
Sorcerer
GroupMembers
Posts903
JoinedJan 29, 2007

Well, without knowing exactly what the issue is, it's hard to suggest a solution.

If you're deleting things forward in the iteration, a temporary list for removal won't work, nor will a temporary local copy.

You need a way to identify which elements are now bogus. You can do so in several ways; one is to not actually delete them until the end of the tick (when you know that you have completed all iterations). Another is to keep a list of deleted pointers so that as you iterate, you know to skip them.
       
Post is unread #18 Aug 11, 2010, 12:17 pm
Go to the top of the page
Go to the bottom of the page

Keirath
Magician
GroupMembers
Posts144
JoinedJan 24, 2008

Would it be better to just leave the stock LINK'd list system?
       
Post is unread #19 Aug 11, 2010, 1:21 pm
Go to the top of the page
Go to the bottom of the page

Samson
Black Hand
GroupAdministrators
Posts3,639
JoinedJan 1, 2002

It makes no difference. The stock handling of these issues was just as broken.

That said, there were never any significant number of issues in AFKMud after switching to std::list. Yes, there are still some that can break, but the number of lists that do so is less than it did with Smaug's own LINK/UNLINK system. I know the raw_kill function works, as does slaying, so perhaps there's something in yours that's sneaking in an extra bit that AFKMud didn't have?

The problem is, in order to fix it all properly, most of the way things are done needs to be rewritten. The amount of effort involved is probably not worth it in the end.
       
Post is unread #20 Aug 11, 2010, 1:22 pm
Go to the top of the page
Go to the bottom of the page

David Haley
Sorcerer
GroupMembers
Posts903
JoinedJan 29, 2007

As a general rule I try to avoid fixing things unless they're broken, so I would probably have left this particular bit alone myself.

But, well, really the thing to do here is to track down what exactly is going on. I'd try running valgrind to see if you're accessing deleted memory; use gdb to see exactly what's happening; etc. We already know that it appears to be accessing 'd' and failing, so the question is when/how 'd' became invalid. Valgrind can help you see exactly which line of code deleted it.
       
Pages:<< prev 1, 2 next >>