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

Fix syntactic search #147

Merged
merged 4 commits into from
Sep 15, 2023
Merged

Fix syntactic search #147

merged 4 commits into from
Sep 15, 2023

Conversation

stilscher
Copy link
Member

@stilscher stilscher commented Jun 20, 2023

Fixes #146.

The syntactic search looks up the corresponding (possibly differently named) varinfo and location in the environment and checks whether it is within the function currently in focus. The check whether the location is within the function only considers the starting line number. Also, as the end of the function the starting line number of the "next" function is taken. There can however be several different files, so the line number is not a sufficiently precise representation of the location. Also the start line number of the next function might be in a different file and therefore does not correspond with the actual end line number of the function (apart from this I am not sure if any reasonable ordering of the functions in the list can even be assumed).

In the example from the issue, this leads to function main reaching from line 4-5 (5 is the start line of function pthread_once), and so no variable use of fail is found within.

Changes:

  • the location comparison is changed to also respect the file field of the location and use its endLine field directly whenever it is set.
  • an exception is thrown in the case that the query is not supported such that it can be distinguished from "No result found".

This also fixes the issue goblint/gobview#7 (comment). The expression of the semantic query is only evaluated on the results from the syntactic analysis, so if no variable uses are found in the syntactic search, the semantic search will also look broken. I could not find an example where the semantic analysis is still broken (apart from what is fixed in goblint/analyzer#1092 (comment)).

Related PRs: goblint/analyzer#1092 (comment), goblint/gobview#25 (comment)

@michael-schwarz
Copy link
Member

Goblint-CIL aims to support OCaml >= 4.05, so things such as List.find_map that are more recent cannot be used here.

@michael-schwarz
Copy link
Member

This also fixes goblint/gobview#7 (comment).

I am extremely skeptical that this indeed fixes that issue. We had identified that it was due to prune pruning the entire hashtable because of some hash issue. This seems unrelated to the issue you are fixing here.

@michael-schwarz
Copy link
Member

Ok, I am sure this is a stupid question, but here it comes: Why rely on something so brittle as line numbers here at all?

@stilscher
Copy link
Member Author

I am extremely skeptical that this indeed fixes that issue. We had identified that it was due to prune pruning the entire hashtable because of some hash issue. This seems unrelated to the issue you are fixing here.

I did some more investigation:
The test case from the semantic search issue actually works in GobView also without the proposed changes from this (and the related) PRs. My fixes are necessary only for other test cases (such as the one described in #146, also with semantic queries) that contain pseudo-return nodes or where the computation with the line numbers is unlucky. I am however not able to reconstruct the false behavior of GobView anymore (no matter which of the current versions), where no results are found for the semantic query. This is confusing, because I am convinced that I did observe this unexpected behavior earlier.

@stilscher
Copy link
Member Author

Ok, I am sure this is a stupid question, but here it comes: Why rely on something so brittle as line numbers here at all?

CIL renames duplicate variable names in its so-called alpha-conversion. These renamings are stored in a hash table that maps variable names from the c source code to (possibly several) tuples of varinfo (which possibly has a different name) and location (where the varinfo with the new name was created). The function find_uses_in_fun_var tries to find all variable occurrences in a specific function body and for this looks up all (across the complete project) possible renamings of the searched variable in the hashtable. Because those renamings are not restricted to occurrences in the relevant functions, they are filtered with this location comparison.

Here is an example:

1 void f(int i) {
2   int i = 0; // renamed to i___0
3   int i___1 = 0;
4   i++;
5   return;
5 }

int main() {
  int i = 0;
  int i = 0; // renamed to i___0;
  int i = 1; // renamed to i___1
  f(i);
  return 0;
}

Querying for variables of name i in the body of function named f should lead to finding occurrences in lines 2 and 4. Without the line number check, the additional renaming i -> i___1 in main is retrieved from the map and the additional occurrence in line 3 is wrongfully found.

I was wondering if one could avoid this check by searching for the varinfo, instead of just its name, but I think this would need some more extensive refactoring.

@michael-schwarz
Copy link
Member

Thanks for the clarification! I feel like I knew this at some point, but have forgotten it since then.

@stilscher stilscher marked this pull request as ready for review July 21, 2023 08:57
Copy link
Member

@michael-schwarz michael-schwarz left a comment

Choose a reason for hiding this comment

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

LGTM!

@michael-schwarz
Copy link
Member

@stilscher what is blocking this?

Co-authored-by: Michael Schwarz <michael.schwarz93@gmail.com>
@michael-schwarz michael-schwarz merged commit 0d7db1c into develop Sep 15, 2023
46 checks passed
@michael-schwarz michael-schwarz deleted the fix-syntactic-search branch September 15, 2023 18:50
@sim642 sim642 added this to the 2.0.3 milestone Nov 20, 2023
sim642 added a commit to sim642/opam-repository that referenced this pull request Nov 20, 2023
CHANGES:

* Add `asm inline` parsing (goblint/cil#151).
* Ignore top level qualifiers in `__builtin_types_compatible_p` (goblint/cil#157).
* Add attribute `goblint_cil_nested` to local variables in inner scopes (goblint/cil#155).
* Expose `Cil.typeSigAddAttrs`.
* Add option to suppress `long double` warnings (goblint/cil#136, goblint/cil#156).
* Fix syntactic search (goblint/cil#147).
nberth pushed a commit to nberth/opam-repository that referenced this pull request Jun 18, 2024
CHANGES:

* Add `asm inline` parsing (goblint/cil#151).
* Ignore top level qualifiers in `__builtin_types_compatible_p` (goblint/cil#157).
* Add attribute `goblint_cil_nested` to local variables in inner scopes (goblint/cil#155).
* Expose `Cil.typeSigAddAttrs`.
* Add option to suppress `long double` warnings (goblint/cil#136, goblint/cil#156).
* Fix syntactic search (goblint/cil#147).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Syntactic search does not find variable use
3 participants