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

Added undefined throwing function. #17397

Closed

Conversation

bdrodes
Copy link
Contributor

@bdrodes bdrodes commented Sep 6, 2024

Functions with no body are assumed to potentially throw.

This PR is blocked by https://github.com/github/codeql-c-team/issues/2423, we first must address the unexplained loop IR inconsistency as this will increase those inconsistencies.

The prior PR 128053e adds an apparent inconsistency with respect to this PR by specifying explicit non-throwing functions. For SEH some of what have been marked as non-throwing actually do throw (e.g., memcpy may throw with respect to SEH). We will work on a separate PR to differentiate for SEH, but the prior PR doesn't involve IR so the inconsistency, to my understanding, wouldn't be noticed as this PR affects IR flow specifically.

@github-actions github-actions bot added the C++ label Sep 6, 2024
Comment on lines +11 to +14
/**
* Calls to functions in a try statement with no body
* are assumed to potentially (not unconditionally) throw anything.
*/

Check warning

Code scanning / CodeQL

Class QLDoc style. Warning

The QLDoc for a class should start with 'A', 'An', or 'The'.
@MathiasVP
Copy link
Contributor

FWIW, the relevant inconsistencies have now all been fixed 🎉

@bdrodes bdrodes marked this pull request as ready for review September 26, 2024 15:10
@bdrodes bdrodes requested a review from a team as a code owner September 26, 2024 15:10
@jketema
Copy link
Contributor

jketema commented Sep 26, 2024

@bdrodes This has a merge conflict, could you resolve that? Thanks!

@bdrodes bdrodes force-pushed the undefined_throwing_function_upstream2 branch from a3890e4 to de8f91d Compare September 26, 2024 15:13
@bdrodes
Copy link
Contributor Author

bdrodes commented Sep 26, 2024

@bdrodes This has a merge conflict, could you resolve that? Thanks!

@jketema Fixed.

@jketema
Copy link
Contributor

jketema commented Sep 26, 2024

@bdrodes I'm still seeing some failing tests:

  FAILED: cpp/ql/test/library-tests/controlflow/guards-ir/tests.ql
  FAILED: cpp/ql/test/library-tests/controlflow/guards/GuardsControl.ql
  FAILED: cpp/ql/test/library-tests/controlflow/guards/GuardsEnsure.ql

@bdrodes
Copy link
Contributor Author

bdrodes commented Sep 26, 2024

@bdrodes I'm still seeing some failing tests:

  FAILED: cpp/ql/test/library-tests/controlflow/guards-ir/tests.ql
  FAILED: cpp/ql/test/library-tests/controlflow/guards/GuardsControl.ql
  FAILED: cpp/ql/test/library-tests/controlflow/guards/GuardsEnsure.ql

@jketema , fixed

@jketema
Copy link
Contributor

jketema commented Sep 27, 2024

I've two concerns with the approach taken in this PR:

  • The set functions represented by UndefinedFunction overlaps with the set represented by NonThrowingFunction, as as a declaration for which we do not have a definition might have a noexcept() specification. Of course this specification does not apply to structured exceptions, but by taking a general approach file this, this also affects codebases that do not target Windows.
  • The PR creates a very asymmetric situation in that we will now have exception edges in the IR for functions without bodies, but not for functions with bodies for which we know that they throw.

I wonder if we can instead add the exception edges everywhere. I'm not sure what the performance implications of that will be though.

@jketema
Copy link
Contributor

jketema commented Sep 27, 2024

Closing this, as discussed F2F.

@jketema jketema closed this Sep 27, 2024
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