-
Notifications
You must be signed in to change notification settings - Fork 76
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
Use TrieDomain to distribute accesses to contained fields #1089
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Co-authored-by: Simmo Saan <simmo.saan@gmail.com>
Co-authored-by: Simmo Saan <simmo.saan@gmail.com>
Co-authored-by: Simmo Saan <simmo.saan@gmail.com>
Co-authored-by: Simmo Saan <simmo.saan@gmail.com>
Co-authored-by: Simmo Saan <simmo.saan@gmail.com>
Co-authored-by: Simmo Saan <simmo.saan@gmail.com>
Co-authored-by: Simmo Saan <simmo.saan@gmail.com>
Co-authored-by: Simmo Saan <simmo.saan@gmail.com>
Co-authored-by: Simmo Saan <simmo.saan@gmail.com>
Co-authored-by: Simmo Saan <simmo.saan@gmail.com>
sim642
added
bug
unsound
pr-dependency
Depends or builds on another PR, which should be merged before
labels
Jun 15, 2023
This was referenced Jun 22, 2023
Where is the PR-dependency here? Does it make sense to review this as of now? |
sim642
removed
the
pr-dependency
Depends or builds on another PR, which should be merged before
label
Jul 8, 2023
The dependency was #1084, which is now merged, so this can be reviewed. |
michael-schwarz
approved these changes
Jul 11, 2023
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.
Apart from the comment of deriving eq
for type
, LGTM!
Thanks for the clarification!
…On Tue, Jul 11, 2023, 4:46 PM Simmo Saan ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/domains/access.ml
<#1089 (comment)>:
> @@ -80,11 +80,34 @@ let reset () =
type acc_typ = [ `Type of CilType.Typ.t | `Struct of CilType.Compinfo.t * Offset.Unit.t ] [@@deriving eq, ord, hash]
(** Old access type inferred from an expression. *)
+module MemoRoot =
+struct
+ include Printable.StdLeaf
+ type t = [`Var of CilType.Varinfo.t | `Type of CilType.Typ.t] [@@deriving eq, ord, hash]
CilType.Typ exists precisely to take care of it. The recursion in Cil.typ
structural comparison comes from compinfos and CilType.Compinfo has an
implementation that only compares cids.
—
Reply to this email directly, view it on GitHub
<#1089 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADJME3O6SJMICP5S55ZZSZDXPVRN5ANCNFSM6AAAAAAZIBJUJA>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
This was referenced Aug 2, 2023
sim642
added a commit
to sim642/opam-repository
that referenced
this pull request
Sep 13, 2023
CHANGES: * Add `setjmp`/`longjmp` analysis (goblint/analyzer#887, goblint/analyzer#970, goblint/analyzer#1015, goblint/analyzer#1019). * Refactor race analysis to lazy distribution (goblint/analyzer#1084, goblint/analyzer#1089, goblint/analyzer#1136, goblint/analyzer#1016). * Add thread-unsafe library function call analysis (goblint/analyzer#723, goblint/analyzer#1082). * Add mutex type analysis and mutex API analysis (goblint/analyzer#800, goblint/analyzer#839, goblint/analyzer#1073). * Add interval set domain and string literals domain (goblint/analyzer#901, goblint/analyzer#966, goblint/analyzer#994, goblint/analyzer#1048). * Add affine equalities analysis (goblint/analyzer#592). * Add use-after-free analysis (goblint/analyzer#1050, goblint/analyzer#1114). * Add dead code elimination transformation (goblint/analyzer#850, goblint/analyzer#979). * Add taint analysis for partial contexts (goblint/analyzer#553, goblint/analyzer#952). * Add YAML witness validation via unassume (goblint/analyzer#796, goblint/analyzer#977, goblint/analyzer#1044, goblint/analyzer#1045, goblint/analyzer#1124). * Add incremental analysis rename detection (goblint/analyzer#774, goblint/analyzer#777). * Fix address sets unsoundness (goblint/analyzer#822, goblint/analyzer#967, goblint/analyzer#564, goblint/analyzer#1032, goblint/analyzer#998, goblint/analyzer#1031). * Fix thread escape analysis unsoundness (goblint/analyzer#939, goblint/analyzer#984, goblint/analyzer#1074, goblint/analyzer#1078). * Fix many incremental analysis issues (goblint/analyzer#627, goblint/analyzer#836, goblint/analyzer#835, goblint/analyzer#841, goblint/analyzer#932, goblint/analyzer#678, goblint/analyzer#942, goblint/analyzer#949, goblint/analyzer#950, goblint/analyzer#957, goblint/analyzer#955, goblint/analyzer#954, goblint/analyzer#960, goblint/analyzer#959, goblint/analyzer#1004, goblint/analyzer#558, goblint/analyzer#1010, goblint/analyzer#1091). * Fix server mode for abstract debugging (goblint/analyzer#983, goblint/analyzer#990, goblint/analyzer#997, goblint/analyzer#1000, goblint/analyzer#1001, goblint/analyzer#1013, goblint/analyzer#1018, goblint/analyzer#1017, goblint/analyzer#1026, goblint/analyzer#1027). * Add documentation for configuration JSON schema and OCaml API (goblint/analyzer#999, goblint/analyzer#1054, goblint/analyzer#1055, goblint/analyzer#1053). * Add many library function specifications (goblint/analyzer#962, goblint/analyzer#996, goblint/analyzer#1028, goblint/analyzer#1079, goblint/analyzer#1121, goblint/analyzer#1135, goblint/analyzer#1138). * Add OCaml 5.0 support (goblint/analyzer#1003, goblint/analyzer#945, goblint/analyzer#1162).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Builds on #1084.
Instead of distributing an access-to-a-struct to all of the fields in that struct, the implementation distributes to those fields only that had direct access(es) themselves. Meaning that if there is access to a struct only, there will be one warning for that struct instead of a separate warning for each of the struct fields. And if there was access to a field of a struct, a race between the access to that struct's field and the access to the whole struct is not lost.
For that, the accesses are collected in a
trie
structure representing a struct. In thetrie
node, there is a set of accesses to that offset and a map of that offset's children with the child offset as a key and a sub-trie
as a value. Later, the accesses of each node's ancestors are also checked for races with the node accesses, doing the distribution.Also fixes soundness issue described in #978.
Previously
Access.add_distribute_inner
was based on the type. Since alloc variables havevoid
type, contained fields for a blob could not be iterated and distributed to. Since this delays the inner distribution and flips the direction, the issue is automatically avoided: an access to(alloc).foo
will be checked against an access to just(alloc)
simply due to the trie structure.