RFC: C++ Support non type template parameter values by matt-gretton-dann · Pull Request #2153 · github/codeql

matt-gretton-dann · 2019-10-18T10:34:04Z

This is an initial PR adding support for values from non-type template parameter values. It is not yet ready for merging as it doesn't have stats or complete testing added.

Questions:

Is the Database Schema taking the right approach
What else needs to be added to the libraries to give proper support for this functionality?

matt-gretton-dann · 2019-10-18T10:40:10Z

Internal PR: https://git.semmle.com/Semmle/code/pull/34800

jbj

I think it would be helpful for discussing my comments if you add some direct tests of the new getTemplateArgumentValue predicate.

cpp/upgrades/98a075d5495d7be7ede26557708cf22cfa3964ef/upgrade.properties

jbj · 2019-10-18T11:10:40Z

cpp/ql/test/library-tests/ir/ir/PrintAST.expected

 #-----|     0: [Parameter] p#0
 #-----|         Type = [RValueReferenceType] S &&
-#    9| [MemberFunction] int Bad::S::MemberFunction(int)
+#    9| [FunctionTemplateInstantiation,MemberFunction] int Bad::S::MemberFunction<int>(int)


I'd expect to see <6> here instead of <int>. I suppose the function signature pretty-printer needs to learn about getTemplateArgumentValue to find the 6, but where is the int coming from?

I'm still working on getting this tidied-up

If it causes you trouble, feel free to throw it over the fence to the analysis team.

cpp/ql/src/semmle/code/cpp/Declaration.qll

cpp/ql/src/semmlecode.cpp.dbscheme

dave-bartolomeo · 2019-10-18T17:15:54Z

I think the overall approach seems reasonable, but I have a few high-level questions/comments:

Is support for template template parameters planned as a follow-up? I think it's lower priority than non-type template parameters, but we'll need it eventually.
I'd really like to see tests to cover some of the more exotic cases, like non-type template parameters whose type is a pointer, reference, or pointer-to-member. Another case to test would be where the type of the non-type template parameter is dependent on a previous type template parameter, e.g. template<typename T, T* p> class Foo;.
Given template<typename A, int B> class Foo; Foo<float, 5> bar;, does getTemplateArgument(0) return float, getTemplateArgument(1) return int, and getTemplateArgumentValue(1) return 5?

cpp/ql/src/semmle/code/cpp/exprs/Call.qll

cpp/ql/src/semmle/code/cpp/Declaration.qll

matt-gretton-dann · 2019-10-21T16:21:22Z

I think the overall approach seems reasonable, but I have a few high-level questions/comments:

Is support for template template parameters planned as a follow-up? I think it's lower priority than non-type template parameters, but we'll need it eventually.

It wasn't but can be :-)

I'd really like to see tests to cover some of the more exotic cases, like non-type template parameters whose type is a pointer, reference, or pointer-to-member. Another case to test would be where the type of the non-type template parameter is dependent on a previous type template parameter, e.g. template<typename T, T* p> class Foo;.

I will add these

Given template<typename A, int B> class Foo; Foo<float, 5> bar;, does getTemplateArgument(0) return float, getTemplateArgument(1) return int, and getTemplateArgumentValue(1) return 5?

You will get two Declarations one for the template declaration, and one for the instantiation.

For the declaration you will get: getTemplateArgument(0) returns A, getTemplateArgument(1) returns int and getTemplateArgumentValue(1) returns B.

For the instantiation you are correct with the results.

matt-gretton-dann · 2019-10-22T10:01:51Z

cpp/ql/test/library-tests/templates/nontype_instantiations/general/test.cpp

+
+static const int one1 = 1, one2 = 1;
+C<one1> c = C<one2>();
+C<one1 + one2> e;


The front-end presents the extractor with the integer constant '2' here with no further information - not even a link back to the source code so we can see how '2' was written. There is currently no way I can find to tell the extractor about one1 + one2.

matt-gretton-dann · 2019-10-22T10:13:21Z

cpp/ql/test/library-tests/templates/nontype_instantiations/general/test.expected

+| test.cpp:16:8:16:8 | E<T, X> | file://:0:0:0:0 | T * | file://:0:0:0:0 | X |
+| test.cpp:16:8:16:8 | E<T, X> | test.cpp:15:19:15:19 | T | file://:0:0:0:0 | X |
+| test.cpp:16:8:16:8 | E<int, (int *)nullptr> | file://:0:0:0:0 | int | file://:0:0:0:0 | 0 |
+| test.cpp:16:8:16:8 | E<int, (int *)nullptr> | file://:0:0:0:0 | int * | file://:0:0:0:0 | 0 |


I do not understand why getATemplateArgument() is returning two types here (int and int*) for the same parameter. The TRAP looks correct: just one class_template_argument() tuple for index 1 which has a type T*. Is this something to do with how the underlying and unresolved element QL lib code works>

cpp/ql/src/semmlecode.cpp.dbscheme

matt-gretton-dann · 2019-10-22T10:15:12Z

cpp/ql/test/library-tests/ir/ir/PrintAST.expected

 #-----|     0: [Parameter] p#0
 #-----|         Type = [RValueReferenceType] S &&
