search

odin-lang / Odin

Odin Programming Language
https://odin-lang.org
BSD 3-Clause "New" or "Revised" License
6.38k stars 561 forks source link

Image test failure on linux/darwin amd64 with -o:speed on LLVM 17 & 18 #3480

Closed laytan closed 2 months ago

laytan commented 3 months ago

Context

While trying to verify #3308 by running all tests with optimisations turned on, I found this test failure that has been there for a while already. I couldn't figure it out in reasonable time there, so I am making this issue.

Reproduced on Linux and Darwin amd64, with -o:speed on LLVM 17 and 18.

There is even a comment by @Kelimion in the test file mentioning failure https://github.com/odin-lang/Odin/blob/c72a269b7cfd5f955254f9c3ad8e5e63bac7aa2c/tests/core/image/test_core_image.odin#L1494

Steps to Reproduce

odin run tests/core/image -o:speed

laytan commented 2 months ago

Found the issue, bottom line is we hit undefined behaviour.

During the blend of PNG images we have this part of the code: c.r = u8((1.0 - alpha) * bg[0] + f32(c.r) * alpha) ultimately casting a f32 to u8, in the case here this resolves to: c.r = u8((1.0 - 0) * 65535 + f32(255) * 0) c.r = u8(65535)

which overflows, ok, that can still be as designed, Odin should not have undefined behaviour so should be fine.

But then:

package main

import "core:fmt"

main :: proc() {
    oh := f32(65535)
    no := u8(oh)
    fmt.println(no) // 255 on -o:none, 0 on -o:speed
}

This all took me way way too long to get to the bottom of for it to be so simple

Kelimion commented 2 months ago

I've also replicated it on Windows and Linux on other optimization levels:

W:\Odin\bug>odin run .
48 [255, 255, 0, 255, 255, 0, 255, 255, 0, 255, 255, 0, 255, 255, 0, 255, 255, 0, 255, 255, 0, 255, 255, 0, 255, 255, 0, 255, 255, 0, 255, 255, 0, 255, 255, 0, 255, 255, 0, 255, 255, 0, 255, 255, 0, 255, 255, 0]
W:\Odin\bug>odin run . -o:size
48 [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]
W:\Odin\bug>odin run . -o:speed
48 [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]
W:\Odin\bug>odin run . -o:aggressive
48 [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]

/mnt/w/Odin/bug $ odin run .
48 [255, 255, 0, 255, 255, 0, 255, 255, 0, 255, 255, 0, 255, 255, 0, 255, 255, 0, 255, 255, 0, 255, 255, 0, 255, 255, 0, 255, 255, 0, 255, 255, 0, 255, 255, 0, 255, 255, 0, 255, 255, 0, 255, 255, 0, 255, 255, 0]
/mnt/w/Odin/bug $ odin run . -o:size
48 [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]
/mnt/w/Odin/bug $ odin run . -o:speed
48 [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]
/mnt/w/Odin/bug $ odin run . -o:aggressive
48 [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]
Kelimion commented 2 months ago

Fixed via 8fcfd8c506752d3e0d65e4d9ef7856486a65ca5f