ast_canopy: skip Itanium mangling for dependent-signature functions#324
ast_canopy: skip Itanium mangling for dependent-signature functions#324isVoid wants to merge 2 commits intoNVIDIA:mainfrom
Conversation
clang::ItaniumMangleContext::mangleName segfaults when handed a FunctionDecl whose parameter or return types are still template-dependent. Because the Function constructor unconditionally mangled every FunctionDecl it visited, any header containing a CRTP base class, a function template with a dependent return type, or Eigen-style expression templates would segfault the whole Python process. Introduce has_dependent_signature(FD), which returns true when: - the return type is dependent, or - any parameter type is dependent, or - the parent (for methods) is a dependent context. When it returns true, skip mangling and fall back to the qualified name. Otherwise proceed as before. Also wrap the ItaniumMangleContext handle in std::unique_ptr to plug a leak -- ItaniumMangleContext::create allocates with raw new but there was no delete. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdded a helper to detect template-dependent function signatures and updated Function construction to avoid Itanium mangling for dependent signatures (falling back to the qualified name). Switched the Itanium mangle context to a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ast_canopy/cpp/src/function.cpp`:
- Around line 79-80: The current fallback sets mangled_name = qual_name when
has_dependent_signature(FD) is true, which loses parameter/qualifier info and
conflates overloads; instead, construct an overload-stable fallback by appending
a printed signature (parameter type list and any cv/ref qualifiers) to qual_name
or by tagging the name as non-unique (e.g., qual_name + "<dependent-signature:
...>") so callers won't treat it as a unique mangled symbol. Locate the branch
where has_dependent_signature(FD) is checked and build the signature from FD’s
parameter types and function qualifiers (use the FunctionDecl/FunctionProtoType
accessors to print each parameter type and cv/ref qualifiers), then set
mangled_name to qual_name + separator + that printed signature (or qual_name +
explicit dependent tag) so overloads remain distinguishable.
In `@ast_canopy/tests/test_crtp_dependent_mangle.py`:
- Around line 49-54: The test currently only checks that "extract_value"
appears; add an assertion that verifies the dependent function template's
mangled name uses the fallback instead of an empty/Itanium mangled form: after
obtaining decls.function_templates and selecting the template whose
ft.function.name == "extract_value", assert that ft.function.mangled_name is
non-empty and not starting with "_Z" (or assert equality to the qualified-name
fallback via ft.function.qualified_name) to ensure Function::Function assigned
the dependent-signature fallback.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 556a84e8-ef3e-4292-8e04-5bb82e3f7fd4
📒 Files selected for processing (3)
ast_canopy/cpp/src/function.cppast_canopy/tests/data/sample_crtp_dependent.cuast_canopy/tests/test_crtp_dependent_mangle.py
| if (has_dependent_signature(FD)) { | ||
| mangled_name = qual_name; // best-effort fallback |
There was a problem hiding this comment.
Use an overload-stable fallback instead of qual_name alone.
qual_name drops the parameter list, so dependent overloads in the same scope will now get the same mangled_name. That makes overloads indistinguishable downstream even though the crash is avoided. Please build the fallback from a printed signature (qual_name + parameter types + cv/ref qualifiers) or mark it separately so callers do not treat it as a unique mangled symbol.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ast_canopy/cpp/src/function.cpp` around lines 79 - 80, The current fallback
sets mangled_name = qual_name when has_dependent_signature(FD) is true, which
loses parameter/qualifier info and conflates overloads; instead, construct an
overload-stable fallback by appending a printed signature (parameter type list
and any cv/ref qualifiers) to qual_name or by tagging the name as non-unique
(e.g., qual_name + "<dependent-signature: ...>") so callers won't treat it as a
unique mangled symbol. Locate the branch where has_dependent_signature(FD) is
checked and build the signature from FD’s parameter types and function
qualifiers (use the FunctionDecl/FunctionProtoType accessors to print each
parameter type and cv/ref qualifiers), then set mangled_name to qual_name +
separator + that printed signature (or qual_name + explicit dependent tag) so
overloads remain distinguishable.
| def test_dependent_function_template_captured(source_path): | ||
| """extract_value has a dependent return type. It should appear as a | ||
| function template without having segfaulted the mangler.""" | ||
| decls = parse_declarations_from_source(source_path, [source_path], "sm_80") | ||
| ft_names = [ft.function.name for ft in decls.function_templates] | ||
| assert "extract_value" in ft_names, ft_names |
There was a problem hiding this comment.
Assert the dependent-signature fallback explicitly.
This only proves that extract_value was collected. It does not verify the new behavior at Function::Function, so a regression in the fallback assignment could still pass. Please also assert that the dependent function template's mangled_name is the qualified-name fallback (or at least non-empty and not _Z-prefixed).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ast_canopy/tests/test_crtp_dependent_mangle.py` around lines 49 - 54, The
test currently only checks that "extract_value" appears; add an assertion that
verifies the dependent function template's mangled name uses the fallback
instead of an empty/Itanium mangled form: after obtaining
decls.function_templates and selecting the template whose ft.function.name ==
"extract_value", assert that ft.function.mangled_name is non-empty and not
starting with "_Z" (or assert equality to the qualified-name fallback via
ft.function.qualified_name) to ensure Function::Function assigned the
dependent-signature fallback.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ast_canopy/tests/data/sample_crtp_dependent.cu`:
- Around line 23-24: CRTPBase's const member add() calls the non-const helper
derived(), breaking const-correctness; add a const overload of derived() (i.e.,
a __device__ const Derived& derived() const that static_casts *this to const
Derived&) so const member functions can call it without casting, leaving the
existing non-const __device__ Derived& derived() for mutable use.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d9985115-e372-4178-a329-3c18baba4df1
📒 Files selected for processing (1)
ast_canopy/tests/data/sample_crtp_dependent.cu
| __device__ Derived &derived() { return static_cast<Derived &>(*this); } | ||
| __device__ Derived add(const Derived &other) const { return derived(); } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In C++, can a const member function call a non-const member function on the same object? Please cite the const-correctness rule and a minimal example.
💡 Result:
No, a const member function cannot call a non-const member function on the same object. The const-correctness rule is specified in the C++ standard via [class.mfct.non-static]: In the body of a const member function, the implicit object parameter (this) has type cv-qualified (const-qualified), so only other const member functions may be called normally on *this. A non-const member function requires a non-const this pointer, causing a compilation error. Minimal example: struct Example { void nonConst {} // non-const member function void constFunc const { // const member function nonConst; // error: passing 'const Example' as 'this' argument discards qualifiers } }; This fails to compile, as confirmed across sources including cppreference.com and Stack Overflow discussions.
Citations:
- 1: https://cppreference.dev/w/cpp/language/member_functions
- 2: https://en.cppreference.com/w/cpp/language/member_functions.html
- 3: https://isocpp.org/wiki/faq/const-correctness
- 4: https://stackoverflow.com/questions/5008541/how-to-call-a-non-const-function-within-a-const-function-c
- 5: https://stackoverflow.com/questions/35342001/can-i-call-a-non-const-member-function-of-a-const-object-in-c
🏁 Script executed:
# Check if the file exists and read the relevant lines
if [ -f "ast_canopy/tests/data/sample_crtp_dependent.cu" ]; then
echo "=== File exists, showing lines 23-24 ==="
sed -n '20,28p' ast_canopy/tests/data/sample_crtp_dependent.cu
else
echo "File not found at expected path"
fiRepository: NVIDIA/numbast
Length of output: 529
🏁 Script executed:
# Check if the Python test file exists and references this data file
if [ -f "ast_canopy/tests/test_crtp_dependent_mangle.py" ]; then
echo "=== Python test file found, searching for reference to the data file ==="
grep -n "sample_crtp_dependent" ast_canopy/tests/test_crtp_dependent_mangle.py
else
echo "Python test file not found"
fiRepository: NVIDIA/numbast
Length of output: 199
Add const overload for derived() to fix const-correctness violation
CRTPBase<Derived>::add is const (line 24) but calls non-const derived() (line 23). Per C++ const-correctness rules, a const member function cannot call a non-const member function on *this without explicit casting. This code cannot compile as written and breaks the test fixture.
Suggested fix
template <typename Derived> struct CRTPBase {
__device__ Derived &derived() { return static_cast<Derived &>(*this); }
+ __device__ const Derived &derived() const {
+ return static_cast<const Derived &>(*this);
+ }
__device__ Derived add(const Derived &other) const { return derived(); }
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| __device__ Derived &derived() { return static_cast<Derived &>(*this); } | |
| __device__ Derived add(const Derived &other) const { return derived(); } | |
| template <typename Derived> struct CRTPBase { | |
| __device__ Derived &derived() { return static_cast<Derived &>(*this); } | |
| __device__ const Derived &derived() const { | |
| return static_cast<const Derived &>(*this); | |
| } | |
| __device__ Derived add(const Derived &other) const { return derived(); } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ast_canopy/tests/data/sample_crtp_dependent.cu` around lines 23 - 24,
CRTPBase's const member add() calls the non-const helper derived(), breaking
const-correctness; add a const overload of derived() (i.e., a __device__ const
Derived& derived() const that static_casts *this to const Derived&) so const
member functions can call it without casting, leaving the existing non-const
__device__ Derived& derived() for mutable use.
1 participant
Summary
clang::ItaniumMangleContext::mangleNamesegfaults when handed aFunctionDeclwhose parameter or return types are still template-dependent. Because theFunctionconstructor unconditionally mangled everyFunctionDeclit visited, any header containing a CRTP base class, a function template with a dependent return type, or Eigen-style expression templates would segfault the whole Python process.has_dependent_signature(FD), which returns true when the return type is dependent, any parameter type is dependent, or the parent (for methods) is a dependent context. When it returns true, mangling is skipped and the qualified name is used as a best-effort fallback.ItaniumMangleContexthandle instd::unique_ptr(create()allocates with rawnewbut there was no matchingdelete).Test plan
ast_canopy/tests/data/sample_crtp_dependent.cu:T::value_type.vec3_dot) that must still be mangled normally.ast_canopy/tests/test_crtp_dependent_mangle.py:CRTPBaseandVec3class templates are captured.extract_valueappears as a function template (dependent-signature path works).vec3_dotstill has a valid_Z-prefixed Itanium mangled name (regression guard).libastcanopy.sowith only this branch applied and ran the test to confirm the fix is sufficient in isolation.Summary by CodeRabbit
Bug Fixes
Tests