Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
laytan opened this issue Apr 24, 2024 · 3 comments
Closed

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

laytan opened this issue Apr 24, 2024 · 3 comments

Comments

@laytan
Copy link
Sponsor Collaborator

laytan commented Apr 24, 2024

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

// This struct compare fails at -opt:2 if PNG_Dims is not #packed.

Steps to Reproduce

odin run tests/core/image -o:speed

@laytan
Copy link
Sponsor Collaborator Author

laytan commented Jun 8, 2024

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
Copy link
Member

Kelimion commented Jun 9, 2024

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
Copy link
Member

Kelimion commented Jun 9, 2024

Fixed via 8fcfd8c

@Kelimion Kelimion closed this as completed Jun 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants