Add MASM lint mode for advice taint analysis - #1263
Conversation
1fe2758 to
d46ac63
Compare
d46ac63 to
8839518
Compare
bitwalker
left a comment
bitwalker
left a comment
There was a problem hiding this comment.
Looking good! I found a couple of issues, one of which may require a change in Linker, but fairly trivial for the most part. I can get this merged as soon as those are fixed (or we have a plan for fixing them), and will ship a new release of the compiler very soon (basically once this and possibly a couple of other pending PRs land).
| return Some(unsupported); | ||
| } | ||
| } | ||
| Op::While { body, .. } | Op::DoWhile { body, .. } | Op::Repeat { body, .. } => { |
There was a problem hiding this comment.
This pre-scan treats Op::DoWhile as liftable, but signature validation reaches the unconditional infer_do_while todo!(), and body lifting contains another todo!(). Valid MASM panics instead of recording this procedure as skipped. We should reject the construct during pre-scan or implement both paths.
We should also inspect the condition block for DoWhile
There was a problem hiding this comment.
Fixed, preflight now rejects the whole do-while procedure.
| **procedure = stub; | ||
| } | ||
|
|
||
| linker.link(core::iter::empty(), core::iter::empty())?; |
There was a problem hiding this comment.
This final linker pass rejects cyclic call graphs before infer_missing_signatures reaches the new cycle-to-SkippedProcedure handling. A recursive component aborts the entire lint world, including unrelated procedures.
I think we may need to add some support to the linker to allow proceeding in the face of certain errors by collecting them and proceeding with linking anyway (i.e. if we encounter recursion, the linker can obviously resolve the symbol references, even if the resulting call graph cannot be assembled to MAST). In such cases the linker could record the cycle as a deferred error, and proceed with linking anyway. The assembler could then consult the linker for non-fatal errors it gathered, and raise them at that point, while the disassembler could raise diagnostics for those without bailing out of disassembly completely.
The alternative is to detect and stub cyclic SCCs before this, but then we're reimplementing stuff the linker already does for us.
| )?, | ||
| }; | ||
|
|
||
| if config.lint { |
There was a problem hiding this comment.
Signature validation does not cover lift-only invariants like local index bounds. An out-of-range loc_loadw_le passes here and later escapes through lift_bodies, aborting the whole partial world. We probably need to make lifting transactional per procedure or validate every lifting precondition before declaration.
| context: Rc<Context>, | ||
| ) -> Result<DisassembledWorld> { | ||
| let inputs = if let Some(sources) = sources { | ||
| let metadata = project::collect_dependency_metadata(project, &context)?; |
There was a problem hiding this comment.
This collector loads only dependencies declared directly by the root project. A direct source dependency that calls a transitive source dependency is marked skipped, and that skip propagates into valid root code. This should probably accept a resolved dependency graph/full closure as the existing source-based API does.
There was a problem hiding this comment.
Fixed, lint disassembly now takes the parsed sources and resolved dependency graph, then reads metadata from the full dependency closure.
8839518 to
0eb8cac
Compare
a500e2a to
5d83a3b
Compare
Build a reverse caller graph during lint preflight so skipped procedures propagate without repeated full-module scans. Keep root failures actionable and avoid recursively duplicating dependency reasons.
Count queued analysis owners with a hash map before sorting the top summary so budget exhaustion remains inexpensive on large data-flow graphs.
5d83a3b to
e036516
Compare
bitwalker
left a comment
bitwalker
left a comment
There was a problem hiding this comment.
Looks good, thanks! I've added a few commits to address a couple of minor review findings, rather than do another review cycle just for those, but nothing substantial has changed.
2 participants
This PR lets the MASM frontend build a partial HIR world for lint use. This is part of #1209.
Strict MASM disassembly stops on the first procedure it cannot lift. That blocks advice taint analysis on large MASM projects, including the Miden VM core library. This change adds a lint path that skips procedures the frontend cannot lift yet. It records the procedure path, source span, and reason. It also skips callers that depend on skipped procedures.
This change also adds support for
mod.masmmodule index files, relative calls through declared child modules, source dependency metadata, andexp.u32.This PR does not wire this into
midenc --lint. That should be a follow-up PR, with a clear policy for skipped procedures and partial analysis.