Skip to content
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

Fixpoint not reached in some incremental runs on chrony #1091

Closed
jerhard opened this issue Jun 19, 2023 · 11 comments · Fixed by #1104
Closed

Fixpoint not reached in some incremental runs on chrony #1091

jerhard opened this issue Jun 19, 2023 · 11 comments · Fixed by #1104
Assignees
Milestone

Comments

@jerhard
Copy link
Member

jerhard commented Jun 19, 2023

Using the current master, 35 runs of the incremental analysis do not reach the fixpoint on chrony. This occurred with the regular incremental configuration. In all of the 35 runs, the unreached fixpoint is detected at the same code location within chrony.
In the experiments, it does not manifest in the runs where the incremental postsolver is active (with reluctant incremental analysis on or off).

Difference between rhs and computed solution.
Difference: Map: Reversed (top or Set (address (lval (offset (IntDomLifter(intdomtuple)))) * booleans)) * bottom or map:({}, mapping {
                                                                                                                                }) =
 Map: messages =
 trivial arrays: Array: mapping {
                          addr_type ->   Uninitialized
                          data ->   {}
                          descriptor ->   Uninitialized
                          if_index ->   Uninitialized
                          length ->   Uninitialized
                          local_addr ->
                            (If you see this, you are special!, Unknown)
                          remote_addr ->
                            (If you see this, you are special!, Unknown)
                          timestamp ->
                            mapping {
                              hw ->   mapping {
                                        tv_nsec ->   Uninitialized
                                        tv_sec ->   Uninitialized
                                      }
                              if_index ->   Uninitialized
                              kernel ->   mapping {
                                            tv_nsec ->   Uninitialized
                                            tv_sec ->   Uninitialized
                                          }
                              l2_length ->   Uninitialized
                              tx_flags ->   Uninitialized
                            }
                        } not leq Array: mapping {
                                             addr_type ->   Uninitialized
                                             data ->   {}
                                             descriptor ->   Uninitialized
                                             if_index ->   Uninitialized
                                             length ->   Uninitialized
                                             local_addr ->
                                               (If you see this, you are special!, Uninitialized)
                                             remote_addr ->
                                               (If you see this, you are special!, Uninitialized)
                                             timestamp ->
                                               mapping {
                                                 hw ->   mapping {
                                                           tv_nsec ->
                                                             Uninitialized
                                                           tv_sec ->
                                                             Uninitialized
                                                         }
                                                 if_index ->   Uninitialized
                                                 kernel ->   mapping {
                                                               tv_nsec ->
                                                                 Uninitialized
                                                               tv_sec ->
                                                                 Uninitialized
                                                             }
                                                 l2_length ->   Uninitialized
                                                 tx_flags ->   Uninitialized
                                               }
                                           }
@jerhard
Copy link
Member Author

jerhard commented Jun 26, 2023

The fixpoint is not reached at a call:

Fixpoint not reached at L:node 47288 "received = SCK_ReceiveMessages(sock_fd,
                                                                     messages,
                                                                     4, flags);" on chrony/ntp_io.c:426:3-426:62

The line number chrony/ntp_io.c:426:3-426:62 is reported on this small commit, where the fixpoint issue also arises.

@jerhard
Copy link
Member Author

jerhard commented Jun 26, 2023

The fixpoint is also not reached for this commit where only a function is added. Goblint accurately detects that only a function definition is added, and there are no evals recorded for this commit.

@jerhard
Copy link
Member Author

jerhard commented Jun 28, 2023

