Skip to content

Rust: Update legacy MaD models 4 #19948

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.

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

Merged
merged 8 commits into from
Jul 10, 2025
Merged

Rust: Update legacy MaD models 4 #19948

merged 8 commits into from
Jul 10, 2025

Conversation

geoffw0
Copy link
Contributor

@geoffw0 geoffw0 commented Jul 1, 2025

Final update of legacy MaD models to the new model format (continues from #19946 and should be independent of that).

There's a lot of guesswork involved in this last set of changes - i.e. cases where I couldn't find a real getCanonicalPath. There should be tests for most of them though so we will find out if we're way off.

@geoffw0 geoffw0 added no-change-note-required This PR does not need a change note Rust Pull requests that update Rust code labels Jul 1, 2025
@geoffw0
Copy link
Contributor Author

geoffw0 commented Jul 2, 2025

I think it will be prudent to get the first two/three PRs merged before attempting to address the test failures here.

@aibaars
Copy link
Contributor

aibaars commented Jul 4, 2025

The test failures look libc related. It looks like the fn free() in libc doesn't have a result for ItemNode::getCanonicalPath() .

The relevant bits of code are

I would expect the canonical path to be either libc::unix::free or libc::free.

This should address the underlying issue #19988

@geoffw0
Copy link
Contributor Author

geoffw0 commented Jul 8, 2025

I've brought in the fix for extern blocks from main, fixed the libc models, and fixed a couple of typos in other models. There are still a few regressions, but I feel they're at a level where we can merge this and come back to them later.

@geoffw0 geoffw0 marked this pull request as ready for review July 8, 2025 10:49
@Copilot Copilot AI review requested due to automatic review settings July 8, 2025 10:49
@geoffw0 geoffw0 requested a review from a team as a code owner July 8, 2025 10:49
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copy link
Contributor

@aibaars aibaars left a comment

Choose a reason for hiding this comment

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

I think all the MISSING annotations can be fixed by updating the models to match the actual canonical paths. The following queries can come in handy to investigate what the computed canonical paths are:

query predicate resolvedPaths2(CallExprBase e, string path, Addressable target) {
  toBeTested(e) and
  target = e.getStaticTarget() and
  path = concat(target.getCanonicalPath())
}

query predicate resolvedPaths3( Addressable target, string path) { path = target.getCanonicalPath() }

Copy link
Contributor

@aibaars aibaars left a comment

Choose a reason for hiding this comment

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

This looks good to me!

@geoffw0
Copy link
Contributor Author

geoffw0 commented Jul 10, 2025

The following queries can come in handy to investigate what the computed canonical paths are

Yes I've been doing something similar. The issue being that quite a lot of canonical paths were missing (the situation is improving quite fast) so I had to fill in the gaps myself, and I appear to have made a few typos in that process.

@geoffw0
Copy link
Contributor Author

geoffw0 commented Jul 10, 2025

DCA LGTM. This is ready for approval I think.

@geoffw0 geoffw0 merged commit 3debd1a into github:main Jul 10, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-change-note-required This PR does not need a change note Rust Pull requests that update Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants