[1.1.42] Lamp color is wrong when the sum of red+green circuit wires overflows

Post your bugs and problems so we can fix them.
Post Reply
quale
Inserter
Inserter
Posts: 34
Joined: Thu Aug 15, 2019 4:16 pm
Contact:

[1.1.42] Lamp color is wrong when the sum of red+green circuit wires overflows

Post by quale »

Lamp 2-wire overflow.png
Lamp 2-wire overflow.png (661.51 KiB) Viewed 487 times


One constant combinator supplies 2 billion red. The other supplies 2 billion green and negative 2 billion blue. When passed through both red and green wires, the sum of those wires overflows at the input. This isn’t normally a concern, as demonstrated by letting an arithmetic combinator do the summing. However, the lamp itself behaves erratically when the overflow occurs at its input. How does overflowed/now-negative red become yellow? How does a lamp radiate black?

I do like the black lamp. It would be nice if you could set that color by normal means, and as the off state for improved contrast on displays that don’t need a combinator per pixel.

Edit again: confirmed in 1.1.42.
Attachments
factorio-current.log
(5.59 KiB) Downloaded 41 times
Last edited by quale on Wed Oct 13, 2021 7:18 pm, edited 2 times in total.

SoShootMe
Fast Inserter
Fast Inserter
Posts: 212
Joined: Mon Aug 03, 2020 4:16 pm
Contact:

Re: [1.1.39] Lamp color is wrong when the sum of red+green circuit wires overflows

Post by SoShootMe »

Hmm... when I import and place your blueprint, I get two white lamps (left) and two blue lamps (right), which seems to be the expected result: the highest priority of the colours that are positive, after summing the wires, determines the colour.

The only obvious difference is Factorio 1.1.39 (build 58937, mac, steam) (you) vs Factorio 1.1.39 (build 58937, win64, steam) (me)...

quale
Inserter
Inserter
Posts: 34
Joined: Thu Aug 15, 2019 4:16 pm
Contact:

Re: [1.1.39] Lamp color is wrong when the sum of red+green circuit wires overflows

Post by quale »

Then I smell undefined behavior. People forget that C and C++ don’t allow signed overflow among other things. It will often behave like the underlying hardware, until it comes to bite you. Maybe the Mac compiler took advantage in this case. A compiler flag like -fwrapv should stop that, if not a source code change to make it overflow in a defined way.

SoShootMe
Fast Inserter
Fast Inserter
Posts: 212
Joined: Mon Aug 03, 2020 4:16 pm
Contact:

Re: [1.1.39] Lamp color is wrong when the sum of red+green circuit wires overflows

Post by SoShootMe »

quale wrote:
Tue Sep 14, 2021 1:01 pm
Then I smell undefined behavior.
Agreed, but I'd be surprised if it's the undefined behaviour (in the C or C++ standard sense) of signed integer overflow as that is invariably defined for a given CPU architecture and virtually universal anyway.

quale
Inserter
Inserter
Posts: 34
Joined: Thu Aug 15, 2019 4:16 pm
Contact:

Re: [1.1.39] Lamp color is wrong when the sum of red+green circuit wires overflows

Post by quale »

That doesn’t matter. A compiler can do what it wants, and can have legitimate reasons to optimize. Examples include using wider types to store values, and eliminating conditions that are redundant under these assumptions. I can observe the latter on x86-64 in practice. It very much resembles this bug.

Code: Select all

bool redundant(int a, int b)
{
    return a > 0 && b > 0 && a + b > 0;
}

redundant(int, int):                         # @redundant(int, int)
        test    edi, edi
        setg    cl
        test    esi, esi
        setg    al
        and     al, cl
        ret

SoShootMe
Fast Inserter
Fast Inserter
Posts: 212
Joined: Mon Aug 03, 2020 4:16 pm
Contact:

Re: [1.1.39] Lamp color is wrong when the sum of red+green circuit wires overflows

Post by SoShootMe »

quale wrote:
Tue Sep 14, 2021 4:13 pm
That doesn’t matter. A compiler can do what it wants, and can have legitimate reasons to optimize. Examples include using wider types to store values, and eliminating conditions that are redundant under these assumptions. I can observe the latter on x86-64 in practice. It very much resembles this bug.
Sorry, I misread your previous post and missed the point. But AIUI, the optimisation in your example is valid because, given a > 0 and b > 0, "a + b > 0" is true and therefore can be optimised away in all cases where the behaviour is defined, ie where a + b does not overflow. "The usual arithmetic conversions" prohibit "using wider types to store values" (assuming you mean eg the result of a + b in your example being wider than int) but that is part of what makes the optimisation valid - it's important to specify when undefined behaviour occurs :).

quale
Inserter
Inserter
Posts: 34
Joined: Thu Aug 15, 2019 4:16 pm
Contact:

Re: [1.1.39] Lamp color is wrong when the sum of red+green circuit wires overflows

Post by quale »

I dug into the assembly because this was killing me. It was indeed as I suspected, in addition to being an unspeakable mess I’ll spare you. Distilled down to the bare minimum that should help you understand the bugged coloring:

Code: Select all

color_channels = {
	{"red", {1, 0, 0, 1}},
	{"green", {0, 1, 0, 1}},
	{"blue", {0, 0, 1, 1}},
	{"yellow", {1, 1, 0, 1}},
	{"pink", {1, 0, 1, 1}},
	{"cyan", {0, 1, 1, 1}},
	{"white", {1, 1, 1, 1}}
};

result = {1, 1, 1, -1};
for (channel : color_channels) {
	signal = red_network[channel.id] + green_network[channel.id];
	
	// Using the flags of the add instruction, as if we have 33 result bits.
	// Undefined behavior allows this to happen.
	if (signal > 0) {
		result.b = channel.color.b;
		result.a = channel.color.a;
	}
	
	// Using the flags of a subsequent test instruction,
	// based on the wrapped 32-bit result.
	if (wrap(signal) > 0) {
		result.r = channel.color.r;
		result.g = channel.color.g;
		break;
	}
}
return result;
An alpha value of -1 means the color is ignored, making it white.

SoShootMe
Fast Inserter
Fast Inserter
Posts: 212
Joined: Mon Aug 03, 2020 4:16 pm
Contact:

Re: [1.1.39] Lamp color is wrong when the sum of red+green circuit wires overflows

Post by SoShootMe »

quale wrote:
Wed Sep 15, 2021 7:20 am

Code: Select all

...
	signal = red_network[channel.id] + green_network[channel.id];
	
	// Using the flags of the add instruction, as if we have 33 result bits.
	// Undefined behavior allows this to happen.
	if (signal > 0) {
...
Given your "as if we have 33 result bits" comment, the flags state tested to effectively evaluate "signal > 0" is not obvious to me (at least). Care to elaborate, to satisfy my curiousity?

Not that I see reason to doubt there is a bug due to reliance on a particular outcome of undefined behaviour, as you suggested.

quale
Inserter
Inserter
Posts: 34
Joined: Thu Aug 15, 2019 4:16 pm
Contact:

Re: [1.1.39] Lamp color is wrong when the sum of red+green circuit wires overflows

Post by quale »

It uses a JLE instruction, which jumps when ZF=1 or SF≠OF. Like all instructions named after inequalities, it uses a flag based on the carry bit. That’s very useful after a comparison (which is just a subtraction that doesn’t store its result). The optimization opportunity exists in the limited set of conditional instructions: there is no single instruction for > or ≤ that doesn’t use the carry bit. If you were subtracting, you could swap the operands and use SF for < or ≥ instead. But after an addition, all you can do is test the 32-bit result with an extra instruction, or check ZF and SF separately in two conditional instructions.

This is where the compiler decides it’s not having any of it, and invokes undefined behavior to save a whole instruction. Not that it helps when it goes split-brain on the two halves of the result, with the overhead of both. There’s more weird stuff accounting for way more than one instruction, while other sequences are conspicuously optimal. It must be hard to identify which optimization to apply at which time, and when to end. It’s honestly amazing that optimizers do a pretty good job in a reasonable amount of time.

Post Reply

Return to “Bug Reports”