Ssa: Refactor data flow integration to make the input signature simpler #19147
Conversation
aschackmull
added
the
no-change-note-required
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
Files not reviewed (11)
- cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaInternals.qll: Language not supported
- csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPrivate.qll: Language not supported
- csharp/ql/lib/semmle/code/csharp/dataflow/internal/SsaImpl.qll: Language not supported
- java/ql/lib/semmle/code/java/dataflow/internal/DataFlowNodes.qll: Language not supported
- java/ql/lib/semmle/code/java/dataflow/internal/SsaImpl.qll: Language not supported
- javascript/ql/lib/semmle/javascript/dataflow/internal/sharedlib/Ssa.qll: Language not supported
- ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll: Language not supported
- ruby/ql/lib/codeql/ruby/dataflow/internal/SsaImpl.qll: Language not supported
- rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll: Language not supported
- rust/ql/lib/codeql/rust/dataflow/internal/SsaImpl.qll: Language not supported
- shared/ssa/codeql/ssa/Ssa.qll: Language not supported
Tip: Copilot only keeps its highest confidence comments to reduce noise and keep you focused. Learn more
github-actions
bot
added
C#
JS
C++
Java
Ruby
Rust
hvitved
left a comment
hvitved
left a comment
There was a problem hiding this comment.
LGTM, two minor comments.
shared/ssa/codeql/ssa/Ssa.qll
Outdated
| WriteDefinition getDefinition() { result = def } | ||
|
|
||
| override string toString() { result = p.toString() } | ||
| override string toString() { result = def.toString() } |
There was a problem hiding this comment.
Perhaps add a [source] prefix or suffix to make it easier to debug?
| } | ||
|
|
||
| predicate ssaDefInitializesParam(WriteDefinition def, Parameter p) { p.isInitializedBy(def) } | ||
|
|
There was a problem hiding this comment.
Should ssaDefHasSource be implemented, restricting to any(Parameter p).isInitializedBy(def) or def.(Ssa::WriteDefinition).assigns(_)?
There was a problem hiding this comment.
Optionally, yes. It makes no semantic difference, it only causes a slight reduction in generated nodes (nodes that are of course unreachable by any step). I figured that such a restriction was likely most of the write definitions, which is why I didn't bother originally, but I just checked, and there is a noticeable count difference, so I'll add it.
hvitved
left a comment
hvitved
left a comment
There was a problem hiding this comment.
Remember DCA before merging.
2 participants
This replaces the two predicates,
ssaDefAssignsandssaDefInitializesParam, with a per-default synthesized input-value node that can then be mapped to the RHS of an assignment or the parameter as appropriate.The changes are split into individual commits that can be reviewed one-by-one, but the combined changes are simple enough that it's possibly just as easy to just look at all the changes together.