-#    9| [MemberFunction] int Bad::S::MemberFunction(int)
+#    9| [FunctionTemplateInstantiation,MemberFunction] int Bad::S::MemberFunction<int>(int)


I'm still working on getting this tidied-up

matt-gretton-dann · 2019-10-22T10:16:35Z

cpp/ql/test/library-tests/templates/nontype_instantiations/general/test.cpp

@@ -0,0 +1,19 @@
+// semmle-extractor-options: --edg --trap_container=folder --edg --trap-compression=none


@dbartol: This should cover some of the test cases you wanted to see.

matt-gretton-dann · 2019-10-29T14:42:07Z

I think I have now got the DB side of this in good shape - I still need to generate the stats file update, but apart from that the DB and tests look good.

@jbj: I have made changes upgrades to the help. I'm still not sure they are good enough. I think we could do with a conversation about what should be put there and what is missing.

igfoo · 2019-10-30T10:16:19Z

cpp/ql/src/semmle/code/cpp/Class.qll

+   */
+  override Expr getTemplateArgumentValue(int i) {
+    class_template_argument_value(underlyingElement(this), i, unresolveElement(result))
+  }


Would it be better to just generalise

Type getTemplateArgument(int i)

to

Element getTemplateArgument(int i)

(and similarly in the dbscheme)?

I think the QL-level API you proposed there is more intuitive than what's proposed in this PR, and it would also cut down on the number of similar-looking predicates that are added. Unfortunately it's a breaking change to change the return type because it would no longer be possible to write getTemplateArgument(0).getName(). It might be a breaking change we're willing to make since I don't expect too many external queries to use this API at all.

We also currently get the declared type of a non-type template parameter extracted and accessible via getTemplateArgument (the myIntAlias in template<myIntAlias i> ...). If we want to keep that, we'd also need an accessor predicate for it. I'd call it the kind of the template argument, but maybe there's a more appropriate C++-specific word for it.

My initial design of the interface had the goal to not break anything, and to keep the changes small. I agree its not necessarily intuitive, and therefore am happy to change it.

However, I think having a predicate which returns either a Type or Expr depending on the template parameter kind is not good either as this means that those who want to know the type of Expr need to use another predicate to get that type.

I think there should be two predicates:

One to get the type of a template argument (either explicitly specified by the user, or deduced by the compiler). This has a value for every template argument.

One to get the value of the template argument. Only has a value for non-type templates.

(This is actually the current interface - although it could have much better names).

Maybe a better approach would be something like the following (which I think is closer to how the C++ Standard views the world):

Consider the following:

template<typename T, T X> T foo() { T y = X; return y + y; } int x = foo<int, 10>();

Currently. the trap we produce for the instantiation of foo<int, 10> currently does not reference the type T anywhere - instead the return type and type of y are directly int. Similarly, X is replaced with 10.

Maybe instead we should expose T as a typedef and X as a variable declaration, and use those instead. In the example above this would mean the return type of the instantiation of foo() is T which is a typedef for int.

Then in the template declaration we have getTemplateParameter() which return the names T, X (in this example) and provide a link to the types in the instantiation. (This final bit isn't clearly thought out :-) ).

My quick scan of the C/C++ Front-End source suggests that it holds enough info in the IL for this to be workable.

This is definitely a breaking change, and much more complicated to implement, but would be the more natural view of the world. However, does what we're currently doing provide enough for end users?

I think the most natural interface would be, if inst is your foo<int, 10> instantiation,

inst.getTemplateArgumentKind(0) = type // currently not representable inst.getTemplateArgument(0) = int inst.getTemplate().getTemplateArgumentKind(0) = typename // currently not representable inst.getTemplate().getTemplateArgument(0) = T inst.getTemplateArgumentKind(1) = int inst.getTemplateArgument(1) = 10 inst.getTemplate().getTemplateArgumentKind(1) = T inst.getTemplate().getTemplateArgument(1) = X

I like your idea of using typedefs and variables. With C++11 template<...> using declarations, I think this will even generalise to template template parameters. But for non-type template parameters, it's unclear to me how we'd map the variable to its value. If an instantiated template has variable X for a non-type template parameter, should the Initializer of that variable then be the expression that holds its value (10, in your example)? What if the same template instantiation is found in multiple source locations? One location might have foo<int, 10>, and another might have foo<int, 5+5>.

The interface currently implemented in this PR is not great, but I don't think it's an interface that will be used much, so I'm fine with merging it as it is as long as there is extensive QLDoc about all the surprising behaviour we've discussed.

Yes I did - updated :-).

I could imagine writing something like:

template<class T, T::size_type initial_size> array

where I would want to know the underlying type of T::size_type (4 or 8 bytes?).

This is where treating the parameters as typedefs and variable decls would be more natural because initial_size would be a variable of type T::size_type.

Anyway where I think we've got to is the need for two predicates on Declaration:

Element getTemplateArgument(int i) which returns the user-supplied i-th template argument.

