bugFreeciv - Bugs: bug #20498, Pirate archers alone in the water

 
 
Show feedback again

bug #20498: Pirate archers alone in the water

Submitted by:  Marko Lindqvist <cazfi>
Submitted on:  Mon 11 Feb 2013 04:22:42 AM UTC  
 
Category: generalSeverity: 3 - Normal
Priority: 5 - NormalStatus: Fixed
Assigned to: Marko Lindqvist <cazfi>Open/Closed: Closed
Release: Operating System: None
Planned Release: 2.4.0, 2.5.0

Add a New Comment (Rich MarkupRich Markup):
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Sun 07 Apr 2013 06:19:55 AM UTC, comment #16:

> the only known user-visible change here is to fix the unloading
> in assert-less release builds?


Yes

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Sat 06 Apr 2013 06:59:35 PM UTC, comment #15:

I'm still struggling to describe the change made here for NEWS.
Am I right in thinking that the original symptom was finally explained by bug #20699, and that the only known user-visible change here is to fix the unloading in assert-less release builds?

Jacob Nevins <jtn>
Project Administrator
Mon 01 Apr 2013 02:16:14 PM UTC, comment #14:

> Maybe so, I had no reproducible case to test against.


But now I had, and it got fixed almost accidentally by bug #20699 patch. When pirate ship is wiped (usually retired) and it has both undisbandable Barbarian Leader and men in it, and Leader gets teleported away (should this happen for barbarian leaders?), counter of units to drown gets subtrackted by one, but that unit then is not the Leader himself...

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Thu 14 Mar 2013 09:44:25 AM UTC, comment #13:

> unit.c:
> fc_assert(unit_transport_unload(pcargo)); ->
> unit_transport_unload(pcargo);

Quite right. I was indeed snooze.

Jacob Nevins <jtn>
Project Administrator
Thu 14 Mar 2013 07:27:18 AM UTC, comment #12:

> I can't actually see how unit_list_iterate could do the wrong
> thing here, where only the current unit can be removed.


Maybe so, I had no reproducible case to test against. But then, as far as I can see, unit.c assert can happen only if unit_transport_unload() fails to remove unit from the list -> list size > 0 even after unit_transport_unload() called for all units in it.

> I don't see anything in the committed change about moving
> functionality out of asserts


unit.c:
fc_assert(unit_transport_unload(pcargo)); ->
unit_transport_unload(pcargo);

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Thu 14 Mar 2013 01:10:51 AM UTC, comment #11:

Also, while this is a fine change, I can't actually see how unit_list_iterate could do the wrong thing here, where only the current unit can be removed. Perhaps I'm just being dim.
Has this change been observed to remove a symptom?

Another opportunity to prove I'm asleep: comment #5 says:

> 1) It's done in fc_assert() so won't do anything in release build

I don't see anything in the committed change about moving functionality out of asserts. Has this been dealt with? I can't spot the original problem. (This wasn't bug #20519, was it? -- but fix committed before the comment was made, and caused no bug anyway.)

Jacob Nevins <jtn>
Project Administrator
Wed 13 Mar 2013 11:54:47 PM UTC, comment #10:

I think the code had been this way since patch #2477, more or less. So it probably caused some form of trouble in 2.4.0-beta1?

Jacob Nevins <jtn>
Project Administrator
Tue 05 Mar 2013 07:34:31 AM UTC, SVN revision 22476:

Fixed iteration through units to be unloaded from transport.

See bug #20498

(Browse SVN revision 22476)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Tue 05 Mar 2013 07:34:26 AM UTC, SVN revision 22475:

Fixed iteration through units to be unloaded from transport.

See bug #20498

(Browse SVN revision 22475)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Wed 27 Feb 2013 11:57:06 AM UTC, comment #7:

Fix

(file #17363)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Wed 27 Feb 2013 11:38:23 AM UTC, comment #6:

> 2) It uses unit_list_iterate() when it should use
> unit_list_iterate_safe() as units are removed from the list
> during the iteration


...which gave me the idea that I should check other users of unit_transport_unload(), and indeed wipe_unit() has the same error. That one can explain all these problems, I think.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Wed 27 Feb 2013 11:26:39 AM UTC, comment #5:

> [13:14:57] in unit_virtual_destroy()
> [../../src/common/unit.c::1890]:
> assertion 'unit_list_size(punit->transporting) == 0' failed.


This at least indicates easy to spot bug. It might be separate issue, as I didn't see this failed assert as the first one but only later appearance.

Code above this assert tries to unload all units from transport, so soemthing has to go wrong with it for the assert to fail. And indeed, there's two problems in unloading:
1) It's done in fc_assert() so won't do anything in release build
2) It uses unit_list_iterate() when it should use unit_list_iterate_safe() as units are removed from the list during the iteration

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Wed 27 Feb 2013 11:17:42 AM UTC, comment #4:

