C++: Fix `getQualifiedName` performance issues #1303

jbj · 2019-05-03T08:44:45Z

See https://jira.semmle.com/browse/CPP-280. This PR is best reviewed one commit at a time.

This PR is supposed to be completely behaviour-preserving. I'll run the full test suite and CPP-Differences to confirm that.

This imports `QualifiedName.qll` from 2f74a456290b9e0850b7308582e07f5d68de3a36 and makes minimal changes so it compiles. Original author: Ian Lynagh <ian@semmle.com>

This removes all uses of `Declaration.getQualifiedName` that I think can be removed without changing any behaviour. The following uses in the LGTM default suite remain: * `cpp/ql/src/Security/CWE/CWE-121/UnterminatedVarargsCall.ql` (in `select`). * `cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowDispatch.qll` (needs template args). * `cpp/ql/src/semmle/code/cpp/security/FunctionWithWrappers.qll` (used for alert messages).

This predicate handles templates differently from the other overloads with the same name, so it's likely to cause confusion.

The predicate is still deprecated, but we can't mark it as such until the queries in our internal repo have migrated away from it.

geoffw0

Do you have any performance figures for this change?

geoffw0 · 2019-05-03T11:19:36Z

cpp/ql/src/semmle/code/cpp/internal/QualifiedName.qll

+class Namespace extends @namespace {
+  string toString() { result = "QualifiedName Namespace" }
+
+  string getName() { namespaces(this, result) }


Should this be namespaces(underlyingElement(this),result) as it is in Namespace.qll?

No, because underlyingElement is defined higher up in the stack of abstractions.

geoffw0 · 2019-05-03T11:23:10Z

cpp/ql/src/semmle/code/cpp/internal/QualifiedName.qll

@@ -0,0 +1,343 @@
+class Namespace extends @namespace {


This has the same name as a class in Namespace.qll, as do most of the other classes in this file. Are we relying on (QL) namespaces to make them distinct? Is it worth explaining what's going on in a comment?

Yes, I’ll add a comment at the top of the file to explain what’s going on.

The reason for writing this file directly on the dbscheme is that qualified names are needed to improve the ResolveClass mechanism, and ResolveClass sits below the AST classes. That’s what Ian was doing on the branch I took this from.

ian-semmle · 2019-05-03T14:44:34Z

cpp/ql/src/semmle/code/cpp/models/implementations/Printf.qll

-    getQualifiedName() != "g_strdup_printf" and
-    getQualifiedName() != "__builtin___sprintf_chk" and
+    getName() != "g_strdup_printf" and
+    getName() != "__builtin___sprintf_chk" and


Shouldn't this be

not hasGlobalName("g_strdup_printf") and not hasGlobalName("__builtin___sprintf_chk") and

?

I think that would give the same result, but I wanted to avoid the negation to minimise the performance risk. Given that we've already restricted the name in the charpred to come from a small set, do you agree it's equivalent?

Ah, yes, I hadn't looked outside this predicate.

Since the types in `QualifiedName.qll` are raw db types, callers need to use `underlyingElement` and `unresolveElement` as appropriate. This has no effect right now but will be needed when we switch the AST type hierarchy to `newtype`s.

jbj · 2019-05-06T09:59:55Z

I ran https://jenkins.internal.semmle.com/job/Query-Changes/job/CPP-Differences/555 to confirm that this PR doesn't change behaviour, but there were three changes, all in jdk8u.

One change is an alert from StackAddressEscapes.ql that disappears in a function named unpacker::abort. I can't find the original alert locally. I think it's just wobble because I can find previous jobs (like Query-Changes/CPP # 658) where this particular query wobbles on this particular snapshot.
Two alerts from IntegerOverflowTainted.ql disappeared. That's because the security-pack taint tracking library uses hasGlobalName to identify sources, and hasGlobalName changes in this PR so that the function os::recvfrom is no longer considered to have the global name recvfrom (os is a class, not a namespace). I think that's a change we want to keep.

jbj · 2019-05-06T11:39:41Z

I think this PR is now ready for review. I'll post performance results when I have them.

jbj · 2019-05-08T20:34:22Z

I've measured performance, and it looks good. Compare the 1.20 results to the new results. It's not an apples-to-apples comparison because many unrelated things have changed between the two revisions, and the second evaluation ran with more RAM, but I think it's clear what happened with the qualified-name predicates:

getQualifiedName went from 59 invocations taking a total of 2163 seconds to just 9 invocations taking a total of 252 seconds. I'd like to get it even further down, but this is what I could easily do without changing semantics.
declarationHasQualifiedName (which implements the new multi-argument hasQualifiedName predicates and hasGlobalName) has 2 invocations (one of them is for a join_rhs helper predicate) taking a total of 31 seconds. That number is only for the predicate itself. Its whole cached stage takes 94 seconds, which can be seen in the Per-stage tab of the spreadsheet.

rdmarsh2

A couple of minor nits, LGTM otherwise

cpp/ql/src/semmle/code/cpp/security/CommandExecution.qll

rdmarsh2 · 2019-05-13T16:32:11Z

cpp/ql/src/semmle/code/cpp/internal/QualifiedName.qll

+  override string getName() { enumconstants(this, _, _, _, result, _) }
+
+  UserType getDeclaringEnum() { enumconstants(this, result, _, _, _, _) }
+  // Unlike the usual `EnumConstant`, this one doesn't have a `getDeclaringType()`.


geoffw0

Only reviewed a few bits. I think this change is a good idea.

geoffw0 · 2019-05-14T08:02:55Z

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

+   * For performance, prefer the multi-argument `hasQualifiedName` or
+   * `hasGlobalName` predicates since they don't construct so many intermediate
+   * strings. For debugging, the `semmle.code.cpp.Print` module produces more
+   * detailed output but are also more expensive to compute.


geoffw0 · 2019-05-14T08:04:08Z

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

+   * component of `namespaceQualifier`, no declaring type, and a base name of
+   * `baseName`.
+   *
+   * See the 3-argument `hasQualifiedName` for more examples.


Just for examples as there are no examples here. Though I'd prefer you move the "std", "vector" example to this predicate.

jbj · 2019-05-15T12:56:22Z

@rdmarsh2 and @geoffw0, I've pushed a commit to address your comments.

jbj added 6 commits May 3, 2019 10:37

C++: Take QualifiedName.qll from Ian's branch

b97ff1a

This imports `QualifiedName.qll` from 2f74a456290b9e0850b7308582e07f5d68de3a36 and makes minimal changes so it compiles. Original author: Ian Lynagh <ian@semmle.com>

C++: Autoformat QualifiedName.qll

b51ce87

C++: Rework how qualified names are computed

0a2e288

C++: Remove all uses of hasQualifiedName/1

5e78990

C++: Deprecate hasQualifiedName/1

6d954fe

This predicate handles templates differently from the other overloads with the same name, so it's likely to cause confusion.

jbj added the C++ label May 3, 2019

C++: Remove deprecated from hasQualifiedName/1

b98daae

The predicate is still deprecated, but we can't mark it as such until the queries in our internal repo have migrated away from it.

geoffw0 reviewed May 3, 2019

View reviewed changes

ian-semmle reviewed May 3, 2019

View reviewed changes

jbj added 4 commits May 6, 2019 10:14

C++: Change note for hasGlobalName

98657eb

C++: Add tests for qualified names

662d55f

C++: QLDoc for QualifiedName.qll

b52015a

jbj marked this pull request as ready for review May 6, 2019 11:39

jbj requested a review from a team as a code owner May 6, 2019 11:39

jbj mentioned this pull request May 13, 2019

[CPP-370] Non-constant format arguments to printf and friends #1251

Merged

rdmarsh2 suggested changes May 13, 2019

View reviewed changes

geoffw0 reviewed May 14, 2019

View reviewed changes

C++: Address review comments about the comments

d820fc9

rdmarsh2 approved these changes May 15, 2019

View reviewed changes

rdmarsh2 merged commit 1479586 into github:master May 15, 2019

jbj mentioned this pull request May 29, 2019

C++: Use Definition.qll's getName #1376

Merged

C++: Fix getQualifiedName performance issues #1303

C++: Fix getQualifiedName performance issues #1303

Uh oh!

Conversation

jbj commented May 3, 2019

Uh oh!

geoffw0 left a comment

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!

Choose a reason for hiding this comment

Uh oh!

jbj commented May 6, 2019

Uh oh!

jbj commented May 6, 2019

Uh oh!

jbj commented May 8, 2019

Uh oh!

rdmarsh2 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geoffw0 left a comment

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 commented May 15, 2019

Uh oh!

Uh oh!

C++: Fix `getQualifiedName` performance issues #1303

C++: Fix `getQualifiedName` performance issues #1303