enigma-dev / enigma-dev

The Extensible Non-Interpreted Game Maker Augmentation.
http://enigma-dev.org
337 stars 120 forks source link

Uninitialized Variables Broken #1188

Open RemoveRusky opened 6 years ago

RemoveRusky commented 6 years ago

Treat unintialized vars as 0 is generating bad code.

In obj_0 I have the following in step


with (id)
{
    o = 4;
    cons_show_message(string(o));
}

and in obj_1 I have the following in create local int o;

When I run the game I'd expect 4 to printed but it prints 0 instead. Without the with 4 will be printed. Or if I uncheck treat intialized vars as 0 it will also print 4. Here's a diff on the codegen: https://github.com/RemoveRusky/bug/pull/3/files

RemoveRusky commented 6 years ago

@RobertBColton fix this kthx

RobertBColton commented 6 years ago

Did you try opening the gm6 or gm81 in the older LGM and then defragging ids? The problem could be lastinstanceid or lastobjectid, which are also recorded when loading, so the room editor knows where to pick up at when you start creating new instances. Because GMX does not have ids, LGM essentially has to defrag the ids each time you load a GMX, so defragging the ids should have fixed it for you. Also with (id) would basically mean with the instance calling the code, thus the with is redundant.

RemoveRusky commented 6 years ago

links for old lgm are dead on lateralgm site

RobertBColton commented 6 years ago

https://github.com/IsmAvatar/LateralGM/releases

RobertBColton commented 6 years ago

I just loaded the file in GM8 and it also shows 4, so I would say its probably LGM.

RemoveRusky commented 6 years ago

same behavior after resaving from old lgm

RemoveRusky commented 6 years ago

i know with(id) is redundant my code doesnt atually do that. I just simplified it for this bug

RobertBColton commented 6 years ago

What is the id for object0 and object1 according to LateralGM? Also what is the id of the instances in the room? If you deleted and recreated objects about 62 times, LGM will give you an id about that high, since it does not reset, which is how GM used to be. But I still don't understand why GM8 would load it fine. I am pretty sure this bug is LGM side. I can't even get LGM to work over here now, I deleted that old portable and it won't work 64 bit for me.

RemoveRusky commented 6 years ago

the id is 100001 and object_index is 0

RobertBColton commented 6 years ago

Can't be, GM8 and LGM tell me it's 100510:

GM8 instance id

RemoveRusky commented 6 years ago

this is on the defragged version

RobertBColton commented 6 years ago

Hrm, this is really confusing, because the defrag ids option does seem to reset instances and tiles as well: https://github.com/IsmAvatar/LateralGM/blob/f22437fdae90c70c871f23456622ecd0f6938690/org/lateralgm/file/ProjectFile.java#L389

I wonder if it's forgetting to do something else, it goes through all of the lists of resources and resets their ids too... so why would ENIGMA be allocating 62 objects?

RemoveRusky commented 6 years ago

so I created a new diff with defragged ids here https://github.com/RemoveRusky/bug/pull/3/files and arrays correct size. I'm not sure why its writing different code for a gm6 vs a gmx

RemoveRusky commented 6 years ago

const int variant::default_type = ty_undefined; I'm guessing it's that as its ty_real. Do you know what setting toggles that in lgm?

RobertBColton commented 6 years ago

If you look here: https://github.com/enigma-dev/enigma-dev/blob/134aea511a3cf852ef0908c31db92b5290457d07/CompilerSource/compiler/components/write_defragged_events.cpp#L159

It's taking the highest id by looking at the end of list of parsed objects and writing out its id.

RemoveRusky commented 6 years ago

ok its the treat unitialized vars as 0 thats causing this

RobertBColton commented 6 years ago

Try setting the GM compliance setting lower in Enigma settings, I think somebody changed its behavior recently.

RemoveRusky commented 6 years ago

i did. its the treat unitialized vars setting thats causing the issue. looks like its a parser issue.

RemoveRusky commented 6 years ago

@JoshDreamland fix this.

RobertBColton commented 6 years ago

Please take a look at #1082 where I changed the treat uninitialized setting, replacing -1 with ty_undefined enum constant. I believe this may have introduced the regression discussed here.

RemoveRusky commented 6 years ago

testcase.gm6.zip

mikeypro83 commented 6 years ago

Can we remove this bug post? This is not a bug anymore. It works on debian Linux [xubuntu]

RemoveRusky commented 6 years ago

nothing was done to fix this. the codegen is still wrong. so no

JoshDreamland commented 6 years ago

The problem is not in the codegen. The codegen is correct.

This line that changed: dummy_0 = var();

That's where your issue seems to be. Feel free to confirm that manually.

If that line is reached, execution has already gone wrong. For whatever reason, the current instance inside that with statement is not an instance of obj_1... that's your core problem. I don't know why we're conditionally zeroing that variable. Reaching that line is a runtime error.

RobertBColton commented 5 years ago

I just tested this on master after #1659 was merged with the new JDI and it seems this issue has gone away. It could have also been the newly improved var Josh wrote in #1644. When I copy fundies example into the create event of an object and place an instance in the room and run, it does print 4 instead of 0. Fundies GM6 test case also does the same showing me a message box with 4 and not 0 in it. I tested both of these with and without the treat uninitialized variables as 0 setting enabled. So this has been resolved then?

JoshDreamland commented 5 years ago

It's likely that the uninitialized type wasn't being properly copied by the var constructor. Based on my previous comment, there seems to be a bigger issue at play, though. That code should not be accessing the default var; that means that the instance in question doesn't have the variable being requested. But in this case, it should.

JoshDreamland commented 5 years ago

To clarify: if this issue is fixed, then you should be able to replace all lines that say return dummy_0; (or whatever dummy) with a call to abort() and have fundies' example function properly. If that is the case, I likely fixed this when I tore up the compiler a couple months ago. If that's not the case, then we have masked this problem by fixing var::var.

RobertBColton commented 5 years ago

It seems the nice debug error messages I added for variable access no longer work now either. show_message(string(somevar));

Just shows an empty message in run and debug. It seems what happened is we lost the following check for uninitialized variables. This just bit me and Hugar in the ass looking for a bug in his game. https://github.com/enigma-dev/enigma-dev/blob/7c2cabcf7c878c99c4dbcb8bffa38b68ea8c086d/ENIGMAsystem/SHELL/Universal_System/var4.cpp#L31