feat(logging): add SpanGuard and SPAN_GUARD macro for IR source location in errors#496
feat(logging): add SpanGuard and SPAN_GUARD macro for IR source location in errors#496luohuan19 wants to merge 1 commit intohw-native-sys:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces a thread-local span context and RAII SpanGuard to attach IR source locations to InternalError messages. Adds a span-aware internal_check_with_span binding and inserts SPAN_GUARD calls across codegen and IR transform visitors to populate the context during traversal and code generation. Tests added for end-to-end behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant IR as IR Node Visitor
participant SpanGuard as SpanGuard (thread-local)
participant Code as Codegen / Transform
participant Logger as FatalLogger / InternalError
participant Python as Python Binding
IR->>SpanGuard: SPAN_GUARD(op->span_) (set g_current_span_str)
IR->>Code: perform visitor / emit work
Code->>Logger: INTERNAL_CHECK fails
Logger->>SpanGuard: read g_current_span_str
Logger->>Python: raise InternalError with "(at file:line:col)" appended
SpanGuard-->>IR: destructor restores previous span (scope exit)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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. 📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances debugging capabilities by integrating IR node source location information directly into internal error messages. By introducing a thread-local span context managed by an RAII guard, developers can now pinpoint the exact user code location that led to an internal check failure, drastically improving the clarity and actionability of error reports. This change provides a more robust and user-friendly error reporting mechanism without altering the core logic of the system. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
luohuan19
force-pushed
the
fix-issue-477
branch
from
8ee7708 to
813973d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
tests/ut/core/test_logging.py (1)
624-653: Test name slightly misrepresents the behavior being tested.The test
test_nested_span_guard_restores_outer_spantests sequentialinternal_check_with_spancalls rather than truly nested RAII guards. Each call creates and immediately destroys its ownSpanGuard. A true nested test would require C++ code where oneSPAN_GUARDis active while another is created inside it.The test is still valuable for verifying that sequential calls don't leak span context, which is the actual behavior being validated. Consider renaming to
test_sequential_span_checks_isolate_contextor updating the docstring to clarify the sequential nature.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/ut/core/test_logging.py` around lines 624 - 653, Rename the test function test_nested_span_guard_restores_outer_span to test_sequential_span_checks_isolate_context (or update its docstring) to accurately reflect that it exercises sequential internal_check_with_span calls that set and clear SpanGuard context rather than true nested RAII guards; update the function name and/or the triple-quoted docstring at the top of the test to mention that it verifies sequential creation/destruction of SpanGuard via pypto.internal_check_with_span and that pypto.internal_check has no span afterwards, referencing symbols internal_check_with_span, internal_check, and SpanGuard in the docstring to make intent explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/codegen/orchestration/orchestration_codegen.cpp`:
- Around line 547-548: The IfStmt visitor is missing a span scope so errors
during condition generation or traversing a top-level if lack source location;
add a SPAN_GUARD(if_stmt->span_) at the start of VisitStmt_(const IfStmtPtr&
if_stmt) to mirror other statement visitors, and likewise add SPAN_GUARD(...) to
the other unguarded statement visitors referenced (the neighboring visitors
around the same block) to ensure all statement paths are wrapped with the span
guard.
In `@src/codegen/pto/pto_codegen.cpp`:
- Around line 1054-1055: The AssignStmt visitor lacks span guarding so errors
during lowering lose user locations; add SPAN_GUARD(op->span_) as the first
statement in VisitStmt_(const AssignStmtPtr& op) to ensure any
CHECK/INTERNAL_CHECK reports the assignment span, and likewise add the same
SPAN_GUARD(op->span_) to the other unguarded VisitStmt_ overloads referenced
(the other VisitStmt_(...) functions missing guards) so all hot-path statement
lowering uses the span guard.
In `@src/ir/transforms/mutator.cpp`:
- Around line 49-50: The mutator currently only installs SPAN_GUARD in a few
overloads (e.g., IterArg, Call, ForStmt), leaving many INTERNAL_CHECK failures
outside the correct span; update the mutator to install the span guard at every
visitor entry so failures report the node's span: either add
SPAN_GUARD(op->span_) to the remaining VisitExpr_/VisitStmt_ overloads (e.g.,
AssignStmt, IfStmt, MakeTuple, TupleGetItemExpr and the binary/unary operator
visitors) or centralize the guard by adding it to the common dispatch entry used
by ExprFunctor/StmtFunctor before delegating to specific VisitExpr_/VisitStmt_
methods (ensure VisitExpr_/VisitStmt_ signatures such as
IRMutator::VisitExpr_(const XPtr& op) and IRMutator::VisitStmt_(const YPtr& op)
all run SPAN_GUARD with op->span_).
In `@src/ir/transforms/visitor.cpp`:
- Around line 41-42: The visitor only guards a few overloads (e.g.,
VisitExpr_(IterArgPtr)) which leaves many other nodes calling INTERNAL_CHECK
without a current span; instead, add span propagation at the central recursion
entry points by placing the SPAN_GUARD (or equivalent span-setting logic) at
IRVisitor::VisitExpr and IRVisitor::VisitStmt so every recursive dispatch sets
the current span before calling any VisitExpr_/VisitStmt_ overloads (this will
cover MakeTuple, TupleGetItemExpr, unary/binary visitors, AssignStmt, IfStmt,
YieldStmt, ReturnStmt, WhileStmt and container statements); update or remove
redundant per-overload SPAN_GUARDs (e.g., the ones at the three existing
overloads and lines mentioned) to avoid duplication and ensure INTERNAL_CHECK
always sees a valid span.
In `@tests/ut/core/test_logging.py`:
- Around line 619-622: The f-string assertion in the test (assert start_line <=
for_stmt.span.begin_line <= func_end_line) contains an en-dash character in the
message f"...(lines {start_line}–{func_end_line})"; replace that en-dash with a
standard hyphen-minus so the message becomes f"...(lines
{start_line}-{func_end_line})" to eliminate the ambiguous character while
keeping the same formatting and variables (start_line, for_stmt.span.begin_line,
func_end_line).
---
Nitpick comments:
In `@tests/ut/core/test_logging.py`:
- Around line 624-653: Rename the test function
test_nested_span_guard_restores_outer_span to
test_sequential_span_checks_isolate_context (or update its docstring) to
accurately reflect that it exercises sequential internal_check_with_span calls
that set and clear SpanGuard context rather than true nested RAII guards; update
the function name and/or the triple-quoted docstring at the top of the test to
mention that it verifies sequential creation/destruction of SpanGuard via
pypto.internal_check_with_span and that pypto.internal_check has no span
afterwards, referencing symbols internal_check_with_span, internal_check, and
SpanGuard in the docstring to make intent explicit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 457e3c16-3089-4dc2-af79-6f25788b19e8
📒 Files selected for processing (13)
include/pypto/core/logging.hpython/bindings/modules/logging.cpppython/pypto/__init__.pypython/pypto/pypto_core/__init__.pyipython/pypto/pypto_core/logging.pyisrc/codegen/cce/cce_codegen.cppsrc/codegen/orchestration/orchestration_codegen.cppsrc/codegen/pto/pto_codegen.cppsrc/ir/transforms/mutator.cppsrc/ir/transforms/python_printer.cppsrc/ir/transforms/split_chunked_loops_pass.cppsrc/ir/transforms/visitor.cpptests/ut/core/test_logging.py
| void VisitStmt_(const ForStmtPtr& for_stmt) override { | ||
| SPAN_GUARD(for_stmt->span_); |
There was a problem hiding this comment.
IfStmt is the remaining unguarded statement path here.
VisitStmt_(const IfStmtPtr& if_stmt) at Lines 594-639 still runs without a span scope, so failures while generating the condition or traversing a top-level if will produce an error without the source location even though the neighboring statement visitors now do.
Minimal fix
void VisitStmt_(const IfStmtPtr& if_stmt) override {
+ SPAN_GUARD(if_stmt->span_);
std::string cond_expr = GenerateExprString(if_stmt->condition_);Also applies to: 641-642, 691-692
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/codegen/orchestration/orchestration_codegen.cpp` around lines 547 - 548,
The IfStmt visitor is missing a span scope so errors during condition generation
or traversing a top-level if lack source location; add a
SPAN_GUARD(if_stmt->span_) at the start of VisitStmt_(const IfStmtPtr& if_stmt)
to mirror other statement visitors, and likewise add SPAN_GUARD(...) to the
other unguarded statement visitors referenced (the neighboring visitors around
the same block) to ensure all statement paths are wrapped with the span guard.
| void PTOCodegen::VisitStmt_(const EvalStmtPtr& op) { | ||
| SPAN_GUARD(op->span_); |
There was a problem hiding this comment.
AssignStmt is still the unguarded hot path.
VisitStmt_(const AssignStmtPtr& op) at Lines 491-537 is where most PTO lowering starts, but it still lacks SPAN_GUARD. Any CHECK/INTERNAL_CHECK raised while lowering an assignment RHS will therefore miss the user location on the common codegen path.
Minimal fix
void PTOCodegen::VisitStmt_(const AssignStmtPtr& op) {
+ SPAN_GUARD(op->span_);
if (auto call = As<ir::Call>(op->value_)) {Also applies to: 1061-1062, 1078-1079, 1200-1201
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/codegen/pto/pto_codegen.cpp` around lines 1054 - 1055, The AssignStmt
visitor lacks span guarding so errors during lowering lose user locations; add
SPAN_GUARD(op->span_) as the first statement in VisitStmt_(const AssignStmtPtr&
op) to ensure any CHECK/INTERNAL_CHECK reports the assignment span, and likewise
add the same SPAN_GUARD(op->span_) to the other unguarded VisitStmt_ overloads
referenced (the other VisitStmt_(...) functions missing guards) so all hot-path
statement lowering uses the span guard.
| ExprPtr IRMutator::VisitExpr_(const IterArgPtr& op) { | ||
| SPAN_GUARD(op->span_); |
There was a problem hiding this comment.
This still leaves most mutator failures outside a span scope.
Only IterArg, Call, and ForStmt install a guard here, but this file still has many other INTERNAL_CHECK paths (AssignStmt, IfStmt, MakeTuple, TupleGetItemExpr, the binary/unary mutators, etc.). Because the recursion in this class goes straight through ExprFunctor/StmtFunctor, failures in those paths will still report no user location or keep the enclosing node’s span instead of the failing node’s.
Also applies to: 83-84, 337-338
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ir/transforms/mutator.cpp` around lines 49 - 50, The mutator currently
only installs SPAN_GUARD in a few overloads (e.g., IterArg, Call, ForStmt),
leaving many INTERNAL_CHECK failures outside the correct span; update the
mutator to install the span guard at every visitor entry so failures report the
node's span: either add SPAN_GUARD(op->span_) to the remaining
VisitExpr_/VisitStmt_ overloads (e.g., AssignStmt, IfStmt, MakeTuple,
TupleGetItemExpr and the binary/unary operator visitors) or centralize the guard
by adding it to the common dispatch entry used by ExprFunctor/StmtFunctor before
delegating to specific VisitExpr_/VisitStmt_ methods (ensure
VisitExpr_/VisitStmt_ signatures such as IRMutator::VisitExpr_(const XPtr& op)
and IRMutator::VisitStmt_(const YPtr& op) all run SPAN_GUARD with op->span_).
| void IRVisitor::VisitExpr_(const IterArgPtr& op) { | ||
| SPAN_GUARD(op->span_); |
There was a problem hiding this comment.
Three guarded overloads are not enough for visitor-wide coverage.
MakeTuple, TupleGetItemExpr, the binary/unary visitors, AssignStmt, IfStmt, YieldStmt, ReturnStmt, WhileStmt, and several container statements still call INTERNAL_CHECK without setting the current span. Since this class already recurses through VisitExpr/VisitStmt, guarding those two entry points would make span propagation consistent instead of only covering these three node kinds.
Also applies to: 71-72, 183-184
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ir/transforms/visitor.cpp` around lines 41 - 42, The visitor only guards
a few overloads (e.g., VisitExpr_(IterArgPtr)) which leaves many other nodes
calling INTERNAL_CHECK without a current span; instead, add span propagation at
the central recursion entry points by placing the SPAN_GUARD (or equivalent
span-setting logic) at IRVisitor::VisitExpr and IRVisitor::VisitStmt so every
recursive dispatch sets the current span before calling any
VisitExpr_/VisitStmt_ overloads (this will cover MakeTuple, TupleGetItemExpr,
unary/binary visitors, AssignStmt, IfStmt, YieldStmt, ReturnStmt, WhileStmt and
container statements); update or remove redundant per-overload SPAN_GUARDs
(e.g., the ones at the three existing overloads and lines mentioned) to avoid
duplication and ensure INTERNAL_CHECK always sees a valid span.
| assert start_line <= for_stmt.span.begin_line <= func_end_line, ( | ||
| f"ForStmt span line {for_stmt.span.begin_line} is not within " | ||
| f"this test method (lines {start_line}–{func_end_line})" | ||
| ) |
There was a problem hiding this comment.
Replace en-dash with hyphen-minus.
The static analysis tool flagged an ambiguous en-dash character (–) at line 621. Use a standard hyphen-minus (-) instead for consistency.
Proposed fix
assert start_line <= for_stmt.span.begin_line <= func_end_line, (
f"ForStmt span line {for_stmt.span.begin_line} is not within "
- f"this test method (lines {start_line}–{func_end_line})"
+ f"this test method (lines {start_line}-{func_end_line})"
)📝 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.
| assert start_line <= for_stmt.span.begin_line <= func_end_line, ( | |
| f"ForStmt span line {for_stmt.span.begin_line} is not within " | |
| f"this test method (lines {start_line}–{func_end_line})" | |
| ) | |
| assert start_line <= for_stmt.span.begin_line <= func_end_line, ( | |
| f"ForStmt span line {for_stmt.span.begin_line} is not within " | |
| f"this test method (lines {start_line}-{func_end_line})" | |
| ) |
🧰 Tools
🪛 Ruff (0.15.5)
[warning] 621-621: String contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/ut/core/test_logging.py` around lines 619 - 622, The f-string assertion
in the test (assert start_line <= for_stmt.span.begin_line <= func_end_line)
contains an en-dash character in the message f"...(lines
{start_line}–{func_end_line})"; replace that en-dash with a standard
hyphen-minus so the message becomes f"...(lines {start_line}-{func_end_line})"
to eliminate the ambiguous character while keeping the same formatting and
variables (start_line, for_stmt.span.begin_line, func_end_line).
…ion in errors Introduce SpanGuard RAII class and SPAN_GUARD macro to embed IR node span information into INTERNAL_CHECK failure messages. When a check fails inside a visitor/mutator/codegen method, the error now includes the user's source location (e.g. "(at user_model.py:12:5)") before the standard check-failed line. - Add thread-local `g_current_span_str` and `SpanGuard` to `logging.h` - Add `SPAN_GUARD(span)` macro for use at the top of Visit*_ methods - Add `internal_check_with_span()` C++ helper and Python binding for explicit span-scoped checks from Python - Expose `internal_check_with_span` in Python public API and type stubs - Instrument Visit*_ methods in mutator, visitor, python_printer, split_chunked_loops_pass, and all three codegens with SPAN_GUARD - Add comprehensive tests covering valid span, unknown span, pass-through, and nested guard restoration using real @pl.program IR closes hw-native-sys#477
luohuan19
force-pushed
the
fix-issue-477
branch
from
813973d to
907fece
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/ut/core/test_logging.py (1)
590-590: Remove debugLine 590 contains a
print(msg)that appears to be leftover debug code. It should be removed before merging.🧹 Proposed fix
msg = str(exc_info.value) - print(msg) assert "ForStmt has null loop_var" in msg🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/ut/core/test_logging.py` at line 590, Remove the leftover debug print by deleting the print(msg) statement in tests/ut/core/test_logging.py (the debug print that references msg); if you need to retain visibility for debugging use the existing test logger or assertions instead of print, but do not leave print(msg) in the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/ut/core/test_logging.py`:
- Line 590: Remove the leftover debug print by deleting the print(msg) statement
in tests/ut/core/test_logging.py (the debug print that references msg); if you
need to retain visibility for debugging use the existing test logger or
assertions instead of print, but do not leave print(msg) in the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 92ad138c-4ef8-4f22-bc78-4e3d4f3f6b08
📒 Files selected for processing (13)
include/pypto/core/logging.hpython/bindings/modules/logging.cpppython/pypto/__init__.pypython/pypto/pypto_core/__init__.pyipython/pypto/pypto_core/logging.pyisrc/codegen/cce/cce_codegen.cppsrc/codegen/orchestration/orchestration_codegen.cppsrc/codegen/pto/pto_codegen.cppsrc/ir/transforms/mutator.cppsrc/ir/transforms/python_printer.cppsrc/ir/transforms/split_chunked_loops_pass.cppsrc/ir/transforms/visitor.cpptests/ut/core/test_logging.py
🚧 Files skipped from review as they are similar to previous changes (6)
- src/codegen/cce/cce_codegen.cpp
- python/pypto/pypto_core/init.pyi
- src/codegen/orchestration/orchestration_codegen.cpp
- src/codegen/pto/pto_codegen.cpp
- src/ir/transforms/mutator.cpp
- src/ir/transforms/split_chunked_loops_pass.cpp
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a SpanGuard RAII class and SPAN_GUARD macro, which is a great feature for improving error diagnostics by including IR source locations in INTERNAL_CHECK failure messages. The implementation using a thread_local variable is clean and thread-safe. The feature is well-integrated throughout the codebase by instrumenting visitor and codegen methods, and the new tests, including end-to-end tests with @pl.program, are comprehensive. Overall, this is a high-quality addition to the project.
1 participant
Introduce SpanGuard RAII class and SPAN_GUARD macro to embed IR node span information into INTERNAL_CHECK failure messages. When a check fails inside a visitor/mutator/codegen method, the error now includes the user's source location (e.g. "(at user_model.py:12:5)") before the standard check-failed line.
g_current_span_strandSpanGuardtologging.hSPAN_GUARD(span)macro for use at the top of Visit*_ methodsinternal_check_with_span()C++ helper and Python binding for explicit span-scoped checks from Pythoninternal_check_with_spanin Python public API and type stubscloses #477