Skip to content

Conversation

MathiasVP
Copy link
Contributor

@MathiasVP MathiasVP commented Nov 20, 2020

Fixes https://github.com/github/codeql-c-analysis-team/issues/186.

In #3571 we made it possible to treat this as an argument, but there are still a couple of places where we need to distinguish between qualifiers and parameters (and between this and arguments). These places are:

  • When accessing the arguments supplied to a CallInstruction (which has a getPositionalArgument and getThisArgument predicates)
  • When working with instructions that initialize parameters (where initializers of positional parameters have a result for the getParameter predicate, but instructions initializing this need to refer to the IRVariable). The same problem also exists for ReturnIndirectionInstruction.

This PR adds a new predicate for each of these scenarios:

  • CallInstruction now has a getPositionalOrThisArgumentOperand (and getPositionalOrThisArgument) predicates that, given an index, gives back the argument operand (and instruction) with the given index, or when the index is -1, gives back the operand used as the qualifier.
  • InitializeParameterInstruction and InitializeIndirectionInstruction now have an isParameterOrQualifierIndex predicate that holds if the instruction initializes a positional parameter with a given index, or initialize this with the index -1.
    Similarly, ReturnIndirectionInstruction has a new predicate isParameterOrThisIndirection with similar semantics.

@MathiasVP MathiasVP added the C++ label Nov 20, 2020
@MathiasVP MathiasVP requested review from a team as code owners November 20, 2020 11:29
@github-actions github-actions bot added the C# label Nov 20, 2020
@MathiasVP MathiasVP changed the title C++: Qualifiers as parameters C++: Abstractions for treating qualifiers as parameters in IR Nov 20, 2020
@MathiasVP MathiasVP requested a review from rdmarsh2 November 20, 2020 11:37
@MathiasVP
Copy link
Contributor Author

@rdmarsh2
Copy link
Contributor

rdmarsh2 commented Nov 20, 2020

I think we want to guide users towards using these predicates by default, which usually means giving them names that are simpler than their alternatives. Could getPositionalOrThisArgumentOperand just be getArgumentOperand? Similarly, would isParameterOrQualifierIndex be better as hasIndex?

…uide users to use these predicates by default.
… own implementation of getArgument already (which didn't handle qualifiers). The predicate wasn't used anywhere, so I simply removed it, as a better predicate is now available on the base class of DataFlowCall.
@MathiasVP
Copy link
Contributor Author

I think we want to guide users towards using these predicates by default, which usually means giving them names that are simpler than their alternatives. Could getPositionalOrThisArgumentOperand just be getArgumentOperand? Similarly, would isParameterOrQualifierIndex be better as hasIndex?

I think those are some excellent names! I've changed the names to match your suggestions now. I had to get rid of an unused predicate on DataFlowCall in DataFlowPrivate since the name was clashing with your suggestion (and I think removing that predicate is okay since I don't think we expect users to use DataFlowCall?)

@MathiasVP
Copy link
Contributor Author

CPP-difference started: https://jenkins.internal.semmle.com/job/Changes/job/CPP-Differences/1563/

CPP-difference looks good (for some reason there's a 10% reduction in build/extraction/import time on openjdk?)

jbj
jbj previously approved these changes Nov 24, 2020
Copy link
Contributor

@jbj jbj left a comment

Choose a reason for hiding this comment

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

Looks great! I'm happy with the names too.

Comment on lines +804 to +808
final predicate hasIndex(int index) {
index >= 0 and index = this.getParameter().getIndex()
or
index = -1 and this.isThisIndirection()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The implementation of this predicate looks a lot like getArgumentOperand, but only the latter has pragma[noinline] on it. Is there a good reason to put pragma[noinline] on one but not the other?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, no. I suspect the pragma[noinline] is an artifact of me copy/pasting from getPositionalArgumentOperand. I'll just double-check that I can safely remove the new pragma[noinline]s.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm... looking at this comment you made, I'd actually feel more comfortable adding a pragma[noinline] to the hasIndex predicates. It doesn't seem to be necessary right now, but if it avoids a future bad join-order we might as well put it there now. Would that be reasonable?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes.

@jbj jbj merged commit 260a8d4 into github:main Nov 24, 2020
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.

3 participants