Friday Facts #296 - All kinds of bugs

[Klonan]

https://factorio.com/blog/post/fff-296

[Koub]

These remind me so much about when I was in school, learning to code, and trying to debug stuff XD

[Optera]

That logic is "correct" in that it does what it was supposed to do: open any gates that the train could drive over. What it wasn't accounting for was a rail system where everything looped back onto itself 5-10 times per junction.The time complexity for the algorithm it was using was O(N^2). That's "fine" when N is small. However, in this save file, with this rail network, and with these modded trains (with 2,500% speed bonus from modded fuel no less) it meant N ended up somewhere around 75,865. That - as it turns out - was slow.

Finally a reason beside the terrible look to ban roundabout junctions from servers.

[Philip017]

Im sure glad you like to fix these bugs, because we sure dont like it when they break our game we are tring to enjoy

thank you for all your hard work, know it is appreciated!

[Blacky007]

the same junction with working all parallel drive through



and yes finding those bugs is hard.
I have an open bug I'am unable to find it.
viewtopic.php?f=29&t=62155

Keep up the good work @DEVs

[UPS_Eater]

As a budding programmer, this has been very interesting to read and I will keep this page as it has very good advise. Thank you for posting these!

[svalorzen]

A thing I tend to do for these cases, when the code is supposed to be very performant, is just add a ton of asserts. Then the game crashes the same, but when you debug the problem you can get much closer to the source by starting to look from the first failed assert rather than the last thing which explodes due to the chain of impossibilities.

[Shingen]

blah blah missing signal in red circle blah blah OCD blah blah blue-circled signals do nothing blah blah

factorio_missing_signal.png (912.22 KiB) Viewed 14780 times

A thing I tend to do for these cases, when the code is supposed to be very performant, is just add a ton of asserts. Then the game crashes the same, but when you debug the problem you can get much closer to the source by starting to look from the first failed assert rather than the last thing which explodes due to the chain of impossibilities.

when code is supposed to be very performant i assume i've written it properly and don't add asserts which slow it down constantly, but instead when something does break, i just work my way backwards from the code that breaks. if a bug is reproducible it's really not a problem.
actually, i do that also when it's not supposed to be very performant.
because most likely the bug is gonna happen in such a way that they're not gonna help you anyway :v

[Dave64738]

Klonan: If you're interested in sorting out overflow errors maybe you could take a look at the Production Scrap bug where creating a boiler results in attempting to create 4.3G stone furnaces? If you try to cancel it this results in the map and your inventory being spammed with tons of stone. The author has "fixed" it by disallowing intermediate products, which suggests it could be problem in the base game rather than his mod - like this one I think something small is going negative and being converted to unsigned, and also because stone -> (high chance of boiler, small chance of just getting stone back). But it doesn't occur when you just try to create a stone furnace; it has to be an intermediate furnace.

I did have a savegame where this was 100% reproducible but I think I deleted it. I can have a go at recreating it if you like. The problem is reproducible in very early game: just get some stone and iron and try to create a boiler.

[programaths]

The best thing to do when you encounter the NPE is work out why and if you should or not correct in place.

If not, then you should still catch and throw a new exception and set the original exception as a cause. This permits to add extra information that can help the calling code to discriminate this exception against a general NPE one.

Also, you can hint the developer by explaining what went wrong in the message and in the documentation of the exception.

Lastly, in Java (for java fellow), where you put a comment to document an assumption, remove it to add an explicit "assert" statement. These are a noop on normal circumstances, but can be enabled while testing. Also, they are more visible than comments AND clearly state what they are: an assertion.

In C or any languages with preprocessing, macros can be used.

[Bilka]

If you're interested in sorting out overflow errors maybe you could take a look at the Production Scrap bug where creating a boiler results in attempting to create 4.3G stone furnaces?

That was fixed a while ago: 68836

[eradicator]

The best thing to do when you encounter the NPE is work out why and if you should or not correct in place.

Error: Acronym overflow has occured near "NPE".

[bNarFProfCrazy]

I really like these in depth details.

[xng]

You're in good company with the signed integer cast problem, that is the reason Gandhi in Civilization is so aggressive (his aggressiveness was -1). The bug was introduced in the first game and because it was an interesting bug to have Gandhi as a nuke maniac he still is the same in every new iteration of the series.

[ledow]

NULL checks are critical and should be absolutely everywhere. And result in instant failure of the application, with full debugging trace.

The first line of any new C function that I write is literally just a list of any pointer parameters with:

assert(parameter != NULL);
assert(parameter2 != NULL);

That stops it "breaking" the program, but it doesn't stop it having NULL sent to it - which is what I think you're getting at. Catching the invalid values is the safety net to stop an unintended crash and instead crash *knowingly*. Actually stopping the crash happening is another matter entirely.