Type getTemplateArgumentKind(int i) which returns the type of the i`-th template argument if that is a non-type argument, and has no result otherwise.

Correct?

And that the above is a breaking change - but we do not expect it to cause many people issues (if any).

Even if it currently only returns Types, getTemplateArgumentKind(int) should have result type Element. If a predicate is only going to have results of type Type, then its name should contain Type rather than Kind.

I'm not particularly wedded to the name Kind, incidentally; I was more talking about functionality than naming. I don't have a better suggestion, though.

I've implemented the above change - although the return type of both getTemplateArgument and getTemplateArgumentKind is now Locatable as that is the common-parent of Type & Expr.

jbj

These breaking changes will require an entry in change-notes/1.23/analysis-cpp.md.

cpp/ql/src/semmle/code/cpp/Declaration.qll

We now support getting the name used for non-type template parameters

Note that Declaration::getTemplateArgumentType() and Declaration::getTemplateArgumentValue() need to be public so that they can be overriden in derived classes.

matt-gretton-dann · 2019-11-05T11:42:13Z

These breaking changes will require an entry in change-notes/1.23/analysis-cpp.md.

Done. Please review.

nickrolfe

Jonas said, "I'm happy enough with the QL that any follow-up changes won't have to block the PR."

This test output must have been wrong because I produced it with an extractor that didn't have github#2153 applied.

matt-gretton-dann added C++ depends on internal PR This PR should only be merged in sync with an internal Semmle PR WIP This is a work-in-progress, do not merge yet! labels Oct 18, 2019

matt-gretton-dann requested review from dave-bartolomeo, ian-semmle and nickrolfe October 18, 2019 10:34

matt-gretton-dann requested a review from a team as a code owner October 18, 2019 10:34

matt-gretton-dann assigned nickrolfe and ian-semmle Oct 18, 2019

jbj reviewed Oct 18, 2019

View reviewed changes

jbj reviewed Oct 21, 2019

View reviewed changes

cpp/ql/src/semmle/code/cpp/exprs/Call.qll Show resolved Hide resolved

cpp/ql/src/semmle/code/cpp/Declaration.qll Show resolved Hide resolved

matt-gretton-dann commented Oct 22, 2019

View reviewed changes

matt-gretton-dann force-pushed the cpp-447-support-non-type-template-parameters branch from 05a2bd4 to 3c026fc Compare October 29, 2019 14:38

igfoo reviewed Oct 30, 2019

View reviewed changes

matt-gretton-dann force-pushed the cpp-447-support-non-type-template-parameters branch from 45bd6be to c468b8f Compare November 1, 2019 14:36

jbj reviewed Nov 4, 2019

View reviewed changes

cpp/ql/src/semmle/code/cpp/Declaration.qll Show resolved Hide resolved

cpp/ql/src/semmle/code/cpp/Declaration.qll Outdated Show resolved Hide resolved

Matthew Gretton-Dann added 10 commits November 5, 2019 11:39

C++: add *_template_parameter_value() tuples

48c7d1d

C++: Basic lib support for template param values

df7d212

C++: Update expected test results

faf5ba4

C++: Add DB Upgrade script.

6334ad9

C++: Further use of TemplateArgumentValue

4ab8729

C++: Add some simple non-type template tests

4698326

C++: Update test results

57cd9b3

We now support getting the name used for non-type template parameters

C++: Further nontype template testcases.

ca898d4

C++: Correct spelling mistake

f1c3ce0

C++: Print print nontype template params

809d97d

Matthew Gretton-Dann added 6 commits November 5, 2019 11:39

C++: Improve Template Value docs.

afeaa62

C++: Update expected test output.

45ec852

C++: Change API for exposing template parameters.

6fe22a7

Note that Declaration::getTemplateArgumentType() and Declaration::getTemplateArgumentValue() need to be public so that they can be overriden in derived classes.

C++: Update schema stats

6b4506d

C++: Simplify getTemplateArgument*() impl.

afe6665

C++: Update 1.23 change notes

8eef953

matt-gretton-dann force-pushed the cpp-447-support-non-type-template-parameters branch from c468b8f to 8eef953 Compare November 5, 2019 11:41

C++: Tidy up formatting

20ae183

nickrolfe approved these changes Nov 6, 2019

View reviewed changes

nickrolfe merged commit 5b00b21 into github:master Nov 6, 2019

jbj added a commit to jbj/ql that referenced this pull request Nov 11, 2019

C++: Fix semantic merge conflict

eb55d96

This test output must have been wrong because I produced it with an extractor that didn't have github#2153 applied.

kamarcum unassigned nickrolfe and ian-semmle Apr 28, 2020

		@@ -0,0 +1,19 @@
		// semmle-extractor-options: --edg --trap_container=folder --edg --trap-compression=none

Conversation

matt-gretton-dann commented Oct 18, 2019

Uh oh!

matt-gretton-dann commented Oct 18, 2019

Uh oh!

jbj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dave-bartolomeo commented Oct 18, 2019

Uh oh!

Uh oh!

Uh oh!

matt-gretton-dann commented Oct 21, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matt-gretton-dann commented Oct 29, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igfoo Oct 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

matt-gretton-dann commented Nov 5, 2019

Uh oh!

nickrolfe left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

igfoo Oct 31, 2019 •

edited

Loading