Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Perfecting GLFW for Zig, and finding lurking undefined behavior that went unnot

devlog.hexops.com

Post reply on HN
Print view View original

51–60 of 60 posts

Re: Perfecting GLFW for Zig, and finding lurking undefined behavior that went unnot

#51

Post by wffurr »

vlovich123 wrote: post #31
wffurr wrote: post #24

Earlier quoted context omitted.

We run sanitizers nightly, but not as part of presubmit checks. It strikes a good balance between productivity and safety.

I’ve always run sanitized builds as presubmit without any meaningful issue. It’s always nicer to have these issues caught before code gets merged into a shared repo. The long part of things is code review anyway so waiting on a build isn’t the biggest deal in the world.

That's great if it's fast enough. For some builds, like the GP and mine, it's extremely slow.

Re: Perfecting GLFW for Zig, and finding lurking undefined behavior that went unnot

#52

Post by _vvhw »

quag wrote: post #8
abainbridge wrote: post #6

The article discusses some undefined behaviour resulting from shifting an unsigned char left by 24 places. Any idea why the compiler wasn't warning about that? It seems like it would be easy to implement that warning in the compiler. I feel that I'm missing something.

That’s the point made at the end of the article. The compiler check does exist, but because it isn’t defaulted to on, no body in 6 years use it. Zig has the checks on by default, so the first time someone used it, they found and fixed it for everyone. > Anybody using GLFW could have enabled UBSan in their C compiler. Anybody could have run into this same crash and debugged it in the last 6 years. But they didn’t. Onl…

This is what I appreciate about another aspect Zig, checked arithmetic, which is on by default in safe release builds.

For example, if a u64 would overflow through addition, then instead of just allowing the addition to wrap the u64 around to 0, Zig will rather detect this and crash at runtime with an error, unless you explicitly indicate through the "%+" operator that you do intend to allow the addition to wrap.

If this is an unknown unknown for you, you don't need to worry about it because Zig will detect it, simply because the default is correct.

