Skip to content

C++: Prepare for model generation adoption #19274

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 6 commits into from
Apr 11, 2025

Conversation

MathiasVP
Copy link
Contributor

@MathiasVP MathiasVP commented Apr 10, 2025

This PR does the necessary changes to existing C++ code required for model generation. It should be a no-opt in terms of results and performance.

DCA was uneventful (as expected).

@github-actions github-actions bot added the C++ label Apr 10, 2025
@MathiasVP MathiasVP force-pushed the prepare-cpp-for-mad-generation branch from 4e4ae32 to 3bb249f Compare April 10, 2025 13:28
@MathiasVP MathiasVP added the no-change-note-required This PR does not need a change note label Apr 10, 2025
@MathiasVP MathiasVP marked this pull request as ready for review April 10, 2025 16:35
@Copilot Copilot AI review requested due to automatic review settings April 10, 2025 16:35
@MathiasVP MathiasVP requested a review from a team as a code owner April 10, 2025 16:35
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 wasn't able to review any files in this pull request.

Files not reviewed (4)
  • cpp/ql/lib/semmle/code/cpp/dataflow/ExternalFlow.qll: Language not supported
  • cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll: Language not supported
  • cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll: Language not supported
  • cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/TaintTrackingUtil.qll: Language not supported

tausbn
tausbn previously approved these changes Apr 11, 2025
Copy link
Contributor

@tausbn tausbn left a comment

Choose a reason for hiding this comment

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

Two typos, otherwise this looks okay to my (relatively untrained on this codebase) eyes.

*/
bindingset[f]
pragma[inline_late]
string getParameterTypeWithoutTemplateArguments(Function f, int n) {
string getParameterTypeWithoutTemplateArguments(Function f, int n, boolean canonical) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Where/how will canonical be used? I only see this predicate used with a wildcard argument for that parameter in this PR.

Copy link
Contributor Author

@MathiasVP MathiasVP Apr 11, 2025

Choose a reason for hiding this comment

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

Your observation is correct! Currently, we're fairly lenient in with the parameter names we accept when mapping a MaD representation to the targeted function. For example:

using MyIntType = int;
void f(MyIntType t);

will both match the parameter list (int) and (MyIntType) (because, annoyingly, some implementations of the C++ standard library differ on whether they use an internal typedef'd name, and what that internal typedef is even called).

However when we want to generate a MaD row (i.e., what model generation will be doing) we don't want to map a function to many parameter lists. So:

  • When we consume MaD rows we will always use _ for the canonical parameter (because we want to accept many possible names for the parameter type), but
  • When we produce MaD rows we need to pick 1 parameter type name. So when this PR is merged I'll open the PR that actually adds the model generation, and this library will use true for canonical.

@MathiasVP MathiasVP merged commit 877118f into github:main Apr 11, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C++ no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants