Port bugfix for incorrect heap deallocation on conditional operator

[WPMGPRoSToTeMa]

Fixes #621.

There are another changes in hier13, but I've ported only fix.
I'm a bit worry that this fix doesn't save ALT register, maybe it is guaranteed that ALT isn't used across condition and branch body?

@rtxa test it please.

Related to #623.

[Arkshine]

I tried a little.

It works well for general use like:

server_print("1. %s", test1 ? fmt("Something") : ""); // Nothing
server_print("2. %s", test2 ? fmt("Something") : ""); // "Something"
server_print("3. %s", test1 ? "" : fmt("Something")); // "Something"
server_print("4. %s", test2 ? "" : fmt("Something")); // Nothing
server_print("5. %s", test1 ? "Hello" : "World");     // Hello
server_print("6. %s", test2 ? "Hello" : "World");     // World
server_print("7. %s", test2 ? (test2 ? "Egg" : "Something") : "Something else"); // Egg
server_print("8. %s", test2 ? (test1 ? "Egg" : "Something") : "Something else"); // Something
server_print("9. %s", test1 ? (test2 ? "Egg" : "Something") : "Something else"); // Something else

However, it asserts (line1085, assert(sc_status!=statWRITE || heap1==heap2);) with the following:

server_print("11. %s", test2 ? (test2 ? fmt("Egg") : "Something") : "Something else"); // Egg
server_print("12. %s", test2 ? (test1 ? fmt("Egg") : "Something") : "Something else"); // Something

Note that without the assert, the first works fine, but the second throwns a runtime error.
If fmt is used everywhere, it works though:

server_print("10. %s", test2 ? (test2 ? fmt("Egg") : fmt("Something")) : fmt("Something else")); // Egg

[WPMGPRoSToTeMa]

@Arkshine can you test your code with #623?

[Arkshine]

Well, no assert and no runtime error. Looks like your patch works better. Maybe this PR is not complete.
Though no idea if that's something you should do. I mean comparing to upstream, it's quite different.

[WPMGPRoSToTeMa]

@Arkshine can you try to test it on SP compiler and somehow on upstream compiler?

I think only binaries will be enough, I can analyze them in Disassembler.

[WPMGPRoSToTeMa]

Tested it on the upstream compiler and it is also bugged: http://tpcg.io/7OMWqr.

UPD: Fixed.

[Arkshine]

I tried it, but I see no difference. Still throw an assert, and even without, still see runtime error.

[WPMGPRoSToTeMa]

@Arkshine did you compile it yourself? Or downloaded from the first post?

[Arkshine]

Pulled from your branch, checking the new changes to make sure, then compiling myself yes.

I tried two times.

[WPMGPRoSToTeMa]

@Arkshine sorry, I tested it only on 1.8.1. Now it should work.

[Arkshine]

Well, still the same. I have even tried to copy-paste manually hier13() from the PR.
It doesn't seem like you changed something before the rebase.

[Arkshine]

I see, actually it works, but not with a function retourning an array.

server_print("11. %s", test2 ? (test2 ? fmt2("Egg") : "Something") : "Something else"); // Egg

fmt2()
{
  new array[256] =  "hey";
  return array;
}

[WPMGPRoSToTeMa]

@Arkshine that is because this function is located under it call. On the first pass it doesn't know what this function actually returns. We need an additional pass for this case or we can use #623 or we need some lazy evaluation for heaplist.

[Arkshine]

Without assertion, it seems to work fine.
It's working under 1.8.2, It's stills a regression and something should be done.
Maybe the assertion could be commented?

[WPMGPRoSToTeMa]

@Arkshine unfortunately it generates bad assembly, try:

server_print("42. %s", true ? "Something" : fmt2());

[WPMGPRoSToTeMa]

It is bugged in SourcePawn too:

#include <sourcemod>

#pragma semicolon 1
//#pragma newdecls required

public Plugin myinfo = {
	name        = "",
	author      = "",
	description = "",
	version     = "0.0.0",
	url         = ""
};

public void OnPluginStart()
{
	PrintToServer(true ? "Hello, World!" : fmt2());
}

fmt2() {
	new arr[256];
	return arr;
}

There is no heap "free":

 0xB68       PROC                        ; OnPluginStart
 0xB6C       BREAK     
 0xB70       BREAK     
 0xB74       CONST.pri               0x1
 0xB7C       JZER              jump_0000
 0xB84       CONST.pri             0x8DC
 0xB8C       JUMP              jump_0001
 0xB94       HEAP                  0x400 ; target:jump_0000
 0xB9C       PUSH.alt  
 0xBA0       PUSH.C                  0x0
 0xBA8       CALL             .3028.fmt2
 0xBB0       POP.pri   
 0xBB4       SM_TRACKER.PUSH.C            0x400
 0xBBC       PUSH.pri   ; target:jump_0001
 0xBC0       SYSREQ.N      PrintToServer (1 args)
 0xBCC       ZERO.pri  
 0xBD0       RETN      

[WPMGPRoSToTeMa]

@Arkshine try it. I can add more conditions for the reparsing if necessary. For example we can add a new usage flag uPRECALCHEAP and check for it.

I think we can first merge this into the upstream (for review from the compuphase) and then merge it into amxmodx.

[Arkshine]

Looks like it doesn't throw an assert anymore. At runtime, it looks fine as well.

[Arkshine]

@WPMGPRoSToTeMa Do you think it's needed to add more conditions for the reparsing?

[WPMGPRoSToTeMa]

@Arkshine I think it isn't worth to add them.

[Arkshine]

Alright, let's merge in master first and see if people have issues. We are not going to have an answer any time soon from Compuphase.