analyzer icon indicating copy to clipboard operation
analyzer copied to clipboard

Published 20 hours ago • verified publisher icon goblint

Detect renamed functions and global variables.

Open TimOrtel opened this issue 3 years ago • 3 comments

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.

TimOrtel avatar Jul 04 '22 12:07 TimOrtel

What's the explanation for closing this without merging?

sim642 avatar Oct 12 '22 07:10 sim642

I do not have time to work on this pull request anymore and I do not want to give the impression that I will be able to work on this in the future by keeping this open. However, if someone else wants to finish this PR feel free to reopen it.

TimOrtel avatar Oct 12 '22 09:10 TimOrtel

Ok, I re-opened it and assigned it to @stilscher, as she likely knows what needs to be done so it can be merged.

michael-schwarz avatar Oct 12 '22 10:10 michael-schwarz

@stilscher: What is the plan here? Having PRs go so stale is a bit problematic. We should either merge or abandon this.

michael-schwarz avatar Feb 09 '23 09:02 michael-schwarz

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

michael-schwarz avatar Mar 21 '23 10:03 michael-schwarz

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.

stilscher avatar Mar 21 '23 10:03 stilscher

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

sim642 avatar Mar 22 '23 09:03 sim642

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

michael-schwarz avatar May 31 '23 07:05 michael-schwarz

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

stilscher avatar May 31 '23 08:05 stilscher

:tada:

TimOrtel avatar May 31 '23 13:05 TimOrtel