Skip to content

ci: Use ASan for unit tests#98

Merged
AndyFilter merged 3 commits into
AndyFilter:masterfrom
delet-this:tests_sanitizers
Apr 22, 2026
Merged

ci: Use ASan for unit tests#98
AndyFilter merged 3 commits into
AndyFilter:masterfrom
delet-this:tests_sanitizers

Conversation

@delet-this

@delet-this delet-this commented Apr 19, 2026

Copy link
Copy Markdown

Use Address Sanitizer for unit tests in the CI to help catch memory related mistakes. Undefined Behavior Sanitizer (UBSan) should also be enabled eventually, however the unit tests currently have some ub related to left shfiting negative integers and signed integer overflow in the fixed point arithmetic code, which should be looked at first.

@AndyFilter

Copy link
Copy Markdown
Owner

Is there a Duolingo course for GitHub, where you have to open a PR everyday, so that the green bird doesn't torture your family?
Just kidding, obviously. But you've got a nice streak going, haha. By all means - keep it up, I'm really thankful for your contributions. The quality of code in this repo shot through the roof in the last couple of weeks.


Now, with that detour out of the way.

I feel like the fixed-point code is doing much more then the UBSan can comprehend with the left shifts.
Some of these warnings could be fixed with type-casting, but I don't think it's necessary. No one ever complained that their system is crashing while using this driver, so I'd say it's good enough as is.
But nonetheless, I'll look into this.

Looks good to me, like you said - the UBSan can be added later.

@delet-this

Copy link
Copy Markdown
Author

Is there a Duolingo course for GitHub, where you have to open a PR everyday, so that the green bird doesn't torture your family?

Just doing anything at all to procrastinate writing my thesis I guess

I feel like the fixed-point code is doing much more then the UBSan can comprehend with the left shifts.

Apart from bugs, afaik UBSan generally doesn't really have false positives so I'd still be suspicious

No one ever complained that their system is crashing while using this driver

Not sure about the behavior of these specific errors, but generally with ub it's possible that even if it's working fine today in some specific configuration, the results could change in the future due to optimization level, different compiler, compiler update, different architecture or some seemingly innocent, unrelated code change. That can make bugs hard to track down, so it would be nice to eventually have no ubsan errors, or be really sure these ones are fine.

It might not be that bad though. I noticed that relaxing the integer overflow rules with -fno-strict-overflow makes all the errors disappear. The kernel itself uses this flag by default (maybe the gui and tests should too?). However I'm not quite if this flag really allows left shifting negative integers, couldn't find something like that in the docs and clanker said no.

@AndyFilter

Copy link
Copy Markdown
Owner

Just doing anything at all to procrastinate writing my thesis I guess

haha, I feel you!

Apart from bugs, afaik UBSan generally doesn't really have false positives so I'd still be suspicious

Yea, that's fair. I'll look into the exact input values that cause these errors.

Not sure about the behavior of these specific errors (...)

Yea, I get it. What I'm most curious about is how this:

uint64_t remainder;
__asm__ (
//"cqto\n\t"
"idivq %[v]" : "=a"(result), "=d"(remainder) : [v] "r"(v), "a"(u0), "d"(u1)
);

does not cause any ub / errors. It works for negative and positive numbers, up to $2^{32}-1$ for the dividend. Without the cqto! Which is crazy to me, but... if it works, I don't touch it, haha.

The current UB errors are mostly caused by the trigonometric functions (and their subcalls). I'll look into that later, but after a quick glance these errors are mostly explained by the comments in the code.

I noticed that relaxing the integer overflow rules with -fno-strict-overflow makes all the errors disappear.

Yea, that's interesting. Kind of makes sense, as all these UBs are just shifting the number out of bounds to get the fractional part or something like that. Like I said before, I think most (if not all) of these could be fixed by casting the number to the unsigned-version of its type.

@AndyFilter

Copy link
Copy Markdown
Owner

I actually prompted a clanker from github to fix these issues, and it gave up after like 2 minutes of adding (FP_ULONG) everywhere. Which is a very reasonable response, I don't blame it.

(I reached the token limit with that one prompt, even though I thought I have github Pro...)

@AndyFilter AndyFilter merged commit 63f1334 into AndyFilter:master Apr 22, 2026
1 check passed
@delet-this delet-this deleted the tests_sanitizers branch April 22, 2026 13:09
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

Successfully merging this pull request may close these issues.

2 participants