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

[NFC] Output unused test results to /dev/null #7859

Merged
merged 1 commit into from
Nov 20, 2024

Conversation

fzi-hielscher
Copy link
Contributor

See #7852.

The CI failed on 2d416af :

RUN: at line 1: /__w/circt/circt/build/bin/circt-opt /__w/circt/circt/test/Conversion/HWToBTOR2/errors.mlir --convert-hw-to-btor2 --verify-diagnostics --split-input-file -o /__w/circt/circt/build/tools/circt/test/Conversion/HWToBTOR2/Output/errors.mlir.tmp
+ /__w/circt/circt/build/bin/circt-opt /__w/circt/circt/test/Conversion/HWToBTOR2/errors.mlir --convert-hw-to-btor2 --verify-diagnostics --split-input-file -o /__w/circt/circt/build/tools/circt/test/Conversion/HWToBTOR2/Output/errors.mlir.tmp
circt-opt: /__w/circt/circt/llvm/llvm/lib/Support/raw_ostream.cpp:115: void llvm::raw_ostream::SetBufferAndMode(char *, size_t, BufferKind): Assertion `GetNumBytesInBuffer() == 0 && "Current buffer is non-empty!"' failed.

There seems to be a race condition on the output file buffer. I blame -split-input-file for it (although I've wrongfully blamed it for another error in the past). In upstream, if -split-input-file and -o are used in conjunction, the output file seems to either be /dev/null/ or stdout. So, let's follow that pattern and hope it fixes the problem.

Why hasn't this failed on the hw-enums.mlir test before? ... I do not know. But maybe the timing of the newly added HWToBTOR2 test is just particularly prone to run into this race condition.

Copy link
Member

@seldridge seldridge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I noticed this intermittently on a remote build that I could not reproduce (and was considering holding back the release because of it).

Thanks. While it is unfortunate that the cause isn't sorted out, I see no reason to not output an unused file to /dev/null.

I assume this works in the Windows environment? If not, post-merge will catch it and we can do something else.

@fzi-hielscher
Copy link
Contributor Author

I assume this works in the Windows environment?

Yeah, I've been wondering about that, too. Since it is used in various tests in upstream LLVM, I'd guess it's fine. I was hoping not having to fire up my RGB space heater Windows machine tonight. 😅

@fzi-hielscher fzi-hielscher merged commit bdf1cc9 into llvm:main Nov 20, 2024
4 checks passed
@fzi-hielscher
Copy link
Contributor Author

Dang. ☹️

@seldridge
Copy link
Member

FWIW, I have seen this error now on both Linux builds using clang and on macOS using clang.

@seldridge
Copy link
Member

Thinking about this more, there may not be a good reason to even bother writing these to any file (or device) and instead just let these go to stdout. That said, I have no idea why /dev/null would be problematic here.

@fzi-hielscher
Copy link
Contributor Author

Right now my gut feeling is, that the -o argument isn't actually related this problem. It seems more likely to me that the raw_ostream &os member in HWToBTOR2.cpp is where this crash occurs. But at this point, it is just guesswork. So far no attempt to reproduce it locally has worked for me.

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