Skip to content

Conversation

@sim642
Copy link
Member

@sim642 sim642 commented May 3, 2022

Closes #689.
This is only a problem on interactive due to #533 only being there.

The ideal solution would be to have UpdateCil also update expression locations (not just one location for each statement), but that requires a lot more work to distinguish the statements/instructions and store multiple locations for some.

Also, since there's no way to identify specific CFG edges (unlike nodes, which have IDs from either statements or the function), there's no easy way to just update the edge locations incrementally either. PR #689 and this make edge locations largely redundant (at least for messages, maybe something uses them non-incrementally directly).

Therefore this PR just contains a refinement of @vesalvojdani's original fix: simply use expression locations for everything.
This makes it impossible to get and warn about an entire if/while/switch statement, rather than just its expression, but I don't remember any case where we want to do that. If that turns up, a more involved fix is necessary to distinguish the two.

@sim642 sim642 added this to the v2.0.0 milestone May 3, 2022
@sim642 sim642 requested a review from vesalvojdani May 3, 2022 09:20
@sim642 sim642 mentioned this pull request May 3, 2022
5 tasks
Copy link
Member

@vesalvojdani vesalvojdani left a comment

Choose a reason for hiding this comment

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

This works on the examples I tried. I'm not sure about the TODOs. We don't have tests for assertion generation, do we?

@sim642
Copy link
Member Author

sim642 commented May 4, 2022

I'm not sure about the TODOs. We don't have tests for assertion generation, do we?

No. I'll leave it for anyone working on assert generation to figure out in the future, what is intended. Because that's more of an AST transformation and deals with block and If, maybe it intends to use the entire statement's location? Or maybe it doesn't matter because it looks like it puts locations into the AST it generates, where they're irrelevant for printout anyway.
Either way, it's just good to have it noted down, because the semgrep rule for Cil.get_stmtLoc didn't catch them since they're used under open Cil.

@sim642 sim642 merged commit 79ac8c0 into interactive May 4, 2022
@sim642 sim642 deleted the issue-689 branch May 4, 2022 09:49
@sim642 sim642 linked an issue May 4, 2022 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Locations for accesses in guards cover entire statement

3 participants