In a longish game I first got for a long time (many turns) every turn several:
in begin_phase() [../../src/server/srv_main.c::945]: assertion 'ptrans != ((void *)0)' failed

...and now, maybe related...

in unit_transport_unload() [../../src/common/unit.c::2272]: assertion 'same_pos(unit_tile(pcargo), unit_tile(ptrans))' failed.
[13:14:57] Please report this message at http://gna.org/projects/freeciv/
[13:14:57] in unit_virtual_destroy() [../../src/common/unit.c::1890]: assertion 'unit_list_size(punit->transporting) == 0' failed.
[13:14:57] Please report this message at http://gna.org/projects/freeciv/
[13:14:57] in wipe_unit() [../../src/server/unittools.c::1767]: assertion 'drowning == 0' failed.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Sun 17 Feb 2013 08:36:59 AM UTC, comment #3:

Now I no longer have seen that... there's client side problems while observing, but those are not so critical.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Mon 11 Feb 2013 03:38:39 PM UTC, comment #2:

I've seen this in TRUNK only, but as it started after recursive transport wiping fixes went in, I'm quite sure that S2_4 too is affected. Don't want regression like that to beta2.

There was another transport fix that went to S2_3 too, but I hope it's not the faulty one, and we can release 2.3.4 as planned next weekend.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Mon 11 Feb 2013 04:42:27 AM UTC, comment #1:

I don't see how it could explain sanity check failures with rules in use, but barbarian creation just creates a ferry and then units inside, without checking if they are suitable cargo for the ferry. Creation of the units try to load them to ferry with 'force' being FALSE, so they would end failing the loading to ferry.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Mon 11 Feb 2013 04:22:42 AM UTC, original submission:

I've lately got "ptrans != NULL" sanity failures. One time I managed to debug it down to the fact it was Pirate Archer in Deep Ocean tile. The unit was already several turns old when sanity check first failed, so it was not just spawned there without ferry.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.

 

(Note: upload size limit is set to 1024 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

Attached Files
file #17363:  WipeSafe.patch added by cazfi (2kB - text/x-diff)

 

Depends on the following items: None found

Digest:
   bug dependencies.

 

Carbon-Copy List
  • -unavailable- added by jtn (Posted a comment)
  • -unavailable- added by cazfi (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 9 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Sun 07 Apr 2013 10:09:51 AM UTCjtnDependencies-=>bugs #20722 is dependent
    Tue 05 Mar 2013 07:34:44 AM UTCcazfiStatusReady For Test=>Fixed
      Assigned toNone=>cazfi
      Open/ClosedOpen=>Closed
    Wed 27 Feb 2013 11:57:06 AM UTCcazfiAttached File-=>Added WipeSafe.patch, #17363
      CategoryNone=>general
      StatusNone=>Ready For Test
    Sun 17 Feb 2013 08:36:59 AM UTCcazfiPlanned Release2.4.0-beta2, 2.5.0=>2.4.0, 2.5.0
    Mon 11 Feb 2013 03:38:39 PM UTCcazfiPlanned Release=>2.4.0-beta2, 2.5.0
    Show feedback again

    Back to the top


    Powered by Savane 3.1-cleanup