-
Notifications
You must be signed in to change notification settings - Fork 5
Pr/83 #111
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Pr/83 #111
Conversation
|
Hey @cds-amal looks great, I am excited to give this a go later on. Our CI is currently stuck on Code Quality, this can be fixed by running |
dkcumming
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome stuff, I'm loving the whole thing. Some minor fix ups, and I would personally like to add the fast fail if dot is not installed with an informative message.
Do you think you could rebase and consider the suggestions?
Also what do you think about the ut8? I think we leave it for another issue right now so we get this in faster (sorry for my delay in getting this reviewed)
| clean-graphs: | ||
| @rm -rf $(OUTDIR_DOT) $(OUTDIR_SVG) $(OUTDIR_PNG) $(OUTDIR_D2) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| clean-graphs: | |
| @rm -rf $(OUTDIR_DOT) $(OUTDIR_SVG) $(OUTDIR_PNG) $(OUTDIR_D2) | |
| clean-graphs: | |
| @rm -rf $(OUTDIR_DOT) $(OUTDIR_SVG) $(OUTDIR_PNG) $(OUTDIR_D2) | |
| check-graphviz: | |
| @command -v dot >/dev/null 2>&1 || { echo "Error: graphviz not installed. Install with: apt install graphviz"; exit 1; } | |
| fi | ||
| bash tests/ui/run_ui_tests.sh "$$RUST_DIR_ROOT" "${VERBOSE}" | ||
|
|
||
| .PHONY: dot svg png d2 clean-graphs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| .PHONY: dot svg png d2 clean-graphs | |
| .PHONY: dot svg png d2 clean-graphs check-graphviz |
| mv $$name.smir.dot $(OUTDIR_DOT)/ 2>/dev/null || true; \ | ||
| done | ||
|
|
||
| svg: dot |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| svg: dot | |
| svg: check-graphviz dot |
| dot -Tsvg $$dotfile -o $(OUTDIR_SVG)/$$name.svg; \ | ||
| done | ||
|
|
||
| png: dot |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| png: dot | |
| png: check-graphviz dot |
| let legend_lines = ctx.allocs_legend_lines(); | ||
| if legend_lines.is_empty() { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since ctx.allocs_legend_lines(); will always add "ALLOCS" to the output I believe this early return is never possible to be taken right?
| let desc = if is_str && concrete_bytes.iter().all(|b| b.is_ascii()) { | ||
| let s: String = concrete_bytes | ||
| .iter() | ||
| .take(20) | ||
| .map(|&b| b as char) | ||
| .collect::<String>() | ||
| .escape_default() | ||
| .to_string(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this could be changed to work for utf8. Although the code would need to change from take(20) because you are not guaranteed that a char will fit 1 byte so 20 might be in the middle of a char. I don't know if this is needed for this PR though, I would be happy to make an issue for it to handle utf8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One does not casually reason about the wide and skinny bytes in a PR; when implementing this, I wanted to avoid the issue and should have called it out. Would appreciate if we could raise an issue, so a fix is more focused, that would be a good-first-contrib category.
Restructure the graph generation to build indices upfront rather than resolving relationships at traversal time. This improves maintainability and enables richer constant/allocation information in the output. Add new data structures for graph context: - AllocIndex: maps AllocId to processed AllocEntry with descriptions - AllocEntry: contains alloc metadata and human-readable description - AllocKind: categorizes allocs as Memory/Static/VTable/Function - TypeIndex: maps type IDs to display names - GraphContext: holds all indices and provides rendering methods Add ALLOCS legend node to graphs: - Display all GlobalAlloc entries in a yellow info node - Show allocation ID, type, and decoded value where possible - Decode string literals as escaped ASCII - Decode small integers as numeric values Enhance constant operand labels: - Show provenance references: `const [alloc0: Int(I32) = 42]` - Display inline constant values with types: `const 42_Uint(Usize)` - Show function names for ZeroSized function constants Add context-aware rendering functions: - render_stmt_ctx: renders statements with alloc context - render_rvalue_ctx: renders rvalues with operand context - render_intrinsic_ctx: renders intrinsics with operand context Update to_dot_file to use new architecture: - Build GraphContext before consuming SmirJson - Pass context to statement/operand rendering - Use context for call edge argument labels Add accessor methods to AllocInfo in printer.rs: - alloc_id(): returns the allocation ID - ty(): returns the type - global_alloc(): returns reference to GlobalAlloc
Add alternative graph output format using D2 language alongside existing DOT format. D2 offers modern diagramming with better text rendering and can be viewed in browser via d2lang.com playground. Changes: - Add --d2 command-line flag for D2 output - Add to_d2_file() method to SmirJson for D2 rendering - Add emit_d2file() entry point - Add render_terminator_ctx() and terminator_targets() helpers - Add resolve_call_target() method to GraphContext - Add escape_d2() to handle $ and other special characters in labels Output includes: - ALLOCS legend with allocation descriptions - Function containers with basic blocks - Block-to-block control flow edges - Call edges to external functions
- Change &String to &str in short_name() and block_name() signatures - Extract bytes_to_u64_le() helper for repeated byte-to-int conversion - Fix expect() messages to properly interpolate path using unwrap_or_else
Extract D2 rendering into focused helper functions: - render_d2_allocs_legend - render_d2_function - render_d2_blocks - render_d2_block_edges - render_d2_call_edges - render_d2_asm - render_d2_static
Break up rather large mk_graph.rs into focused teeny tiny-er modules: - mk_graph/mod.rs: Entry points (emit_dotfile, emit_d2file) - mk_graph/index.rs: AllocIndex, TypeIndex, AllocEntry structures - mk_graph/context.rs: GraphContext with rendering methods - mk_graph/util.rs: GraphLabelString trait and helper functions - mk_graph/output/dot.rs: DOT (Graphviz) rendering - mk_graph/output/d2.rs: D2 diagram rendering This improves maintainability and makes it easier to add new output formats in the future.
- dot: Generate .dot files in output-dot/ - svg: Convert dot files to SVG (requires graphviz) - png: Convert dot files to PNG (requires graphviz) - d2: Generate .d2 files in output-d2/ Update README with usage instructions.
Leverage the LayoutShape and field type information recently added to TypeMetadata (commits 74a4261, 18f452d) to provide actual useful data instead of leaving it to gather dust in the JSON output. Introduce TypeEntry, LayoutInfo, and associated structures to index.rs, replacing the rather spartan string-only type index with something that knows about field offsets, sizes, alignments, and discriminants. One might argue this is what a type index ought to have done from the start. Add rendering methods to GraphContext for displaying types with their memory layouts. The DOT output now includes a TYPES legend (in a fetching shade of lavender) and locals are annotated with size and alignment. This allows tooling to answer questions like "how large is this struct" and "at what offset does field 3 live" without resorting to prayer or manual calculation.
Co-authored-by: Daniel Cumming <124537596+dkcumming@users.noreply.github.com>
e9395d9
into
runtimeverification:master
Summary
This PR enhances the MIR graph visualization with richer allocation information and adds D2 as an alternative output format.
Key changes:
AllocIndex,TypeIndex, andGraphContextupfront before graph traversal, enabling context-aware rendering throughoutGlobalAllocentries with decoded values (strings as ASCII, small integers as numeric values)const [alloc0: Int(I32) = 42]instead of opaqueconst ?_i32Addresses
#83
Example
Before:
const ?_i32After:
New CLI usage
New Make targets
Test plan