Whereas in other languages that support something like this (and I think even Zig's checked arithmetic is still among the most extensive across all operators), you have to actually opt-in to checked arithmetic, it's not the default in safe builds, which always surprises me.

Re: Perfecting GLFW for Zig, and finding lurking undefined behavior that went unnot

#53

Post by chakkepolja »

user-the-name wrote: post #48
saagarjha wrote: post #46

Earlier quoted context omitted.

I mean, the build instructions could also say "install this package" and then you install it however your distribution does it.

Installing a package to build some code is just fundamentally wrong, though. Why am I making unspecified, permanent changes to my underlying operating system in order to build a random piece of code? That just make no sense , and it is insane that this is the standard way of doing things.

Install in context of library package manager doesn't necessarily mean install to operating system. It can be just caching it somewhere in your home directory.

Re: Perfecting GLFW for Zig, and finding lurking undefined behavior that went unnot

#54

Post by vlovich123 »

wffurr wrote: post #51
vlovich123 wrote: post #31

Earlier quoted context omitted.

I’ve always run sanitized builds as presubmit without any meaningful issue. It’s always nicer to have these issues caught before code gets merged into a shared repo. The long part of things is code review anyway so waiting on a build isn’t the biggest deal in the world.

That's great if it's fast enough. For some builds, like the GP and mine, it's extremely slow.

My point is I've run really big builds (i.e. taking about an hour to build non-sanitized) & we still did sanitized builds pre-submit. How big is the disparity you're seeing between sanitized & non-sanitized builds?

Re: Perfecting GLFW for Zig, and finding lurking undefined behavior that went unnot

#55

Post by phendrenad2 »

saagarjha wrote: post #46
AndyKelley wrote: post #35

Earlier quoted context omitted.

There is a key difference here- if your build instructions include "install this package using apt" then you are limiting your developers to using Ubuntu or Debian, and even then limiting it to a particular version range within those distros. If your build instructions are "zig build" then that works for everyone, on every platform: Windows, macOS, Linux (all distros!), FreeBSD, etc., and it works for all versions of…

I mean, the build instructions could also say "install this package" and then you install it however your distribution does it.

Part of the problem is, packages have different names on different systems, and many are missing altogether (is it libxml-dev or libxml-devel or does your OS simply not have that package, because the package repository owners are in a state of gang warfare with the libxml devs?)

Re: Perfecting GLFW for Zig, and finding lurking undefined behavior that went unnot

#56

Post by wffurr »

vlovich123 wrote: post #54
wffurr wrote: post #51

Earlier quoted context omitted.

That's great if it's fast enough. For some builds, like the GP and mine, it's extremely slow.

My point is I've run really big builds (i.e. taking about an hour to build non-sanitized) & we still did sanitized builds pre-submit. How big is the disparity you're seeing between sanitized & non-sanitized builds?

An hour, ouch. That's pretty long. We try to keep our presubmit under 10 minutes, and that's already frustrating.

I can't figure out how to dig the numbers out of our CI infrastructure for the nightly sanitizer runs.

Re: Perfecting GLFW for Zig, and finding lurking undefined behavior that went unnot

#57

Post by phendrenad2 »

> the left operand, which starts out as unsigned char, will get promoted to int

This is a big flaw in C. Quietly promoting an unsigned type to a signed type, simply because it was added to by a signed number? And that signed number is deterministically known to be > 0? C has major problems like this, you don't even need to look to UB to run into them. I think mixing signed and unsigned values should always give you a stern warning, explaining the pitfalls, and yet (a) the warning you get is some useless Chernobyl error light like "mixing signed and unsigned values" and (b) that warning is probably not enabled easily, without bringing in lots of stupid warnings too.

Re: Perfecting GLFW for Zig, and finding lurking undefined behavior that went unnot

#58

Post by vlovich123 »

wffurr wrote: post #56
vlovich123 wrote: post #54

Earlier quoted context omitted.

My point is I've run really big builds (i.e. taking about an hour to build non-sanitized) & we still did sanitized builds pre-submit. How big is the disparity you're seeing between sanitized & non-sanitized builds?

An hour, ouch. That's pretty long. We try to keep our presubmit under 10 minutes, and that's already frustrating. I can't figure out how to dig the numbers out of our CI infrastructure for the nightly sanitizer runs.

I guess my point was more that it depends on how you structure your workflow. As long as you can do reviews and kick off asynchronous merge requests, the actual time it takes starts to be come largely irrelevant in my experience (aside from the impact it has on local iteration).

These days I'm on a different project and CI ASAN builds are 33 minutes vs normal builds of 21 minutes. I'd say that extra 10 minutes is fine because sanitized builds run in parallel to normal builds & this is clean builds vs incremental.

Re: Perfecting GLFW for Zig, and finding lurking undefined behavior that went unnot

#59

Post by dundarious »

abainbridge wrote: post #44
excizze wrote: post #42

Earlier quoted context omitted.

Probably why it's never been caught or been a problem. Can't think of a reasonable case where a compiler would generate "bad" code because of this.

Agreed. I think it was only a problem because of ubsan. I guess ubsan added checks to the generated code that looked at the value being shifted left 24 and saw that overflow occurred and therefore raised its undefined behaviour signal. The code would never fail on a two's compliment machine. What ubsan is saying is that the rules of C don't guarantee this code to work - it only works because the overflow writes into…

Maybe what I'm suggesting isn't what you had in mind.

The fix for glfw is to manually promote to an appropriate unsigned type, to override the potentially UB-inducing default signed promotion. Since that potentially UB-inducing default can't change now, I was suggesting a warning for it.

It would be an aggressive warning and would probably trigger on a lot of existing code, but honestly, I would enable it for all new code. I really dislike the promotion logic in C.

Re: Perfecting GLFW for Zig, and finding lurking undefined behavior that went unnot

#60

Post by abainbridge »

dundarious wrote: post #59
abainbridge wrote: post #44

Earlier quoted context omitted.

Agreed. I think it was only a problem because of ubsan. I guess ubsan added checks to the generated code that looked at the value being shifted left 24 and saw that overflow occurred and therefore raised its undefined behaviour signal. The code would never fail on a two's compliment machine. What ubsan is saying is that the rules of C don't guarantee this code to work - it only works because the overflow writes into…

Maybe what I'm suggesting isn't what you had in mind. The fix for glfw is to manually promote to an appropriate unsigned type, to override the potentially UB-inducing default signed promotion. Since that potentially UB-inducing default can't change now, I was suggesting a warning for it. It would be an aggressive warning and would probably trigger on a lot of existing code, but honestly, I would enable it for all new…

You're right.

Their expression (simplified) was: *target = pixels[j] I hadn't realized that the pixels[j] was being promoted to an int rather than unsigned int. Eeep.

> I really dislike the promotion logic in C.

Agreed. I'll add that to my list of C dislikes.

I still think that even with manually promoting to the appropriate unsigned type isn't sufficient. Because target has type long*, so coercing the result of (unsigned int)pixels[j] Edit: Damn it, again, that's what the discussion says (https://github.com/glfw/glfw/pull/1986#issuecomment-95800024...).

Post reply on HN