[libcu++] Fix Overflow in Dynamic Shared Memory Validation#9727
[libcu++] Fix Overflow in Dynamic Shared Memory Validation#9727davebayer wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
Walkthrough
ChangesDynamic Shared Memory Overflow Check
Related PRs: None identified. Suggested labels: bug, libcudacxx Suggested reviewers: None identified. Comment |
This comment has been minimized.
This comment has been minimized.
| if (__n * sizeof(value_type) > __max_portable_dyn_smem_size) | ||
| using value_type = typename dynamic_shared_memory_option<_Tp>::value_type; | ||
| const auto [__dyn_smem_size, __overflow] = ::cuda::mul_overflow(__n, sizeof(value_type)); | ||
| if (__dyn_smem_size > __max_portable_dyn_smem_size || __overflow) |
There was a problem hiding this comment.
I would swap these conditions and check __overflow first.
Given the nature of overflow I am afraid that an overzealous compiler could interpret __dyn_smem_size > __max_portable_dyn_smem_size (which would be false on overflow) to mean that __overflow could not possibly be true, and therefore delete the check.
I.e. I am envisioning a scenario similar to deleted nullptr checks:
int *ptr = ...;
int x = *ptr;
if (!ptr) {
// This entire branch is deleted because you deref ptr above
// and because dereferencing a nullptr is UB, the compiler
// is allowed to assume it cannot happen.
}There was a problem hiding this comment.
mul_overflow always has well defined output, so that shouldn't ever happen. But yeah, why not
a6109c1 to
3c29a94
Compare
😬 CI Workflow Results🟥 Finished in 1h 39m: Pass: 97%/120 | Total: 1d 08h | Max: 1h 04m | Hits: 91%/385359See results here. |
No description provided.