Arcbound Ravager and gnomefry's dump

[FastEddie]

Referring to the bug reported by gnomefry here (https://www.slightlymagic.net/forum/viewtopic.php?f=86&t=29888) and following drool66's suggestion I had a quick first look. Which left me baffled for the moment but maybe someone else can make something out of it.

Edit: after having a close look I decided to rewrite the post. This little beast comes in a variety of flavours, namely before attacking, after attacking, during the following turn or seemingly never...

The dump (dump and savegame attached) gives the following (based on the git as of 5-July-2020):

get_counter_type_by_id at F:\Manalink-Dev\src/functions/counters.c:30
special_mill at F:\Manalink-Dev\src/functions/deck.c:2023
pump_ability_until_eot_no_repeat at F:\Manalink-Dev\src/functions/functions.c:1258
?? ??:0

After some playing around I got the following dumps where the last one seemed to be stable (i.e. occured more than once):

new_damage_all at F:\Manalink-Dev\src/functions/manipulate_and_damage_all.c:286
calc_initial_attack_rating_by_iid at F:\Manalink-Dev\src/functions/deck.c:67
finalize_activation at F:\Manalink-Dev\src/functions/engine.c:1585
?? ??:0

new_damage_all at F:\Manalink-Dev\src/functions/manipulate_and_damage_all.c:286
calc_initial_attack_rating_by_iid at F:\Manalink-Dev\src/functions/deck.c:51
human_assign_blockers at F:\Manalink-Dev\src/functions/engine.c:1559
?? ??:0

show_backtrace at F:\Manalink-Dev\src/functions/show_backtrace.c:19
get_card_instance at F:\Manalink-Dev\src/functions/deck.c:309
recopy_card_onto_stack at F:\Manalink-Dev\src/functions/engine.c:1683
?? ??:0

The rest of the dumps refers to the exe I guess and I didn't yet follow up on the dmp.pl which should give some addresses.

Doing some tracing yielded the following

Code: Select all

engine.c | finalize_activation:1608 | Before dispatch_trigger2: player = 1, trig = 210
events.c | dispatch_trigger2:521 | Before dispatch_trigger_impl
events.c | dispatch_trigger2:526 | After dispatch_trigger_impl
engine.c | recopy_card_onto_stack:1692 | Entering recopy_card_onto_stack: a1 = 1, pos = -1


Trigger 210 is TRIGGER_TAP_CARD. The log above corresponds to the following pieces of code.

Code: Select all

// finalize_activation: around line 1608
  EXE_FN(void, 0x436740, void)();   // resolve_top_card_on_stack()

  dispatch_trigger2(current_turn, TRIGGER_TAP_CARD, EXE_STR(0x78fba8), 0, player, card);   // PROMPT_SPECIALFEPHASE[7]

// we never get back here


Code: Select all

//  dispatch_trigger2: around line 520
  trigger_cause_controller = new_trigger_cause_controller;
  trigger_cause = new_trigger_cause;

  LOG_DEBUG("Before dispatch_trigger_impl")
 
  dispatch_trigger_impl(  player, trig, 0, prompt, TENTATIVE_allow_response);
  dispatch_trigger_impl(1-player, trig, 0, prompt, TENTATIVE_allow_response);

  LOG_DEBUG("After dispatch_trigger_impl")
 
  trigger_cause = old_trigger_cause;
  trigger_cause_controller = old_trigger_cause_controller;
  xtrigger_impl_value_dont_use_directly = old_xtrigger;

  return 0; // ← I think we leave safely here


Code: Select all

//  recopy_card_onto_stack around line 1692
  int pos = stack_size - 1;

  LOG_DEBUG("Entering recopy_card_onto_stack: a1 = %d, pos = %d", a1, pos)
 
  card_instance_t* stack_inst = get_card_instance(stack_cards[pos].player, stack_cards[pos].card); // ← pos == -1 which means something hits the fan


Now I haven‘t looked into the order of function calls, namely where we end up after dispatch_trigger2 and when recopy_card_onto_stack is called but cleary someone is setting stack_size to 0 when we don‘t expect it.

If you have any hints more than happy, otherwise I will keep on digging tomorrow.

[drool66]

Using dmp.pl under Ubuntu for WSL I get:

edit: by the way, this has nothing to do with the cranial plating interaction - this is just AI with an Arcbound Ravager in hand. I can sometimes get AI to play Arcbound Ravager, but if I then put Cranial Plating in hand, I get a ctd with no dump.

[FastEddie]

Thanks. So far my impression is that tapping Arcbound Ravager breaks something. Iirc the bug didn't occur after combat, only before. Sacrificing artifacts never broke anything, at least when I tested.

A quick look showed no function that manipulates stack_size so there is probably an issue between activate() and finalize_activation().

I will try to hook up both the dll and the exe with gdb under MinGW. No luck with WSL today, I have no idea when the files are relocated and what the start address is so that I could set a breakpoint at the proper place (both files can be stored anywhere in memory, all addresses are relative to a starting point and right now I don't have a clue how to get this starting point without starting the program because then I cannot intervene anymore...).

[FastEddie]

A quick interim conslusion…
 
Lessons learned:

Always ask for a save file when tackling a dump as there might be different ways to get an errror
Macro expansion and comments don’t seem to mess up the code lines so…
…some dumps were weird in a way I don’t yet understand

 
My current working hypothesis is that the AI tries to sac something to Arcbound Ravager before attacking, cancels that action and thereby messes up the card stack, Let’s see.
 
I made a little progress thanks to Drool66’ hint to look at the wiki here: https://www.slightlymagic.net/wiki/Coding_Cards_in_C_Tutorial Manalink.lds is the linker script that tells the compiler (among other things) where to find the addresses for the global variables like stack_size (did you ever wonder how those two get matched ?). stack_size is located at 0x5057A8 which tells me where to look further.
 
Attack plan:
Assuming I get everything hooked up in gdb there are a couple of ways:

Wait for a crash and go back through the call stack to see what messed stack_size up
If that doesn’t work set a conditional breakpoint if 0x5057A8 == 0 but as the functions around activation are called a gazillion times this gives a ton of false positives
If I can’t get gdb running access 0x5057A8 from the DLL (there are functions for reading and writing into the exe but I didn’t look into them so far), sprinkle everything with logs and see where I end up

 
Another option is to convert the symbols in Manalink.dbg from DWARF format to PDB using cv2pdb (https://github.com/rainers/cv2pdb) and use WinDbg. As cv2pdb is originally written for the D programming language and no executable is easily available I didn’t follow up so far (compiling the code under WSL didn’t work as I probably need some version of Visual Studio). Should this work the steps from above apply.
 
Ruling out weird side effects:
@Drool66: can you do me a favour as you are faster implementing cards? I want to see the Arcbound Ravager code adjusted in a couple of ways, even if it is just for giggles.

Remove the Modular property (making it an Atog with counters)
Remove the Sac for +1/+1 counters property (making it an Arcbound Worker)
Remove both (make it an expensive Memnite)

 
Run the save state a couple of times after each adjustment to see whether the bug still happens.
Goal is to see whether one or both properties trigger the error. If it still occurs for the stripped down version we can attack from a different angle although I think this is rather unlikely.

Edit: MinGW64 or in my case Win-builds is the way to go. Attached to the running process like a charm and broke at card_arcbound_ravager as planned. Here we go...

[drool66]

Thank you very much, FastEddie. From what I've found it looks like there are two issues that are somewhat related, and both have to do with the AI.

The first is if you just put Arcbound Ravager (AR) in AI's hand, you will get a crash with dump bad parameters get_card_instance(2428861, 2428861)
Using ai_is_speculating I was able to isolate this to the AI considering sacrificing AR to itself. If you expand its functions and add if(ai_is_speculating && !is_this_dying(player, card)){ td.not_me = 1} to its sacrifice target function, this crash never comes up. This somewhat limits AR's function, but I think in the most limited way possible.

The other crash is when AI has both AR and Cranial Plating (CP), either can be in play or in hand, doesn't matter. This results in a ctd with no dump. Using the same ai_is_speculating I was able to solve this crash in all but one instance: when AI resolves equipping CP to AR. The way I did this was to have a whole section where if ai_is_speculating, it thinks CP adds +4/+0. I'm guessing the crash comes on RECALC_ALL on resolving the equip ability. I'm not sure, that's as far as I tested. I think adding an id_flag=DOESNT_MATCH for CP in AR's sacrifice function like I did above will work pretty well, too.

To answer your question:

@Drool66: can you do me a favour as you are faster implementing cards? I want to see the Arcbound Ravager code adjusted in a couple of ways, even if it is just for giggles.
Remove the Modular property (making it an Atog with counters)
Remove the Sac for +1/+1 counters property (making it an Arcbound Worker)
Remove both (make it an expensive Memnite)

Removing any one of AR's abilities will avert all of these crashes.

[FastEddie]

So I looked at bit more into it and got more confused...

Drool66, thanks for the analysis. This confirms that saccing is indeed the issue and not some weird memory bug (which I didn't suspect but given the erratic behaviour I wanted to be on the safe side).

The good news is that gdb worked as intended. It dropped me at the line causing the segmentation fault (accessing an array with index out of bound, in our case -1) and I could dig backwards. Unfortunately the calling functions are all in the exe and most are not mentioned in Magic-trace.c so I didn't follow up any further as the effort outweighs the benefit for the moment.

The (somewhat) bad news is that during testing the AI could sacrifice Arcbound Ravager to itself a couple of times and it worked. So the bug occurs often, but not always. The was the point were I decided to put that one aside for the moment unless I have the source code.

@Drool66: could you please put the following line in the code somewhere close to your workaround?

Code: Select all

#pragma message "Workaround to prevent a segmentation fault due to stack_size == 0 in recopy_card_onto_stack. Put on hold until magic.c is better understood."


The precise text doesn't matter. This is just to spit out a message at compile time and let people know that there is something that needs some love at some point in the future.

[FastEddie]

Some more data... attached is a save game with a crash dump that looks quite similar to what we have seen from Arcbound Ravager, but based on Mortarpod attached to a Stoneforge Mystic. The figures in get_card_instance look alike (I haven't checked the exact figures but it is something in the 23 million range, not the usual 0 or -1). Manalink version is 8-2020 a62926f.

Which gives more weight to the hypothesis that Arcbound Ravager is not the issue but that something is broken with the sacrifice creature mechanic.

Right now I have no time to look into this further. This post is just for documentation and later use as this will be on my agenda again at some point in the future.

[Korath]

TARGET_STATE_DYING (== TARGET_STATE_DESTROYED) isn't a legal bit for illegal_state in validate_target(). Even when it is legal - that is, in required_state - it doesn't mean anything at all similar to "require (instance->token_status & STATUS_DYING)" like this alias implies. So the select_card() call in new_sacrifice_impl() lets you pick an artifact that's already been destroyed but is still on the bf awaiting regeneration. That didn't matter until Arcbound Ravager started being activateable at such times. (I suspect because you started allowing arbitrary response during triggers for Stifle, as opposed to it becoming activateable in general during the regeneration phase.)

The AI sees it can activate the Ravager for free by selecting an artifact with STATUS_DYING already set (since kill_card(...KILL_SACRIFICE) doesn't do anything for those), so eventually responds to itself doing so enough times to fill the stack. The stack eventually unwinds, and recopy_card_onto_stack() crashes because the top of the stack is below its start.

I can't get this to crash anymore after adding if ((illegal_state & TARGET_STATE_DESTROYED) && (tgt_instance->token_status & STATUS_DYING)) FAILURE(",dying"); to validate_target_impl(). But don't do that, because that's not what TARGET_STATE_DESTROYED means. The clean fix is to ASSERT(!(illegal_state & TARGET_STATE_DESTROYED)) in validate_target_impl(), at least until something explicitly needs to select an object that can't regenerate; grep for every use of TARGET_STATE_DYING and change them to TARGET_STATE_DESTROYED; and grep for every attempt to put it into illegal_state and move the intended check for ~STATUS_DYING into the auxiliary targeting function, since most uses already have one.

The instrumentation I needed to track this down is in branch wip/instrument-arcbound-ravager.

[FastEddie]

I know already that I will meditate a couple of times over that to fully understand its meanings... for now, thank you so much for your effort and the explanation.

Out of curiosity: is the fact that this bug occurs often but not always related to the fact that the AI goes randomly through the decision tree, i.e. sometimes it takes a path where it stops sacrificing? Or are those unrelated?

[Korath]

I suspect it depends on whether there's a way for an otherwise-sacrificeable artifact to get destroyed (or buried). FWIW, it was always the Ravager re-sacrificing itself in my tests and not the moxes I was giving it to sac; the test deck I use has copies of Lightning Bolt and Ajani Vengeant; and I couldn't get it to crash with an empty AI hand and no other permanents on the bf.

[FastEddie]

This dovetails nicely with my observations. The reason why I was asking is that I usually had crashes when the AI sacced the Arcbound Ravager to itself, but not always (this basically started once I thought I had figured it out). Saccing without crashes clustered for a while which is why I thought about the random number generator.

I also observed that the AI usually (but not always… *sigh*) tries to sac the Arcbound Ravager to itself. My guess is that the modular ability entices it as other sac outlets do not show this behaviour (at least I have not seen it). Never mind that the Ravager is gone afterwards… (I don’t know which AI part is responsible for that action, the combat AI for sure plans only one step ahead and it would make sense that this part of the AI – in case it is not the combat AI – shows the same behaviour).

[drool66]

I added what I think is a sanity check to recopy_card_onto_stack(), to get this (addition is "if(pos > -1)"):

Code: Select all

void recopy_card_onto_stack(int a1)
{
  // 0x436450

  int pos = stack_size - 1;
  if( pos > -1 )
   {
     card_instance_t* stack_inst = get_card_instance(stack_cards[pos].player, stack_cards[pos].card);
     if (stack_inst->internal_card_id == activation_card)
      {
        int pc = stack_inst->parent_card;
        int pp = stack_inst->parent_controller;
        int ts = stack_inst->timestamp;
   
        card_instance_t* parent_inst = get_card_instance(stack_inst->parent_controller, stack_inst->parent_card);
   
        if (parent_inst->state & STATE_DONT_RECOPY_ONTO_STACK)
         parent_inst->state &= ~STATE_DONT_RECOPY_ONTO_STACK;
        else
         {
           memcpy(stack_inst, parent_inst, sizeof(card_instance_t));
   
           stack_inst->internal_card_id = activation_card;
           stack_inst->unknown0x14 = 0;
           stack_inst->kill_code = 0;
           stack_inst->state |= STATE_IN_PLAY;
           stack_inst->parent_controller = pp;
           stack_inst->parent_card = pc;
           stack_inst->timestamp = ts;
   
           if (parent_inst->internal_card_id != -1)
            stack_inst->original_internal_card_id = parent_inst->internal_card_id;
         // Begin additions
           else
            stack_inst->original_internal_card_id = parent_inst->backup_internal_card_id;
         // End additions
   
           if ((int)stack_inst->original_internal_card_id < damage_card
            || (int)stack_inst->original_internal_card_id >= damage_card + 47)
            {
              stack_inst->display_pic_csv_id = cards_data[stack_inst->original_internal_card_id].id;
            // Begin removals
            // stack_inst->display_pic_num = 0
            // End removals
            // Begin additions
              stack_inst->display_pic_num = get_card_image_number(stack_inst->display_pic_csv_id, pp, pc);
            // End additions
            }
         }
   
      // Begin additions
        if (stack_inst->internal_card_id == activation_card)
         stack_inst->token_status &= ~STATUS_DYING;   // so kill_card() will reap it, even if the original card sacrificed itself as an activation card
      // End additions
      }


     if (ai_is_speculating != 1)
      EXE_DWORD_PTR(0x628674)[pos] = a1;

     stack_damage_targets[pos].player = stack_inst->damage_target_player;
     stack_damage_targets[pos].card = stack_inst->damage_target_card;
   }
}

I was able to play a full game after giving the AI three Thran Dynamos in play, and two of each Arcbound Ravager & Cranial Plating in hand. Everything else seemed to work - the AI even ducked a lightning bolt to a Ravager by sacrificing other artifacts; but I will play around with it to see if it breaks anything else.

[FastEddie]

Quick answer: this should be Asserted, otherwise you simply hide an error.

[drool66]

I'm sure you realize the issue is that the AI is speculating onto an empty stack, and querying get_card_instance(stack_cards[-1].player, stack_cards[-1].card)->internal_card_id
and right there is the segfault for these cards. In the proposal above you can replace "if(pos>-1)" with "if( ai_is_speculating != 1)" and also avoid the crash, although the ai seems to make categorically better decisions in the former case. What do you think the best solution is?

[EDIT] I missed Korath's comment above. Looks like there are cracks in allowing responses to triggers, and they are beginning to show. I'm working on this bug, which I find unacceptable. If I can't get that one to work, I suggest we revert and fork response to triggers in general. If I can, I think I am capable of doing what Korath suggests above.

[Aswan jaguar]

I don't understand the programming stuff you are discussing here, so no comment from me on any of that.
My only suggestion is that whenever you have something ready that will stop crashes (not only for this but any bug) even if it is not the best solution (unless it causes more issues than it solves) submit it to go to release and then when somebody is able can fix it in a better way (we don't close the issue and we tag it as waiting for proper fix). Having constantly reports and crashes about Arcbound Ravage and equipment crashing, is waste of everyone's time and getting on your nerves. I usually test new cards or bugs having AI choose random deck as I catch some other bugs this way but when I see AI having an artifact deck I dread with the idea that it will crash again at any moment. And crashes like this happened to me a lot driving me nuts!