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

[UR][layer] Add exception sanitizer layer #2216

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

hdelan
Copy link
Contributor

@hdelan hdelan commented Oct 17, 2024

Add basic exception sanitizer layer that will call std::abort if an exception is thrown. This layer can be used to make sure an adapter isn't throwing, which is prohibited for a C interface. The layer can be enabled by building with the option -DUR_ENABLE_EXCEPTION_SANITIZER=ON

@hdelan hdelan changed the title Add exception sanitizer layer [UR][layer] Add exception sanitizer layer Oct 17, 2024
@github-actions github-actions bot added the loader Loader related feature/bug label Oct 17, 2024
source/loader/ur_lib.hpp Show resolved Hide resolved
Copy link
Contributor

@pbalcer pbalcer left a comment

Choose a reason for hiding this comment

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

I suspect this was never tested :D So might be good to enable this in CI for CTS.

@github-actions github-actions bot added the ci/cd Continuous integration/devliery label Oct 17, 2024
@hdelan hdelan force-pushed the except-san-layer branch 2 times, most recently from f26923e to 375ab46 Compare October 21, 2024 15:27
@hdelan
Copy link
Contributor Author

hdelan commented Oct 22, 2024

I suspect this was never tested :D So might be good to enable this in CI for CTS.

Thanks have done so. I'm not sure what the failures are related to.

@github-actions github-actions bot added cuda CUDA adapter specific issues hip HIP adapter specific issues command-buffer Command Buffer feature addition/changes/specification labels Oct 22, 2024
@hdelan hdelan marked this pull request as ready for review October 22, 2024 09:18
@hdelan hdelan requested review from a team as code owners October 22, 2024 09:18
Add basic exception sanitizer layer that will call std::abort if an
exception is thrown.
And initialize UR_ENABLE_EXCEPTION_SANITIZER to OFF.
Add compile time macro and also fix typos in ur_lib.hpp.
Don't allow UR_CHECK_ERROR to be called outside of a try block.
Copy link
Contributor

@EwanC EwanC left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for improving our code quality

Copy link
Contributor

@kbenzie kbenzie left a comment

Choose a reason for hiding this comment

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

So the missing part here is actually enabling the layer in the CTS environment setup, something along the lines of this:

diff --git a/test/conformance/source/environment.cpp b/test/conformance/source/environment.cpp
index 006ea09b..737d26d2 100644
--- a/test/conformance/source/environment.cpp
+++ b/test/conformance/source/environment.cpp
@@ -90,6 +90,14 @@ uur::PlatformEnvironment::PlatformEnvironment(int argc, char **argv)
             error = "Failed to enable validation layer";
             return;
         }
+#ifdef UR_ENABLE_EXCEPTION_SANITIZER
+        if (urLoaderConfigEnableLayer(config, "UR_LAYER_EXCEPTION_SANITIZER") !=
+            UR_RESULT_SUCCESS) {
+            urLoaderConfigRelease(config);
+            error = "Failed to enable exception sanitizer layer";
+            return;
+        }
+#endif
     } else {
         error = "Failed to create loader config handle";
         return;

CMakeLists.txt Outdated
@@ -43,6 +43,7 @@ option(UR_USE_TSAN "enable ThreadSanitizer" OFF)
option(UR_ENABLE_TRACING "enable api tracing through xpti" OFF)
option(UR_ENABLE_SANITIZER "enable device sanitizer" ON)
option(UR_ENABLE_SYMBOLIZER "enable symoblizer for sanitizer" OFF)
option(UR_ENABLE_EXCEPTION_SANITIZER "enable exception sanitizer" OFF)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure we need a separate option, we could always enable this as part of UR_DEVELOPER_MODE since that's enabled in CI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, have removed.

CMakeLists.txt Outdated Show resolved Hide resolved
.github/workflows/build-hw-reusable.yml Outdated Show resolved Hide resolved
Co-authored-by: Kenneth Benzie (Benie) <k.benzie83@gmail.com>
hdelan and others added 2 commits October 24, 2024 10:04
Co-authored-by: Kenneth Benzie (Benie) <k.benzie83@gmail.com>
@hdelan
Copy link
Contributor Author

hdelan commented Oct 24, 2024

So the missing part here is actually enabling the layer in the CTS environment setup, something along the lines of this:

diff --git a/test/conformance/source/environment.cpp b/test/conformance/source/environment.cpp
index 006ea09b..737d26d2 100644
--- a/test/conformance/source/environment.cpp
+++ b/test/conformance/source/environment.cpp
@@ -90,6 +90,14 @@ uur::PlatformEnvironment::PlatformEnvironment(int argc, char **argv)
             error = "Failed to enable validation layer";
             return;
         }
+#ifdef UR_ENABLE_EXCEPTION_SANITIZER
+        if (urLoaderConfigEnableLayer(config, "UR_LAYER_EXCEPTION_SANITIZER") !=
+            UR_RESULT_SUCCESS) {
+            urLoaderConfigRelease(config);
+            error = "Failed to enable exception sanitizer layer";
+            return;
+        }
+#endif
     } else {
         error = "Failed to create loader config handle";
         return;

Interesting, how was the layer working here without this call?

@pbalcer
Copy link
Contributor

pbalcer commented Oct 24, 2024

Interesting, how was the layer working here without this call?

The init function of all layers gets called unconditionally, and the layer themselves need to check whether they are on the enabled list.
This is so that a layer can force-enable itself if it's needed as a dependency of something else (e.g., validation layers).

@hdelan
Copy link
Contributor Author

hdelan commented Oct 24, 2024

The init function of all layers gets called unconditionally, and the layer themselves need to check whether they are on the enabled list. This is so that a layer can force-enable itself if it's needed as a dependency of something else (e.g., validation layers).

Aha thanks @pbalcer that explains it

@hdelan
Copy link
Contributor Author

hdelan commented Oct 25, 2024

Have updated. Should be good to go.

@github-actions github-actions bot added the conformance Conformance test suite issues. label Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/cd Continuous integration/devliery command-buffer Command Buffer feature addition/changes/specification conformance Conformance test suite issues. cuda CUDA adapter specific issues hip HIP adapter specific issues loader Loader related feature/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants