Skip to content

C/C++ overlay: update discard mechanism #21424

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
IdrissRio wants to merge 3 commits into
base: main
Choose a base branch
from
+41 −92
Open

C/C++ overlay: update discard mechanism #21424

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cbc1598
C/C++ overlay: switch to updated discard strategy
IdrissRio Mar 6, 2026
755225f
C/C++ overlay: fix failing `header_dependency` test
IdrissRio Mar 6, 2026
5c2e5f8
C/C++ overlay: address review comment
IdrissRio Mar 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
133 changes: 41 additions & 92 deletions cpp/ql/lib/semmle/code/cpp/internal/Overlay.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,117 +6,66 @@ private import OverlayXml

/**
* Holds always for the overlay variant and never for the base variant.
* This local predicate is used to define local predicates that behave
* differently for the base and overlay variant.
*/
overlay[local]
predicate isOverlay() { databaseMetadata("isOverlay", "true") }

overlay[local]
private string getLocationFilePath(@location_default loc) {
exists(@file file | locations_default(loc, file, _, _, _, _) | files(file, result))
}

/**
* Gets the file path for an element with a single location.
*/
overlay[local]
private string getSingleLocationFilePath(@element e) {
exists(@location_default loc |
var_decls(e, _, _, _, loc)
or
fun_decls(e, _, _, _, loc)
or
type_decls(e, _, loc)
or
namespace_decls(e, _, loc, _)
or
macroinvocations(e, _, loc, _)
or
preprocdirects(e, _, loc)
or
diagnostics(e, _, _, _, _, loc)
or
usings(e, _, loc, _)
or
static_asserts(e, _, _, loc, _)
or
derivations(e, _, _, _, loc)
or
frienddecls(e, _, _, loc)
or
comments(e, _, loc)
or
exprs(e, _, loc)
or
stmts(e, _, loc)
or
initialisers(e, _, _, loc)
or
attributes(e, _, _, _, loc)
or
attribute_args(e, _, _, _, loc)
or
namequalifiers(e, _, _, loc)
or
enumconstants(e, _, _, _, _, loc)
or
type_mentions(e, _, loc, _)
or
lambda_capture(e, _, _, _, _, _, loc)
or
concept_templates(e, _, loc)
|
result = getLocationFilePath(loc)
)
}

