Skip to content

[analyzer] Handle [[assume(cond)]] as __builtin_assume(cond) #100762

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

Closed
steakhal opened this issue Jul 26, 2024 · 8 comments · Fixed by #116462
Closed

[analyzer] Handle [[assume(cond)]] as __builtin_assume(cond) #100762

steakhal opened this issue Jul 26, 2024 · 8 comments · Fixed by #116462

Comments

@steakhal
Copy link
Contributor

We already handle __builtin_assume calls inside the BuiltinFunctionChecker.
We should also handle the attribute variant of this, e.g.:
https://compiler-explorer.com/z/oq6nY7eqE

void clang_analyzer_printState();
extern int arr[10];

void using_assume_attr(int x) {
    [[assume(x > 100)]]; // NullStmt with an attribute
    //clang_analyzer_printState();
    arr[x] = 404; // FIXME: we don't raise this!
}

void using_builtin(int x) {
    __builtin_assume(x > 100); // CallExpr
    //clang_analyzer_printState();
    arr[x] = 404; // caught!
}
@steakhal steakhal added good first issue https://github.com/llvm/llvm-project/contribute clang:static analyzer labels Jul 26, 2024
@llvmbot
Copy link
Member

llvmbot commented Jul 26, 2024

Hi!

This issue may be a good introductory issue for people new to working on LLVM. If you would like to work on this issue, your first steps are:

  1. Check that no other contributor has already been assigned to this issue. If you believe that no one is actually working on it despite an assignment, ping the person. After one week without a response, the assignee may be changed.
  2. In the comments of this issue, request for it to be assigned to you, or just create a pull request after following the steps below. Mention this issue in the description of the pull request.
  3. Fix the issue locally.
  4. Run the test suite locally. Remember that the subdirectories under test/ create fine-grained testing targets, so you can e.g. use make check-clang-ast to only run Clang's AST tests.
  5. Create a Git commit.
  6. Run git clang-format HEAD~1 to format your changes.
  7. Open a pull request to the upstream repository on GitHub. Detailed instructions can be found in GitHub's documentation. Mention this issue in the description of the pull request.

If you have any further questions about this issue, don't hesitate to ask via a comment in the thread below.

@llvmbot
Copy link
Member

llvmbot commented Jul 26, 2024

@llvm/issue-subscribers-clang-static-analyzer

Author: Balazs Benics (steakhal)

We already handle `__builtin_assume` calls inside the `BuiltinFunctionChecker`. We should also handle the attribute variant of this, e.g.: https://compiler-explorer.com/z/oq6nY7eqE ```c++ void clang_analyzer_printState(); extern int arr[10];

void using_assume_attr(int x) {
[[assume(x > 100)]]; // NullStmt with an attribute
//clang_analyzer_printState();
arr[x] = 404; // FIXME: we don't raise this!
}

void using_builtin(int x) {
__builtin_assume(x > 100); // CallExpr
//clang_analyzer_printState();
arr[x] = 404; // caught!
}

</details>

@llvmbot
Copy link
Member

llvmbot commented Jul 26, 2024

@llvm/issue-subscribers-good-first-issue

Author: Balazs Benics (steakhal)

We already handle `__builtin_assume` calls inside the `BuiltinFunctionChecker`. We should also handle the attribute variant of this, e.g.: https://compiler-explorer.com/z/oq6nY7eqE ```c++ void clang_analyzer_printState(); extern int arr[10];

void using_assume_attr(int x) {
[[assume(x > 100)]]; // NullStmt with an attribute
//clang_analyzer_printState();
arr[x] = 404; // FIXME: we don't raise this!
}

void using_builtin(int x) {
__builtin_assume(x > 100); // CallExpr
//clang_analyzer_printState();
arr[x] = 404; // caught!
}

</details>

@vortex73
Copy link
Contributor

I'm working on this issue.

@vinay-deshmukh
Copy link
Contributor

Hi,
May I be assigned to this issue? I'd like to try to solve it!

@shrey1605
Copy link

Hi,
I would like to work on this. Can I please be assigned to this issue? Thank you.

@steakhal
Copy link
Contributor Author

We usually don't assign issues. Contributors are expected to file a PR with their improvements when they have something.

@vinay-deshmukh
Copy link
Contributor

@shrey1605

Hi! Just to confirm, I'm fully committed to working on this issue and have started making progress on a solution. I'm looking forward to raising the PR soon.

I hit the issue that was seen in #101063, and I think I've moved around it by adding the AttributedStmt to the CFG, now I'm trying to get the right SVal to update the ProgramState similar to how it's done in the BI::__builtin_assume case

vinay-deshmukh added a commit to vinay-deshmukh/llvm-project that referenced this issue Nov 16, 2024
vinay-deshmukh added a commit to vinay-deshmukh/llvm-project that referenced this issue Nov 16, 2024
@steakhal steakhal removed the good first issue https://github.com/llvm/llvm-project/contribute label Dec 18, 2024
steakhal added a commit that referenced this issue Dec 19, 2024
Resolves #100762 

Gist of the change:
1. All the symbol analysis, constraint manager and expression parsing
logic was already present, but the previous code didn't "visit" the
expressions within `assume()` by parsing those expressions, all of the
code "just works" by evaluating the SVals, and hence leaning on the same
logic that makes the code with `__builtin_assume` work
2. "Ignore" an expression from adding in CFG if it has side-effects (
similar to CGStmt.cpp (todo add link))
3. Add additional test case for ternary operator handling and modify
CFG.cpp's VisitGuardedExpr code for `continue`-ing if the `ProgramPoint`
is a `StmtPoint`

---------

Co-authored-by: Balazs Benics <[email protected]>
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.

5 participants