Skip to content

Conversation

@ChrisDryden
Copy link
Collaborator

@ChrisDryden ChrisDryden commented Dec 29, 2025

This GNU test is a tough one because it references the source code directly to use GDB to set up a race condition and it is testing a race condition that architecturally the Rust implementation doesn't have.

It appears that the reason this test exists is because the GNU implementation performs the read before setting up the watchers, and the uutils implementation sets up the watchers before performing the read so the race condition does not exist. I tried to come up with the best "Equivelant" for where to point the gdb source to break on and came up with the watcher_rx.watch_with_parent line in watch.rs since its the same equivalent of breaking before setting up the watcher.

Its not to say the test isn't useful for regressions, if we do change the architecture to perform the reads before the watches, this test will make sure that we handle race conditions in that scenario.

@ChrisDryden ChrisDryden marked this pull request as ready for review December 29, 2025 15:23
@oech3
Copy link
Contributor

oech3 commented Dec 29, 2025

Oh, not upstreamable form... I hope that this hack is last 2

@ChrisDryden
Copy link
Collaborator Author

At least there's only three GDB tests in the whole GNU test suite, these two and rm/r-root.sh which is even more problematic because it uses LD_PRELOAD and gdb. I am not sure how you would approach coming up with an upstream test for this one since its testing for an architecture that we don't have.

@ChrisDryden
Copy link
Collaborator Author

I wonder if to think outside the box for these ones, instead of patching, we could come up with a new criteria for these types of tests, instead of SKIP its a "INCOMPATIBLE". or something similar. Something to indicate to the organizations relying on the GNU test compatibility metric to understand that there's a difference between skipping because uutils doesn't support it and skipping because the tests are implementation specific and are not compatible with the uutils implementation.

@oech3
Copy link
Contributor

oech3 commented Dec 29, 2025

I think r-root.sh could be replaced by unshare, but GNU's thought is "widely supported LD_PRELOAD is better".

@oech3
Copy link
Contributor

oech3 commented Dec 29, 2025

I still want to keep leaving not compatible tests to the log. Someone saw them might find actual bugs/security issues from them.

@sylvestre sylvestre merged commit f39081d into uutils:main Jan 20, 2026
129 of 130 checks passed
@ChrisDryden
Copy link
Collaborator Author

Whoops this is working locally but didn't work on the CI, I think there's a gdb dependency missing. Doing some debugging now

@ChrisDryden
Copy link
Collaborator Author

@oech3 I'm realizing the reason this doesn't work is because the debug symbols are stripped from the release-small profile. Is this something that we can enable for the GDB tests?

@oech3
Copy link
Contributor

oech3 commented Jan 20, 2026

Oops... Just back to release profile

@oech3
Copy link
Contributor

oech3 commented Jan 20, 2026

@ChrisDryden
Copy link
Collaborator Author

I think the release profile does not have debug symbols either, I'm running on some branches to see which flag we can just add to enable it

@ChrisDryden
Copy link
Collaborator Author

I tried changing the ENV variables that control this but I think I'm coming across the issue that we cache the build and we do not take the env variables into account when checking the cache

@oech3
Copy link
Contributor

oech3 commented Jan 21, 2026

Yes. debug profile also skips it...

@ChrisDryden
Copy link
Collaborator Author

I think its still beneficial to keep since it appears that its a combination of an issue with timeout, sleep and having a debug binary. I'm hoping that when latest timeout changes for the GNU tests merge they should start working. I'm struggling to debug because I can only replicate it on the CI's and not locally.

mattsu2020 pushed a commit to mattsu2020/coreutils that referenced this pull request Jan 23, 2026
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.

3 participants