/**
* Gets the file path for an element with potentially multiple locations.
* Holds if the TRAP file or tag `t` is reachable from source file `sourceFile`
* in the base (isOverlayVariant=false) or overlay (isOverlayVariant=true) variant.
*/
overlay[local]
private string getMultiLocationFilePath(@element e) {
exists(@location_default loc |
var_decls(_, e, _, _, loc)
or
fun_decls(_, e, _, _, loc)
or
type_decls(_, e, loc)
or
namespace_decls(_, e, loc, _)
|
result = getLocationFilePath(loc)
private predicate locallyReachableTrapOrTag(
boolean isOverlayVariant, string sourceFile, @trap_or_tag t
) {
exists(@source_file sf, @trap trap |
(if isOverlay() then isOverlayVariant = true else isOverlayVariant = false) and
source_file_uses_trap(sf, trap) and
source_file_name(sf, sourceFile) and
(t = trap or trap_uses_tag(trap, t))
)
}

/**
* A local helper predicate that holds in the base variant and never in the
* overlay variant.
*/
overlay[local]
private predicate isBase() { not isOverlay() }

/**
* Holds if `path` was extracted in the overlay database.
* Holds if element `e` is in TRAP file or tag `t`
* in the base (isOverlayVariant=false) or overlay (isOverlayVariant=true) variant.
*/
overlay[local]
private predicate overlayHasFile(string path) {
isOverlay() and
files(_, path) and
path != ""
private predicate locallyInTrapOrTag(boolean isOverlayVariant, @element e, @trap_or_tag t) {
(if isOverlay() then isOverlayVariant = true else isOverlayVariant = false) and
in_trap_or_tag(e, t)
}

/**
* Discards an element from the base variant if:
* - It has a single location in a file extracted in the overlay, or
* - All of its locations are in files extracted in the overlay.
* - We have knowledge about what TRAP file or tag it is in (in the base).
* - It is not in any overlay TRAP file or tag that is reachable from an overlay source file.
* - For every base TRAP file or tag that contains it and is reachable from a base source file,
* either the source file has changed or the overlay has redefined the TRAP file or tag.
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

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

The predicate doc comment says base retention is sufficient unless the base source file changed or the overlay redefined the TRAP file/tag, but the implementation also allows retention to be bypassed when the overlay has re-extracted the same source file (the locallyReachableTrapOrTag(true, sourceFile, _) disjunct). Please update the bullet list to mention this third case so the documentation matches the logic.

Suggested change
* either the source file has changed or the overlay has redefined the TRAP file or tag.
* either the source file has changed, or the overlay has redefined the TRAP file or tag,
* or the overlay runner has re-extracted the same source file.

Copilot uses AI. Check for mistakes.
*/
overlay[discard_entity]
private predicate discardElement(@element e) {
isBase() and
(
overlayHasFile(getSingleLocationFilePath(e))
or
forex(string path | path = getMultiLocationFilePath(e) | overlayHasFile(path))
// If we don't have any knowledge about what TRAP file something
// is in, then we don't want to discard it, so we only consider
// entities that are known to be in a base TRAP file or tag.
locallyInTrapOrTag(false, e, _) and
// Anything that is reachable from an overlay source file should
// not be discarded.
not exists(@trap_or_tag t | locallyInTrapOrTag(true, e, t) |
locallyReachableTrapOrTag(true, _, t)
) and
// Finally, we have to make sure that base shouldn't retain it.
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

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

Minor clarity/grammar: "make sure that base shouldn't retain it" is a bit hard to parse. Consider rephrasing to something like "make sure the base variant shouldn't retain it" or "make sure the base variant does not retain it" for readability.

Suggested change
// Finally, we have to make sure that base shouldn't retain it.
// Finally, we have to make sure the base variant does not retain it.

Copilot uses AI. Check for mistakes.
// If it is reachable from a base source file, then that is
// sufficient unless either the base source file has changed (in
// particular, been deleted), or the overlay has redefined the TRAP
// file or tag it is in, or the overlay runner has re-extracted the same
// source file (e.g. because a header it includes has changed).
forall(@trap_or_tag t, string sourceFile |
locallyInTrapOrTag(false, e, t) and
locallyReachableTrapOrTag(false, sourceFile, t)
|
overlayChangedFiles(sourceFile) or
locallyReachableTrapOrTag(true, _, t) or
locallyReachableTrapOrTag(true, sourceFile, _)
Copy link
Member

@igfoo igfoo Mar 9, 2026

Choose a reason for hiding this comment

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

When is this last line needed? Is there a test that fails if it is removed?

Copy link
Contributor Author

@IdrissRio IdrissRio Mar 9, 2026

Choose a reason for hiding this comment

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

Yes, that is needed, or possibly a variant of it. You can reproduce this by running the header_dependency test.

In this test, a header file (change.h) changes, but the translation unit that includes it (main.c) does not. So changes.json contains the .h file and therefore overlayChangedFiles = { "change.h" }. But the overlay runner reextract main.c as well.

Because of this:

  • Line 67 does not match because main.c ∉ overlayChangedFiles
  • Line 68 does not match either, since the trap file hash content is now different due to the include.

So we need something like Line 69 to detect that the same file is being re extracted. This is essentially a way to complement Line 67, taking into consideration cases where some files have changed transitively but are not explicitly listed in overlayChangedFiles.

For this to work, we also need this change: https://github.com/github/semmle-code/pull/55032
This is required because we rely on source_file_name(sf, sourceFile), which currently does not apply path transformation.

If you want to reproduce it I suggest you to checkout this branch in semmle-code: idrissrio/cpp/overlay/path_transformer
and
this commit in ql: cbc1598
Then from semmle-code:

cd integration-tests/cpp/all-platforms/overlay/header_dependency
pytest .

)
}