Spurious matches of function definitions

[Derek-Jones]

Running the attached script on the alibaba/tsar repo generates very many spurious matches (around 160,000) for some files, e.g., devel/mod_test.c

This problem occurs when running either a variant analysis or locally (running v1.17.7).

Subset of output: funcdef-subset.csv

CodeQL script: func-def.txt

[hvitved]

Hi

Does it help if you add not func.isConstructedFrom(_)?

[Derek-Jones]

@hvitved
The code is C, not C++, so no templates.

Tried it anyway. Did not change the behavior.

[hvitved]

What happens if you use FunctionDeclarationEntry instead of Function?

[Derek-Jones]

@hvitved

This script appears to work fine on 116,169 other files.

Using FunctionDeclarationEntry instead of Function produces the error

Failed to run query: ERROR: Function::Function is incompatible with Function::FunctionDeclarationEntry (/home/Measure-source/ql/cpp/func-def.ql:22,43-44) ERROR: getMetrics() cannot be resolved for type Function::FunctionDeclarationEntry (/home/Measure-source/ql/cpp/func-def.ql:18,8-18)

[hvitved]

Ok, I think the issue is that multiple functions are collapsed into a single entity in the CodeQL DB. Evaluating

private predicate multiDecls(Function f, int decls) {
  decls = strictcount(FunctionDeclarationEntry decl | decl.getFunction() = f) and
  decls > 1
}

reveals that mod_register has 21 declarations; I don't have access to the logs that created the DB, so I'm not sure exactly why it happens though (perhaps @jketema can help).

As a workaround, the the following adjusted version of your code:

from FunctionDeclarationEntry decl, Function func
where
  func = decl.getFunction() and
  not func.isCompilerGenerated() and
  not decl.getFile() instanceof HeaderFile
select func.getName() as fname, func.getType() as ftype,
  func.getMetrics().getNumberOfParameters() as nparams,
  func.getMetrics().getNumberOfLines() as nlines,
  func.getMetrics().getNumberOfLinesOfCode() as nLOC,
  func.getMetrics().getNumberOfLinesOfComments() as ncomments,
  count(Stmt s | s.getEnclosingFunction() = func) as nstmts, // Number of statements per function
  decl.getBlock().getLocation().getStartLine() as curlstart,
  decl.getBlock().getLocation().getEndLine() as curlend,
  count(int dummy | dummy = 1 and decl.hasCLinkage() | dummy) as clinkage,
  count(int dummy | dummy = 1 and decl.hasExceptionSpecification() | dummy) as exceptionspec,
  count(int dummy | dummy = 1 and func.isConsteval() | dummy) as consteval,
  count(int dummy | dummy = 1 and func.isConstexpr() | dummy) as constexpr,
  count(int dummy | dummy = 1 and func.isDeclaredConstexpr() | dummy) as declconstexpr,
  count(int dummy | dummy = 1 and func.isDeclaredVirtual() | dummy) as declvirtual,
  count(int dummy | dummy = 1 and func.isDefaulted() | dummy) as defaulted,
  count(int dummy | dummy = 1 and func.isDeleted() | dummy) as deleted,
  count(int dummy | dummy = 1 and func.isExplicit() | dummy) as explicit,
  count(int dummy | dummy = 1 and func.isFinal() | dummy) as final,
  count(int dummy | dummy = 1 and func.isInline() | dummy) as inline,
  count(int dummy | dummy = 1 and func.isMember() | dummy) as member,
  count(int dummy | dummy = 1 and func.isNaked() | dummy) as naked,
  count(int dummy | dummy = 1 and decl.isNoExcept() | dummy) as noexcept,
  count(int dummy | dummy = 1 and decl.isNoThrow() | dummy) as nothrow,
  count(int dummy | dummy = 1 and func.isOverride() | dummy) as override,
  count(int dummy | dummy = 1 and func.isPrototyped() | dummy) as prototyped,
  count(int dummy | dummy = 1 and func.isSideEffectFree() | dummy) as seffectfqee,
  count(int dummy | dummy = 1 and decl.isSpecialization() | dummy) as specialization,
  count(int dummy | dummy = 1 and func.isStatic() | dummy) as static,
  count(int dummy | dummy = 1 and func.isVarargs() | dummy) as varargs,
  count(int dummy | dummy = 1 and func.isVirtual() | dummy) as virtual,
  decl.getLocation().getStartLine() as startline, decl.getLocation().getEndLine() as endline,
  decl.getFile().getRelativePath() as filepath