And, (I program in C99 so this is different and much easier in C++) any access to any other structures should be managed by a getter/setter function. Don't just blindly assume that if you're passed a large structure, you can just iterate through it, plug in values inside it and anything else you find in there (like other pointers to other structures), etc. Create it as a type, use getter/setter functions for that type, and deny access to the pointer to the underlying data as much as possible.

Then the getters/setters can validate and test functionality as they go (not to mention centralise and *log* accesses so debugging is easier), functions used come with a set of constraints/invariants/assumptions/whatever you want to call them regarding the validity of the data they are acting on using the official functions, and accidents like this result in asserts all over the shop (at least in any debug build) with trace logs from the very first instance.

Also, you need to *extra carefully* validate any data that comes from outside... this includes mods, saves, translation strings, image files read from disc, everything. You can't just act on the mod's code as if -1 was a valid value, from the very beginning.

It's hard, but after a while, you start to program like that as a force of habit. And if you use the right data-types, you aid yourself a lot. (Again, I'm sure there's a fancy C++ way to do this, but I tend to program with typedef's types for "unsafe_file" and "parsed_file" or similar, and there's literally only one function that takes or can act on an "unsafe_file" and that's the parser function that returns a "parsed_file" type).

If those functions mentioned had had asserts on negative numbers, if they'd had asserts on the parsing routines for negative numbers, if they'd had no way to turn a file with -1 into a valid "parsed" file, e.g. no dumb-casting of values into an unsigned value so it can only hold 0 or above and doesn't see -1 as 4 billion, and if all your access to such values - in-game and by mods - was by a single set of getters-setters that are constrained by these asserts, then you could have had a fighting chance at discovering it before someone was able to send code to a game on my computer that could overflow the memory and potentially access things it shouldn't be able to.

It's all about funnelling down *all* possible access - internal, debugging, modding, scripting, "cheating" (e.g. AI giving itself a thousand free fuel or whatever) to one getter/setter which then has one set of all possible checks on the validity of its values, and nobody else has any access to it. Every shortcut through that path might be "performance enhancing" but it's all "security bypassing".

Then when you disable asserts/debugging, the code optimises away to nothing (almost all modern compilers will inline short validation snippets, remove asserts in production, or even completely remove checks that test only assumptions that can never be true etc.), when you refactor code you have one place to change, and when you have a crash a simple debug run goes BANG, BANG, BANG, assert, error, debug, "here's a problem"...

I know I'm preaching to the converted, but so many times such simple things as "we just assume mods know what they are doing", "we special-case mods so they can do what they like", or even "we don't bother to validate our image textures because they came from us so they must be fine" (where a simple substitution of a texture with a malicious one then gives someone else user privilege next time the game is loaded, etc.) result in major problems that are only patched up after the event, where they should be in there *by design* of the structure of the code.

[Rakshasa]

NULL checks are critical and should be absolutely everywhere. And result in instant failure of the application, with full debugging trace.

The first line of any new C function that I write is literally just a list of any pointer parameters with:

assert(parameter != NULL);
assert(parameter2 != NULL);

The point of 'assert' is that you can compile it with NDEBUG defined to disable it when making a release build, thus allowing you to have as many asserts as you want while working on the code without affecting production code.

[dasiro]

everyone loves a good bug-hunt (once it's solved)

[Omnifarious]

when code is supposed to be very performant i assume i've written it properly and don't add asserts which slow it down constantly, but instead when something does break, i just work my way backwards from the code that breaks. if a bug is reproducible it's really not a problem.
actually, i do that also when it's not supposed to be very performant.
because most likely the bug is gonna happen in such a way that they're not gonna help you anyway :v

I rarely trust myself to generate test cases that truly exercise all the code paths or combinations thereof, and so I write a test that generates significant swaths of the possible input space and tests all of them. Even then I've sometimes missed things.

[Dev-iL]

...why would this code ever say to make a negative amount of smoke?" So I kept going ... why?

• ... Because the logic to make those signed integers was "(cycle progress ...) - (last cycle progress ...)" (cycle was < last cycle ... that should never be possible)
• Because the furnace burner had made a negative amount of progress in burning the fuel (negative progress should not be possible)
• Because the "remaining amount of fuel from this item to burn" was negative (negative fuel values are invalid and the game won't even reach the main menu if some mod tries to set one)
• Because the mod API didn't prevent mods from doing: entity.burner.remaining_burning_fuel = -1 AND the game didn't properly clear "remaining amount of fuel from this item to burn" when the item being burnt was removed due to mod migration/removal.

Good job with the mental stack-trace

[dog80]

that fking smart part of the brain