[DeviceSanitizer] Support multiple error reports (-fsanitize-recover=address) #13948
Conversation
| LocalArgsInfo *LocalArgs = nullptr; // ordered by ArgIndex | ||
| LocalArgsInfo *LocalArgs = nullptr; // Ordered by ArgIndex | ||
|
|
||
| DeviceSanitizerReport SanitizerReport[ASAN_MAX_NUM_REPORTS]; |
There was a problem hiding this comment.
Are there any concerns about register-pressure related to this? Would it make sense to somehow let users use a more light-weight version with fewer reports?
There was a problem hiding this comment.
LaunchInfo is allocated in shared USM, so I think it won't cause register-pressure here.
AllanZyne
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
| __spirv_BuiltInWorkgroupId.z; | ||
|
|
||
| auto &SanitizerReport = ((__SYCL_GLOBAL__ LaunchInfo *)__AsanLaunchInfo) | ||
| ->SanitizerReport[WG_LID % ASAN_MAX_NUM_REPORTS]; |
There was a problem hiding this comment.
Given an error in a workgroup and another error in an another workgroup, these two errors may be saved to the same index and either of them has a chance to be kept.
Program behavior is probably more predicable if we use an atomically increasing index instead.
There was a problem hiding this comment.
Program behavior is probably more predicable if we use an atomically increasing index instead.
What does predicable mean here?
Even using atomic counter, we can't guarantee that we can save all error reports in your case, and their order are also undetermined.
I didn't try to save as most as possible error reports because I think it's not necessary (as most of them are likely pointing to the same error location).
Maybe it would be better to save more different types of error, but it's too complicated to implement now.
There was a problem hiding this comment.
Even using atomic counter, we can't guarantee that we can save all error reports in your case, and their order are also undetermined.
Right, order is indeterministic. Could you please elaborate why we can't guarantee that all of the two errors are not reported in my case?
There was a problem hiding this comment.
Given an error in a workgroup and another error in an another workgroup, these two errors may be saved to the same index and either of them has a chance to be kept.
I don't understand, why does either of them has a chance to be kept?
They can save into specific index of reports unless that index is already used.
There was a problem hiding this comment.
Because the order of workgroups execution is indeterministic.
When there is clash in indexing, we can't say which one is saved since the second error won't be save after the first one is saved.
There was a problem hiding this comment.
Because the order of workgroups execution is indeterministic.
When there is clash in indexing, we can't say which one is saved since the second error won't be save after the first one is saved.
Yep. I think it's okay to save either one.
AllanZyne
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
AllanZyne
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
|
@intel/unified-runtime-reviewers @intel/dpcpp-sanitizers-review Please review. |
|
@AllanZyne oneapi-src/unified-runtime#1677 has been merged, please merge the latest sycl branch in and update the UR tag as suggested |
Co-authored-by: Callum Fare <[email protected]>
Done |
|
@intel/llvm-gatekeepers Please merge |
UR: oneapi-src/unified-runtime#1677
In kernel, we save at most
ASAN_MAX_NUM_REPORTS(default: 10) number of SanitizerReport.We select the index of SanitizerReport by
WG_LINEAR_ID % ASAN_MAX_NUM_REPORTS.When
-fsanitize-recover=addressis passed in compiler flag,asan_loadX_noabort/asan_storeX_noabortis used, we useis_recover = trueflag to distinguish this case.If
is_recoveris true, the UR will print out all the error reports and continue (use at your own risk).If
is_recoveris false (default case), the UR will print out only one error report and exit.