Skip to content

[LIBCLC] Set charSize when calling __assertfail #6370

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

Merged
merged 1 commit into from
Jun 29, 2022

Conversation

jchlanda
Copy link
Contributor

The missing parameter was causing the incorrect formatting of the assert messages.

@jchlanda jchlanda requested a review from a team as a code owner June 28, 2022 16:07
@jchlanda jchlanda requested a review from steffenlarsen June 28, 2022 16:07
@jchlanda
Copy link
Contributor Author

/verify with intel/llvm-test-suite#1070

@bader
Copy link
Contributor

bader commented Jun 28, 2022

PR title: Sat -> Set?

@jchlanda
Copy link
Contributor Author

PR title: Sat -> Set?

Yeap, also just noticed conflicts in llvm-test-suite, will fix now.

@jchlanda jchlanda force-pushed the jakub/nvptx_assert branch from 91f772d to 5700e9f Compare June 28, 2022 16:27
@jchlanda jchlanda changed the title [LIBCLC] Sat charSize when calling __assertfail [LIBCLC] Set charSize when calling __assertfail Jun 28, 2022

_CLC_DECL void __assert_fail(const char *expr, const char *file,
unsigned int line, const char *func) {
__assertfail(expr, file, line, func);
__assertfail(expr, file, line, func, 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Very minor and not that I would ever expect them to differ, but should this be

Suggested change
__assertfail(expr, file, line, func, 1);
__assertfail(expr, file, line, func, sizeof(char));

for good measure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @steffenlarsen, sorry was off last couple of days, I would normally agree, but in the PTX interoperability doc, they make an explicit point:

The only supported character size is 1

Copy link
Contributor

@steffenlarsen steffenlarsen left a comment

Choose a reason for hiding this comment

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

LGTM. Comment is not critical and the tests have been merged, so let's get this merged too.

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