Repository navigation
Conversation
SummaryThis PR fixes an undefined behavior issue in ChangesThe fix refactors
This pattern mirrors the existing approach used in the Public API ImpactThe public surface remains unchanged:
All existing test assertions (e.g., Technical DetailsRoot Cause: The previous implementation's multiple inheritance could cause MSVC to generate different type layouts on host vs. device compilation, leading to potential kernel parameter passing issues. Solution: By reducing to single inheritance and using a Related Discussion: References #9238 (comment) where a similar fix was applied to Lines Changed: +3/-4 WalkthroughThis PR removes ChangesOutput Ordering Type Decoupling
Possibly Related PRs
Suggested Reviewers
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a924df08-036b-48c2-936b-df1ac9f8733e
📒 Files selected for processing (1)
libcudacxx/include/cuda/__execution/output_ordering.h
| template <__output_ordering_t _Guarantee> | ||
| struct _CCCL_DECLSPEC_EMPTY_BASES __output_ordering_holder_t | ||
| : __requirement | ||
| , ::cuda::std::integral_constant<__output_ordering_t, _Guarantee> | ||
| struct __output_ordering_holder_t : __requirement | ||
| { | ||
| static constexpr __output_ordering_t value = _Guarantee; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show relevant sections of output_ordering.h
FILE="libcudacxx/include/cuda/__execution/output_ordering.h"
echo "=== $FILE (around 1-160) ==="
sed -n '1,200p' "$FILE" | nl -ba | sed -n '1,200p'
echo "=== $FILE (around 40-120) ==="
sed -n '35,130p' "$FILE" | nl -ba
echo "=== $FILE (around 120-220) ==="
sed -n '120,260p' "$FILE" | nl -ba
# Locate any mention of integral_constant in this header
echo "=== rg integral_constant in output_ordering.h ==="
rg -n "integral_constant|value_type|operator.*(T|int)|type[[:space:]]*=" "$FILE" || true
# Inspect require.h contract
REQ="libcudacxx/include/cuda/__execution/require.h"
echo "=== $REQ ==="
sed -n '1,220p' "$REQ" | nl -ba | sed -n '1,220p'
rg -n "struct __requirement|require\\(" "$REQ" || true
# Inspect determinism.h holder precedent
DET="libcudacxx/include/cuda/__execution/determinism.h"
echo "=== $DET (around relevant parts) ==="
sed -n '1,260p' "$DET" | nl -ba | sed -n '1,260p'
rg -n "__determinism_holder_t|integral_constant|static constexpr __determinism_t|value" "$DET" || true
# Find where sorted_t/stable_sorted_t/unsorted_t are defined/used
echo "=== rg definitions of sorted_t stable_sorted_t unsorted_t ==="
rg -n "sorted_t|stable_sorted_t|unsorted_t" libcudacxx/include/cuda/__execution/output_ordering.h
echo "=== rg usages of sorted_t/stable_sorted_t/unsorted_t ==="
rg -n "sorted_t|stable_sorted_t|unsorted_t" libcudacxx/include/cuda | head -n 50
echo "=== rg for value_type usage with ordering tags ==="
rg -n "value_type" libcudacxx/include/cuda/__execution/output_ordering.h libcudacxx/include/cuda | head -n 80Repository: NVIDIA/cccl
Length of output: 193
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="libcudacxx/include/cuda/__execution/output_ordering.h"
echo "=== $FILE (full excerpt) ==="
nl -ba "$FILE" | sed -n '1,220p'
echo "=== rg integral_constant/value_type in $FILE ==="
rg -n "integral_constant|value_type|operator|type[[:space:]]*=" "$FILE" || true
REQ="libcudacxx/include/cuda/__execution/require.h"
echo "=== $REQ ==="
nl -ba "$REQ" | sed -n '1,260p'
rg -n "struct __requirement|require\\(" "$REQ" || true
DET="libcudacxx/include/cuda/__execution/determinism.h"
echo "=== $DET ==="
nl -ba "$DET" | sed -n '1,320p'
rg -n "__determinism_holder_t|integral_constant|static constexpr.*value" "$DET" || true
echo "=== ordering tags defined in $FILE ==="
rg -n "sorted_t|stable_sorted_t|unsorted_t|__output_ordering_t" "$FILE"
echo "=== usages of ordering tags (limited) ==="
rg -n "sorted_t|stable_sorted_t|unsorted_t" libcudacxx/include/cuda | head -n 80
echo "=== potential use of integral_constant API with ordering tags ==="
rg -n "stable_sorted_t::|sorted_t::|unsorted_t::" libcudacxx/include/cuda | head -n 80
rg -n "stable_sorted_t\\b|sorted_t\\b|unsorted_t\\b" libcudacxx/include/cuda | head -n 80Repository: NVIDIA/cccl
Length of output: 193
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="libcudacxx/include/cuda/__execution/output_ordering.h"
echo "=== $FILE (excerpt) ==="
nl -ba "$FILE" | sed -n '1,260p'
echo "=== rg integral_constant/value_type in $FILE ==="
rg -n "integral_constant|value_type|operator|type[[:space:]]*=" "$FILE" || true
REQ="libcudacxx/include/cuda/__execution/require.h"
echo "=== $REQ ==="
nl -ba "$REQ" | sed -n '1,220p'
rg -n "struct __requirement|require\\(" "$REQ" || true
DET="libcudacxx/include/cuda/__execution/determinism.h"
echo "=== $DET ==="
nl -ba "$DET" | sed -n '1,320p'
rg -n "__determinism_holder_t|integral_constant|static constexpr.*value" "$DET" || true
echo "=== ordering tags defined in $FILE ==="
rg -n "sorted_t|stable_sorted_t|unsorted_t|__output_ordering_t" "$FILE"
echo "=== usages of ordering tags (limited) ==="
rg -n "sorted_t|stable_sorted_t|unsorted_t" libcudacxx/include/cuda | head -n 120
echo "=== use of integral_constant API (value_type/type/operator) with ordering tags ==="
rg -n "(sorted_t|stable_sorted_t|unsorted_t)::(value_type|type)" libcudacxx/include/cuda | head -n 120
rg -n "(sorted_t|stable_sorted_t|unsorted_t)\\b.*(value_type|::type|operator)" libcudacxx/include/cuda | head -n 120Repository: NVIDIA/cccl
Length of output: 188
🏁 Script executed:
# The sandbox will execute the script and return outputRepository: NVIDIA/cccl
Length of output: 37
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="libcudacxx/include/cuda/__execution/output_ordering.h"
REQ="libcudacxx/include/cuda/__execution/require.h"
DET="libcudacxx/include/cuda/__execution/determinism.h"
echo "=== output_ordering.h: first 220 lines ==="
sed -n '1,220p' "$FILE"
echo "=== output_ordering.h: around __output_ordering_holder_t (lines 1-1200 search for it) ==="
rg -n "__output_ordering_holder_t" "$FILE" || true
# Print a window around the match
line=$(rg -n "__output_ordering_holder_t" "$FILE" | head -n1 | cut -d: -f1 || echo 1)
start=$((line-30)); end=$((line+40))
[ "$start" -lt 1 ] && start=1
sed -n "${start},${end}p" "$FILE"
echo "=== output_ordering.h: ordering tag aliases ==="
rg -n "using (sorted_t|stable_sorted_t|unsorted_t)\\b|struct (sorted_t|stable_sorted_t|unsorted_t)\\b|sorted_t\\b" "$FILE" || true
echo "=== output_ordering.h: integral_constant/value_type/type/operator mentions ==="
rg -n "integral_constant|value_type|\\btype\\b|operator[[:space:]]" "$FILE" || true
echo "=== usages across libcudacxx/include/cuda for ordering tags ==="
rg -n "(sorted_t|stable_sorted_t|unsorted_t)::(value_type|type)" libcudacxx/include/cuda || true
rg -n "(sorted_t|stable_sorted_t|unsorted_t)\\b" libcudacxx/include/cuda | head -n 200 || true
echo "=== require.h: __requirement contract ==="
sed -n '1,220p' "$REQ"
rg -n "struct __requirement|require\\(" "$REQ" || true
echo "=== determinism.h: holder precedent ==="
sed -n '1,260p' "$DET"
rg -n "__determinism_holder_t|integral_constant|static constexpr __determinism_t|static constexpr" "$DET" || trueRepository: NVIDIA/cccl
Length of output: 13511
important: In libcudacxx/include/cuda/__execution/output_ordering.h (the __output_ordering_holder_t / sorted_t / stable_sorted_t / unsorted_t aliases), the ordering tags no longer inherit ::cuda::std::integral_constant—they now only expose static constexpr __output_ordering_t value plus query(...). If any callers previously relied on integral_constant members/conversions (e.g., value_type, type, implicit integral conversion), this is a source-compat break; re-expose the needed integral_constant surface on the holder or explicitly confirm/document the intentional break.
There was a problem hiding this comment.
Yeah, I've been thinking about how we'd treat this breakage in libcu++. @miscco, how do we think about this interface change on the query side and how do we trade-off with the potential issue Georgii pointed out here: #9238 (comment)
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Were those already publicly available in a released CCCL version?
If so, then we can just add the respective functionality to the class too and deprecate it for 4.0
| struct _CCCL_DECLSPEC_EMPTY_BASES __output_ordering_holder_t | ||
| : __requirement | ||
| , ::cuda::std::integral_constant<__output_ordering_t, _Guarantee> | ||
| struct __output_ordering_holder_t : __requirement |
There was a problem hiding this comment.
Q: __requirement is never empty?
There was a problem hiding this comment.
__requirement is expectedly empty. Can you elaborate where you're leading to with the question, not sure I follow.
🥳 CI Workflow Results🟩 Finished in 2h 18m: Pass: 100%/118 | Total: 21h 50m | Max: 50m 39s | Hits: 98%/341893See results here. |
| struct _CCCL_DECLSPEC_EMPTY_BASES __output_ordering_holder_t | ||
| : __requirement | ||
| , ::cuda::std::integral_constant<__output_ordering_t, _Guarantee> | ||
| struct __output_ordering_holder_t : __requirement | ||
| { | ||
| static constexpr __output_ordering_t value = _Guarantee; | ||
|
|
||
| [[nodiscard]] _CCCL_NODEBUG_API constexpr auto query(const __get_output_ordering_t&) const noexcept | ||
| -> __output_ordering_holder_t<_Guarantee> | ||
| { | ||
| return *this; | ||
| } | ||
| }; | ||
|
|
There was a problem hiding this comment.
I believe we should be safe due to _CCCL_DECLSPEC_EMPTY_BASES .
Given that this is a fully empty class with only 3 specializations I believe we should just add the three static_asserts after the class that it is indeed empty to be super safe
This branch has not been deployed
3 participants
cuda::execution::output_ordering::__output_ordering_holder_tinherits from two empty base classes, which can give it a different layout/size between host and device on MSVC and is therefore UB to pass into a kernel. This mirrors the fix applied totie_breakto follow the__determinism_holder_tpatter. We now keep a single empty base and expose the preference via a static constexpr value member instead (see #9238 (comment)).