-
Notifications
You must be signed in to change notification settings - Fork 1.8k
C#: SSA refactorings #4923
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
C#: SSA refactorings #4923
Conversation
0c641dc
to
63f76b1
Compare
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.
Found some minor typos, and added a couple of questions.
implicitEntryDefinition(bb, v) and | ||
i = -1 and | ||
certain = true |
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.
implicitEntryDefinition
covers captured variables, but the writes of captured variables might not be actually executed. So wouldn't it be more accurate to set certain
to false
?
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.
It doesn't matter, actually, as there cannot be any prior definitions.
lsv.isRef() and | ||
strictcount(v.getAnAccess()) > 1 |
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 case is for ref
locals. Why do we need strictcount(v.getAnAccess()) > 1
?
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.
Yes, it is for ref
locals. The check is there to avoid SSA definitions for cases like this: https://github.com/github/codeql/blob/main/csharp/ql/test/library-tests/dataflow/ssa/Consistency.cs#L55. Before the refactoring, this was taken care of by https://github.com/github/codeql/pull/4923/files#diff-7a5016d3998d3aa9a71ee689d871333e9bffa227aae862f5d5183a131fdc4f40L1800-L1810, but since we are now using a common liveness analysis, I applied this (less precise) check instead.
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.
LGTM
SSA refactorings in preparation for a shared SSA library implementation.
The first commit removes the class
ImplicitUntrackedDefinition
, which was used to generate SSA definitions for variables not amenable to SSA analysis. The class was never actually used.The second commit unifies how SSA definitions are constructed, by collapsing the existing
TSsaExplicitDef
,TSsaImplicitEntryDef
,TSsaImplicitCallDef
, andTSsaImplicitQualifierDef
IPA injectors into a singleTWriteDef
injector. This has the benefit that onlyvariableWrite()
is used to construct SSA definitions (a predicate similar to that will be part of the API for a shared implementation), and a common liveness analysis is applied in all cases (though in some corner cases this is less precise).Liveness of writes to captured variables, where reads happen inside capturing callables, is ensured by adding a new (pseudo) read of kind
CapturedVarCallRead
at calls that may reach a read. This means that the common liveness analysis can be applied here as well. For example, ina pseudo read of
captured
is added at the call toInner
, which makes the writecaptured = 0
live.https://jenkins.internal.semmle.com/job/Changes/job/CSharp-Differences/751/