Conversation
8f8225a to
6edcabd
Compare
ccad487 to
f8d0434
Compare
In the upcoming commits we will add a new extensional predicate which allows us to model that a function forwards it arguments to the constructor of a given type. This initial commit adds the test YAML models for this new extensional predicate.
signature to implement the forwardsModel. So instead of recursing on the number of elements in the signature we will recurse on the number of elements in the type (or name) columns. For well-formed models this will be equivalent.
f8d0434 to
ada9784
Compare
…nd implement forwarding.
ada9784 to
232b29e
Compare
hvitved
left a comment
There was a problem hiding this comment.
Overall this approach LGTM; initially I hadn't thought of defining the forwarder models in MaD as well, but if you prefer that it's fine by me.
| getForwardedConstructorType(forwarder, type, name, constructorType).getUnspecifiedType() | ||
| or | ||
| // Or the row specifies forwarding to a specific type. | ||
| classHasQualifiedName(constructor.getDeclaringType(), namespace, constructorType) |
There was a problem hiding this comment.
AFAICT, there is currently no test case for this?
| pack: codeql/cpp-all | ||
| extensible: forwardsModel | ||
| data: # namespace, type, subtypes, name, signature, ext, start, constructor, provenance | ||
| - ["", "Forwarder<T>", True, "forward<Args>", "(Args &&)", "", "0", "T", "manual"] |
There was a problem hiding this comment.
How come these models mention type parameters while the summary models don't?
There was a problem hiding this comment.
The type parameters are optional in MaD for C++. In the summary case we don't need them in this case (since we don't specify a signature). In this case we need to refer to the type parameter in the class name since we need to refer to the type being constructed (i.e., T), and we need the type parameter for the function name since we need to specify the signature.
| result = forwarder.getDeclaringType().getTemplateArgument(index) | ||
| ) | ||
| or | ||
| exists(string nameArguments, int index | |
There was a problem hiding this comment.
I also think this branch is not covered by tests?
| ) | ||
| } | ||
|
|
||
| /** Holds if `forwarder` forwards its arguments starting at `start` to `constructor`. */ |
There was a problem hiding this comment.
Holds if `forwarder` may forward its arguments starting at `start` to `constructor`. The
actual constructor being forwarded to depends on the types of arguments from `start`
at calls to `forwarder`.
Thanks for the review, Tom! I actually opened a non-draft version of the PR here. Sorry about the confusion! The code is essentially what's in here (and I've addressed your review comments by pushing changes to that PR). The difference between this PR and the non-draft one is that the non-draft one only requires specifying the forwards model, and then automatically generates the required summary models. |
|
I'll close this PR to avoid further confusion |
This PR implements the necessary library changes to support MaD summaries for functions such as vector::emplace_back or make_unique. These functions receive a list of arguments and then forwards them to a constructor call.
@hvitved had a super cool implementation idea. Given:
we model a call such as
v.emplace_back(42)as:v.emplace_back(42, &Foo)and give
emplace_backtwo summaries:Argument[0] -> Argument[1].Parameter[0]Argument[1].Parameter[this] -> Argument[this].ElementThe first summary states that
42goes into the 0'th parameter of theFooconstructor, and the second summary states that thethisparameter of the constructed object goes into thethisargument ofvwith anElementcontent.(A few lines I told in the above paragraph:
forwardin MaD. I'm happy to change this name to something else if anyone has any strong opinions about this.)In order to know which constructor to forward to we need to know what type is being constructed. For example:
To know which type is constructed we add a new extensible called
forwardsModelwith rows very much like what we have for MaD summaries. For example, I've added this as row as a test:["", "Container<T>", True, "emplace<Args>", "(Args &&)", "", "0", "T", "manual"]this says that a call to
Container<T>::emplace(args0, ..., argsN)forwards its arguments to a call toT(args0, ..., argsN). The0specifies an offset so we can support cases likev.emplace(v.begin(), 42)where we need to ignore the first argument.This PR doesn't actually add any non-test MaD summaries. I'll delay that to a future PR.