Skip to content

[DevSan][Refactor] Make Options an unified class shared by all sanitizers #17157

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Feb 28, 2025
Merged

[DevSan][Refactor] Make Options an unified class shared by all sanitizers #17157

merged 7 commits into from
Feb 28, 2025

Conversation

yingcong-wu
Copy link
Contributor

@yingcong-wu yingcong-wu commented Feb 25, 2025 •
edited
Loading

  • Make Options shared by different sanitizers.
  • Also, remove the max_redzone option as we don't think it serves much purpose.
  • Move the testing in SYCL e2e test options-invalid-values.cpp to UR unit test.

Reason:

  1. reduce duplicated codes
  2. if there are any options with same name in different sanitizers, then they should have the same type and same meaning, which is more friendly to users. So it should be okay to have one unified options class.

@yingcong-wu yingcong-wu requested review from a team and bader as code owners February 25, 2025 10:57
@bader bader removed their request for review February 25, 2025 16:42
@yingcong-wu yingcong-wu changed the title [WIP][RFC] [DevSan][Refactor] Make Options an unified class shared by all sanitizers [DevSan][Refactor] Make Options an unified class shared by all sanitizers Feb 26, 2025
zhaomaosu
zhaomaosu reviewed Feb 26, 2025
View reviewed changes
unified-runtime/source/loader/layers/sanitizer/sanitizer_common/sanitizer_options.cpp Outdated

Parser.ParseUint64("quarantine_size_mb", MaxQuarantineSizeMB, 0, UINT32_MAX);
Parser.ParseUint64("redzone", MinRZSize, 16, 2048);
Parser.ParseUint64("max_redzone", MaxRZSize, 16, 2048);
Copy link
Contributor

Choose a reason for hiding this comment

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

As we discussed before, 'max_redzone' is probably not useful to the user, maybe we can remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I will remove it.

AllanZyne
AllanZyne reviewed Feb 26, 2025
View reviewed changes
sycl/test-e2e/AddressSanitizer/common/options-invalid-values.cpp Outdated
@@ -13,14 +13,14 @@

// Invalid quarantine_size_mb
// RUN: env UR_LAYER_ASAN_OPTIONS=quarantine_size_mb:-1 %{run} not --crash %t 2>&1 | FileCheck %s --check-prefixes INVALID-QUARANTINE
// RUN: env UR_LAYER_ASAN_OPTIONS=quarantine_size_mb:4294967296 %{run} not --crash %t 2>&1 | FileCheck %s --check-prefixes INVALID-QUARANTINE
// INVALID-QUARANTINE: <SANITIZER>[ERROR]: "quarantine_size_mb" should be an integer in range[0, 4294967295].
// RUN: env UR_LAYER_ASAN_OPTIONS=quarantine_size_mb:4294967296 %{run} %t 2>&1 | FileCheck %s --check-prefixes INVALID-QUARANTINE
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you remove "not" here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Becase the out-of-range problem is now no longer an error. we will limit the value to a valid range(set to minimum or maximum value) and report a warning now, so we don't need a not here anymore.

zhaomaosu
zhaomaosu approved these changes Feb 26, 2025
View reviewed changes
Copy link
Contributor

@zhaomaosu zhaomaosu left a comment

Choose a reason for hiding this comment

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

lgtm

@yingcong-wu
Copy link
Contributor Author

To address the comment #15293 (comment), I will remove the sycl e2e test sycl/test-e2e/AddressSanitizer/common/options-invalid-values.cpp and move those testing to the UR unit test.

Tagging @AlexeySachkov .

@yingcong-wu
Copy link
Contributor Author

yingcong-wu commented Feb 27, 2025 •
edited
Loading

Kindly ping @intel/unified-runtime-reviewers @pbalcer @AlexeySachkov for review.

pbalcer
pbalcer approved these changes Feb 27, 2025
View reviewed changes
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.

This is much nicer than how these options were parsed before. Thanks for doing the refactor!
UR lgtm.

@AlexeySachkov
Copy link
Contributor

Kindly ping @intel/unified-runtime-reviewers @pbalcer @AlexeySachkov for review.

Thanks for moving the test! I will leave the actual review to code owners

@yingcong-wu
Copy link
Contributor Author

Hi @intel/llvm-gatekeepers , the CUDA failure is unrelated and I have opened #17231 to track it. Could you please help merge this PR? Thanks.

@uditagarwal97 uditagarwal97 merged commit e2ab2b9 into intel:sycl Feb 28, 2025
28 of 29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AllanZyne AllanZyne AllanZyne approved these changes

@pbalcer pbalcer pbalcer approved these changes

@zhaomaosu zhaomaosu zhaomaosu approved these changes

@AlexeySachkov AlexeySachkov Awaiting requested review from AlexeySachkov

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@yingcong-wu @AlexeySachkov @AllanZyne @pbalcer @zhaomaosu @uditagarwal97