@stilscher pointed out that this may have to do with anonymous types in C. Indeed, it turns out the type of local_addr is an anonymous union or anonymous struct (in the commits I've checked), see here: https://github.com/mlichvar/chrony/blob/934b8712a57e324581a15ddb4f6c84cd67a5673e/socket.h#L64-L66

Previously, there were similar fixpoint issues that were caused by different names and/or ckeys assigned to the anonymous types by CIL. This may be a similar issue.

@sim642
Copy link
Member

sim642 commented Jun 28, 2023

Something about anonymous structs came up in #678 but maybe that aspect wasn't fixed?
I haven't looked into the details but maybe incremental comparison of struct types should ignore names of anonymous structs and consider them equal if the contents are?

@jerhard
Copy link
Member Author

jerhard commented Jul 3, 2023

I had a look at the previously mentioned commit where only a function was added. For the parent commit of that commit, at the node, where the incremental analysis of the child does not reach the fixpoint, the solver computes local_addr -> (If you see this, you are special!, Uninitialized) as does the right-hand side in the postsolving.

From this, I would conclude for the child commit, that the right-hand-side that is not subsumed, that yields local_addr -> (If you see this, you are special!, Unknown), is the problem (rather than the solution not being accessed correctly). For the parent commit the computed solution subsumes the right-hand-side; and there are no relevant code changes that should change that fact.

Computed value in non-incremental run
{([Unit:(), Reversed (top or Set (varinfo)):{}, top or Set (Set (exp)):{},
    Reversed (top or Set (exp)):{}, Unit:(),
    Reversed (top or Set (address (lval (offset (IntDomLifter(intdomtuple)))) * booleans)) * bottom or map:({}, mapping {
                                                                                                                  }),
    Unit:(), Unit:(), top or Set (variables):{line}, booleans:False,
    MT mode:Multithreaded (main),
    lifted varinfo * node * chain option * Thread * lifted created (once) * created (multiple times) and Unit:(unknown node, main, ()),
    value domain * array partitioning deps * Vars with Weak Update * Unit:(mapping {
                                                                             Local {
                                                                               messages ->
                                                                                 (Array: mapping {
                                                                                           addr_type ->
                                                                                             Uninitialized
                                                                                           data ->
                                                                                             {}
                                                                                           descriptor ->
                                                                                             Uninitialized
                                                                                           if_index ->
                                                                                             Uninitialized
                                                                                           length ->
                                                                                             Uninitialized
                                                                                           local_addr ->
                                                                                             (If you see this, you are special!, Uninitialized)
                                                                                           remote_addr ->
                                                                                             (If you see this, you are special!, Uninitialized)
                                                                                           timestamp ->
                                                                                             mapping {
                                                                                               hw ->
                                                                                                 mapping {
                                                                                                   tv_nsec ->
                                                                                                     Uninitialized
                                                                                                   tv_sec ->
                                                                                                     Uninitialized
                                                                                                 }
                                                                                               if_index ->
                                                                                                 Uninitialized
                                                                                               kernel ->
                                                                                                 mapping {
                                                                                                   tv_nsec ->
                                                                                                     Uninitialized
                                                                                                   tv_sec ->
                                                                                                     Uninitialized
                                                                                                 }
                                                                                               l2_length ->
                                                                                                 Uninitialized
                                                                                               tx_flags ->
                                                                                                 Uninitialized
                                                                                             }
                                                                                         }, (4,[4,4]))
                                                                               i ->
                                                                                 (Unknown int([-31,31]),[-2147483648,2147483647])
                                                                               received ->
                                                                                 (Unknown int([-31,31]),[-2147483648,2147483647])
                                                                               flags ->
                                                                                 (Unknown int([0,1]),[0,1])
                                                                             }
                                                                             Parameter {
                                                                               sock_fd ->
                                                                                 (Unknown int([-31,31]),[-2147483648,2147483647])
                                                                               event ->

@jerhard
Copy link
Member Author

jerhard commented Jul 6, 2023

The first commit of Goblint in the recent past where Fixpoint appears for the output logs on a select subset of chrony-commits is the commit (8d22c45).

I still have to have a look whether there are other issues at the previous commit that may mask the issue in the previous commit (e.g. if more runs were failing due to other reasons).
Edit: It turns out that on the previous goblint commit (53938bc) all the incremental runs fail. This is due to the some reflifts that were still implemented via failwiths before.

Edit: With the parent commit of 53938bc, which is d025538, no fixpoint issue appears on the tested chrony commits.
So it seems that the issue manifested with 8d22c45 and 53938bc.

@jerhard
Copy link
Member Author

jerhard commented Jul 9, 2023

Undoing the changes to the relift functions as in d34cd61ef00534cdd552e7a7ca9f83c544d5ce84 indeed leads to the Fixpoint warnings going away on the chrony commits tested.
Since the the relift functions were looking meaningful, it may be that reverting back justs hides the issue somehow. It may also be that the implementation of the relift in some of these places may have lead to inconsistencies compared to modules where the relifting is not done.

@michael-schwarz
Copy link
Member

This morning @jerhard and I were able to narrow it down: It is the addition of relifting to the CPA, that causes the analysis to start failing. Curiously, CPA is HashCached, so that may have something to do with it.

@jerhard
Copy link
Member Author

jerhard commented Jul 10, 2023

I removed the HashCached Lifter for the CPA, and the fixpoint issue remains.

@jerhard
Copy link
Member Author

jerhard commented Jul 10, 2023

When deactivating the relift of arrays in ValueDomain.relift on the current master-branch, no fixpoints issues are reported on the tested chrony commits.

@michael-schwarz
Copy link
Member

That is very strange: You said you are not using partitioned arrays, right?

jerhard added a commit that referenced this issue Jul 11, 2023
Issue #1091 was caused by the join/widen (and corresponding smart operations)
for unions in ValueDomain not return the same element.
The optimization in PMap that returns the unchanged map when two
physically identical maps were joined was thus yielding a different value in the
from-scratch run compared to the incremental run.
@sim642 sim642 added this to the v2.2.0 milestone Jul 18, 2023
sim642 added a commit to sim642/opam-repository that referenced this issue 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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants