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

Fix argument count check for ops with only a variadic argument list. #93

Merged
merged 1 commit into from
May 23, 2024

Conversation

tsymalla
Copy link
Contributor

We don't count variadic arguments in the getNumFullArguments() method since they can be zero. Not properly checking the return value of this function can however lead to a arg_size() < 0 check in the generated verifier method. This happens if we only have a single variadic argument list argument.

This could also be solved with a call to std::max but just let us not emit verifier code instead.

We don't count variadic arguments in the `getNumFullArguments()` method
since they can be zero. Not properly checking the return value of this
function can however lead to a `arg_size() < 0` check in the generated
verifier method. This happens if we only have a single variadic argument
list argument.
@tsymalla-AMD tsymalla-AMD requested a review from nhaehnle May 22, 2024 09:04
@tsymalla-AMD
Copy link
Contributor

Note that the GH Actions job sporadically fails here now as well:
7.982 OSError: [Errno 26] Text file busy: '/vulkandriver/builds/ci-build/compiler/llpc/llvm/tools/llvm_dialects/test/unit/interface/./DialectsADTTests'

I think this is the same error as we experienced previously, I am going to take a look at it.

Copy link
Member

@nhaehnle nhaehnle left a comment

Choose a reason for hiding this comment

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

Change looks good to me.

The sporadic test failure sounds like perhaps there's a missing dependency specification in the CMake.

@tsymalla-AMD tsymalla-AMD merged commit ae1b86b into GPUOpen-Drivers:dev May 23, 2024
8 checks passed
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