close
The Wayback Machine - https://web.archive.org/web/20260308075422/https://github.com/github/codeql/pull/11712
Skip to content

Java: Apply deadcode guard to data flow nodes.#11712

Merged
aschackmull merged 4 commits intogithub:mainfrom
aschackmull:java/constant-guards
Feb 7, 2023
Merged

Java: Apply deadcode guard to data flow nodes.#11712
aschackmull merged 4 commits intogithub:mainfrom
aschackmull:java/constant-guards

Conversation

@aschackmull
Copy link
Contributor

Refactors the compile-time constant computation, such that we can reuse it for more general constant calculations.
Use this to define a trivial dead-code guard and apply it to data flow nodes to filter all flow through dead code.

It was necessary to reshuffle the dependencies between SSA, virtual dispatch, and data flow nodes a bit in order to be able to refer to SSA from data flow nodes. This means that field SSA uses a slightly lower precision virtual dispatch in the determination of uncertain updates of field-SSA variables. This is in line with what's also done in C#.

@aschackmull aschackmull requested a review from a team as a code owner December 15, 2022 14:28
@github-actions github-actions bot added the Java label Dec 15, 2022
Copy link
Contributor

@hvitved hvitved left a comment

Choose a reason for hiding this comment

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

LGTM, one question.

@aschackmull
Copy link
Contributor Author

The qltests caught a small corner-case: In ArrayIndexOutOfBounds.ql if we would previously catch a case based on a relative bound to an array length field access that happened to be a constant due to the array being defined in a final field and with a fixed length, then the additional constant propagation means that such a length access plus another constant is now found up front to be a constant, and thus the relative bound becomes superfluous, but the query didn't properly account for that case. This is now fixed. I've started an additional dca just for that query.

@aschackmull
Copy link
Contributor Author

Looks like this causes ArrayIndexOutOfBounds.ql to catch a few more results, but it apparently also loses a few - that's worth an additional look before merging.

@zbazztian
Copy link
Contributor

zbazztian commented Dec 21, 2022

@starcke Thanks for doing this! Are you planning to add some flow test cases using this new feature? If not, I am happy to contribute them.

@zbazztian
Copy link
Contributor

zbazztian commented Dec 21, 2022

^
@starcke Apologies, I meant the other Anders, @aschackmull !

@aschackmull
Copy link
Contributor Author

Looks like this causes ArrayIndexOutOfBounds.ql to catch a few more results, but it apparently also loses a few - that's worth an additional look before merging.

One result lost to the very slight change in SSA precision for fields, and two results lost that turned out to actually be FPs.

@aschackmull aschackmull merged commit 3c58089 into github:main Feb 7, 2023
@aschackmull aschackmull deleted the java/constant-guards branch February 7, 2023 08:14
@aschackmull
Copy link
Contributor Author

Are you planning to add some flow test cases using this new feature? If not, I am happy to contribute them.

Feel free to add some as a follow-up. I wanted to merge this sooner rather than later, since it had been open for so long.

@zbazztian
Copy link
Contributor

Makes sense. Thanks for implementing this, I will definitely give this a test this week.

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.

3 participants