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

[WIP][SYCL][CUDA] Lit exceptions #1304

Closed

Conversation

bjoernknafla
Copy link
Contributor

Fix LIT tests that are not compiled with assertions enabled but call
assert.

Only done for LIT tests that require fixes during work on CUDA.

Signed-off-by: Bjoern Knafla bjoern@codeplay.com

Do not catch unexpected exceptions to return an error code but allow
the exception to terminate the tested program. This will result in
useful exception-related error output that is otherwise lost.

Do not catch and then ignore an unexpected exception but re-throw it.

Explicitly returning error codes.

Abort when catching asynchronous exceptions.

Signed-off-by: Bjoern Knafla <bjoern@codeplay.com>
Allow unexpected exceptions to terminate the LIT test as this outputs
extra information.

Signed-off-by: Bjoern Knafla <bjoern@codeplay.com>
@bjoernknafla
Copy link
Contributor Author

bjoernknafla commented Mar 12, 2020

WIP as it requires: #1302

@bader bader added the cuda CUDA back-end label Mar 12, 2020
fwyzard added a commit to fwyzard/llvm that referenced this pull request Mar 14, 2020
@bader
Copy link
Contributor

bader commented Mar 16, 2020

Fix LIT tests that are not compiled with assertions enabled but call
assert.

Aren't assertions enabled by default and should be explicitly disabled with NDEBUG?
https://en.cppreference.com/w/cpp/error/assert

@bjoernknafla
Copy link
Contributor Author

@bader You are right and my PRs to replace assert with a different macro are not required. Great catch and removes unnecessary code changes/PRs.

I seemingly got confused due to working for too long with CMake and explicit CMAKE_BUILD_TYPE settings which for release builds set NDEBUG.

fwyzard added a commit to cms-patatrack/llvm that referenced this pull request Mar 21, 2020
# Conflicts:
#	sycl/test/basic_tests/image_api.cpp
@bader
Copy link
Contributor

bader commented Apr 10, 2020

@bjoernknafla, do you want to proceed with these changes?

@bjoernknafla
Copy link
Contributor Author

I need/want to rework this a bit - should I close and reopen it once ready or keep it open and marked as WIP?

@bader
Copy link
Contributor

bader commented Apr 10, 2020

I'm fine to keep it open.

I was confused by your previous comment:

WIP as it requires: #1302

#1302 is closed, so it was not clear to me whether you decided to abandon this PR or continue with this approach.

@bjoernknafla
Copy link
Contributor Author

As I learned from you, assert won’t be compiled out without explicitly setting NDEBUG, so I closed the MR that introduced a CHECK macro to replace assert. This PR needs to be adapted accordingly.

I striked that old comment through now to prevent future confusion.

@bjoernknafla
Copy link
Contributor Author

This PR is old and outdated and I won't be able to get back to it for a while. While the LIT tests could need another revisit to ensure that exceptions are not ignored I suggest to close this PR.

@bader
Copy link
Contributor

bader commented Aug 4, 2020

Thanks for the update.
Closing...

@bader bader closed this Aug 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cuda CUDA back-end
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants