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

Detect renamed functions and global variables. #774

Merged
merged 102 commits into from
May 31, 2023

Conversation

TimOrtel
Copy link
Contributor

@TimOrtel TimOrtel commented Jul 4, 2022

This pull requests builds on top of my pull request about detection renamed local variables.

It implements an algorithm, that can detect renamed functions and global variables on an incremental run. The algorithm is non recursive.

The following changes have been made:

  • Moved eqF from compareCIL.ml to detectGlobals.ml
  • Removed logic that would just compare globals with matching names with my algorithm implemented in detectRenamedFunctions.ml
  • compareAST.ml is now fully functional, because it cannot have side effects when two wrong functions are compared. Previous side-effective hacks in compareAST have been moved out and are now applied after it is clear that the functions actually match.

The algorithm works like this (simplified):

  1. Find functions and global variables with matching names.
  2. Test these function pairs:
    2.1. Foreach function pair detected in 1.: Compare the functions bodies of the functions.
    2.2. If a function reports back that it depends on a rename of a global variable/function, those dependecies are checked in the next step.
    2.3. Foreach dependency, check if they do not rely on additional dependencies themselves. If this assumption holds for all dependencies, the original function pair from 2.1 is marked as renamed, otherwise it is marked as changed.
  3. Report back the results to compareCIL and apply the change data correctly.

I added test cases, but they are not verified automatically, meaning they have to be verified manually.

# Conflicts:
#	src/incremental/compareCIL.ml
#	src/util/server.ml
@michael-schwarz
Copy link
Member

If I understand correctly, this is now finished and could be merged after another round of reviews?

@stilscher
Copy link
Member

Yes, another round of feedback would be great. I think I also still need to make sure that the new incremental cram tests are also automatically executed in the CI. Other than that I think it is mostly cleaned up and ready for review.

@michael-schwarz michael-schwarz requested review from jerhard and sim642 and removed request for sim642 and jerhard March 21, 2023 12:02
src/framework/constraints.ml Outdated Show resolved Hide resolved
src/util/server.ml Outdated Show resolved Hide resolved
tests/incremental/04-var-rename/01-rename_and_shuffle.t Outdated Show resolved Hide resolved
src/incremental/compareAST.ml Outdated Show resolved Hide resolved
src/incremental/compareAST.ml Outdated Show resolved Hide resolved
src/incremental/compareAST.ml Outdated Show resolved Hide resolved
src/incremental/compareAST.ml Outdated Show resolved Hide resolved
src/incremental/compareAST.ml Show resolved Hide resolved
src/incremental/compareAST.ml Outdated Show resolved Hide resolved
src/incremental/compareCFG.ml Outdated Show resolved Hide resolved
@sim642
Copy link
Member

sim642 commented Mar 22, 2023

I think I also still need to make sure that the new incremental cram tests are also automatically executed in the CI.

That probably needs a step like this in the CI workflows:

      - name: Test incremental cram
        run: opam exec -- dune runtest tests/incremental

src/incremental/compareCIL.ml Outdated Show resolved Hide resolved
src/incremental/compareCIL.ml Outdated Show resolved Hide resolved
@stilscher stilscher requested a review from sim642 April 27, 2023 11:49
@michael-schwarz
Copy link
Member

@stilscher should we aim to get this merged before we hit the 11 month anniversary of this PR next Monday?

@stilscher
Copy link
Member

I thought it would be good to wait for an approval in the reviews before merging.

Copy link
Member

@sim642 sim642 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the comments have been resolved, so this should be good to go.

@stilscher stilscher merged commit 7379f9e into goblint:master May 31, 2023
@TimOrtel
Copy link
Contributor Author

🎉

@sim642 sim642 added this to the v2.2.0 milestone Sep 11, 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
Labels
feature performance Analysis time, memory usage student-job
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extend cfg comparison with local rename mapping
4 participants