[jketema]

I don't have access to the logs that created the DB, so I'm not sure exactly why it happens though

Indeed difficult to tell without the logs produced during database creation.

It does look like mod_register is defined in 21 files (matching the number from above). If those files are only compiled into object files, and not further linked into executables or dynamic libraries, then this is exactly the behavior I would expect.

Looking at the output of:

import cpp

predicate multiDecls(Function f, int decls) {
  decls = strictcount(FunctionDeclarationEntry decl | decl.getFunction() = f) and
  decls > 1
}

from Function f
where multiDecls(f, _)
select f, f.getALinkTarget()

I see that all found functions have a link target that is reported as [empty string], which generally indicates that we're in the situation I described above, i.e., the object files produced for those source files are never linked into an executable or dynamic library.

[Derek-Jones]

@hvitved
Thanks, your script did not experience the problem.

@hvitved , @jketema

Should this issue be left open as a bug?

[jketema]

Should this issue be left open as a bug?

It's expected behavior, so please close if your questions have been answered.

[Derek-Jones]

@jketema
It's certainly not expected behavior by me.

You and @hvitved had to dig around to find out why it was happening. A bug produces expected behavior when you understand the bug.

This is a bug in the sense of "This is not what we want to be happening"

I will close it, if you think this is not an issue worth fixing.

[jketema]

This is a bug in the sense of "This is not what we want to be happening"

No this is expected behavior. If I run:

from string s
where if forall(Compilation c | c.buildModeNone()) then s = "BMN" else s = "Huh?"
select s

then I see that the database was build with build-mode: none. I should probably have checked this first, but it seemed unlikely this would be the case given the date of the initial CodeQL run on the repo.

With build-mode: none there is no concept of executable or dynamic library, and we conceptually assume that all code lives in a single binary. That this gives weird effects as you describe, is something we have explicitly accepted. If you do not want these effects, then you need to rebuild the database using build-mode: autobuild or build-mode: manual.

[Derek-Jones]

@jketema
It sounds like running scripts on build-mode: none is not a good idea.

Is there much of a runtime penalty adding not exists(Compilation c | c.buildModeNone()) to the where clause (Grok's suggestion)?

Are similar problems likely to occur with other scripts?

ChatGPT suggested an import semmle.code.cpp.build.BuildMode, which does not exist!

[jketema]

It sounds like running scripts on build-mode: none is not a good idea.

That very much depends on what you want to achieve. For our queries we generally found out it works quite well.

Is there much of a runtime penalty adding not exists(Compilation c | c.buildModeNone()) to the where clause (Grok's suggestion)?

That's not going to work. Either either all files/compilations in the database have this property (so you will get no results), or the database was created with build-mode: autobuild or build-mode: manual and you won't have this problem.

[Derek-Jones]

@jketema

It sounds like running scripts on build-mode: none is not a good idea.

That very much depends on what you want to achieve. For our queries we generally found out it works quite well.

I would like to achieve accurate results. It was easy to spot that a file containing 160,000 function definitions was suspicious. If there had been, say, 10,000 definitions, I would probably have assumed that the code was machine generated.

Is there much of a runtime penalty adding not exists(Compilation c | c.buildModeNone()) to the where clause (Grok's suggestion)?

That's not going to work. Either either all files/compilations in the database have this property (so you will get no results), or the database was created with build-mode: autobuild or build-mode: manual and you won't have this problem.

If running scripts against build-mode: none is likely to be unreliable, then I'm happy to get no results. However, if it's only unreliable in a few cases, then I would probably want the results.

Would you say that unreliable results are likely to be rare, or common?

[jketema]

I would like to achieve accurate results.

Then you will need to make sure that the database you're using was built with either build-mode: autobuild or build-mode: manual.

Would you say that unreliable results are likely to be rare, or common?

Hard to tell in general. It will very much depend on the codebase.