analysis: add bounded MCS apply profiling - #2363
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved artifact-boundary, timeout parsing, query correctness, dependency-locking, and regression-gate issues remain.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds bounded MCS profiling and telemetry to diagnose iccApplyProfiles and CMM threading performance.
Changes:
- Adds timing, trace, MCS profiling, and CLUT-dimension telemetry.
- Adds benchmark environment-variable support and regression coverage.
- Updates CI, CodeQL tooling, and threaded-output QA.
File summaries
| File | Description |
|---|---|
Tools/CmdLine/IccBenchApply/Readme.md |
Documents environment arguments. |
Tools/CmdLine/IccBenchApply/iccBenchApply.cpp |
Implements environment hints. |
Tools/CmdLine/IccApplyProfiles/Readme.md |
Documents profiling controls. |
Tools/CmdLine/IccApplyProfiles/iccApplyProfiles.cpp |
Adds timing and tracing. |
IccProfLib/IccTagLut.cpp |
Labels 3D/4D CLUT telemetry. |
IccProfLib/IccSignatureUtils.h |
Records CLUT dimensions. |
IccProfLib/IccMpeBasic.cpp |
Checks curve setup failures. |
docs/codeql.md |
Updates local CodeQL guidance. |
docs/avx2-clut-diagnostics.md |
Documents dimension telemetry. |
Build/Cmake/Testing/CMakeLists.txt |
Adds benchmark environment tests. |
.github/workflows/ci-iccdev-tool-tests.yml |
Runs and reports MCS profiling. |
.github/workflows/ci-comprehensive-build-test.yml |
Exposes profiling inputs. |
.github/workflows/ci-build-test-no-fuzzers.yml |
Exposes profiling inputs. |
.github/workflows/_build-matrix.yml |
Forwards profiling inputs. |
.github/scripts/run-codeql-local.sh |
Uses the standalone CodeQL CLI. |
.github/scripts/iccdev-mcs-applyprofiles-profile.sh |
Implements bounded profiling. |
.github/scripts/iccdev-iccapplyprofiles-threading-regression-tests.sh |
Makes thread modes configurable. |
.github/codeql-queries/unbounded-profile-loop.ql |
Refines loop-bound detection. |
.github/codeql-queries/test/unbounded-profile-loop/UnboundedProfileLoop.expected |
Updates expected alerts. |
.github/codeql-queries/test/unbounded-profile-loop/case.cpp |
Adds query test cases. |
.github/codeql-queries/test/qlpack.yml |
Updates the C++ query dependency. |
.github/codeql-queries/test/codeql-pack.lock.yml |
Clears test-pack locks. |
.github/codeql-queries/README.md |
Documents excluded heuristics. |
.github/codeql-queries/iccdev-security-suite.qls |
Adjusts default exclusions. |
.github/codeql-queries/codeql-pack.lock.yml |
Clears query-pack locks. |
.github/ci/quality-assurance/scripts/iccApplySearch-quick-check.sh |
Checks threaded output parity. |
.github/ci/quality-assurance/scripts/iccApplyProfiles-quick-check.sh |
Checks threaded TIFF parity. |
Review details
- Files reviewed: 27/27 changed files
- Comments generated: 7
- Review effort level: Balanced
d320ed7 to
bafff7b
Compare
9d8414b to
565ddf4
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Blocking regressions remain in Docker gating, failure artifacts, TSV packaging, and CodeQL loop classification.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
.github/workflows/ci-pr-action.yml:1311
- Disabling
docker-ciremoves the only caller and required-result check forci-docker-pr.yml, including when the container surface itself changes. This contradicts the repository contract indocs/label-system.md:169-173, so container regressions can now reach a successful PR Summary without Docker verification. Restore the job and its summary/result plumbing.
# Docker PR regression verification is disabled above.
- Files reviewed: 30/30 changed files
- Comments generated: 3
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Docker verification is disabled, and several profiling, query, and threading checks have correctness gaps.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
.github/codeql-queries/unbounded-profile-loop.ql:149
- This predicate treats any field-independent comparison containing a
*min*call and a numeric argument as a hard loop bound. It therefore suppresses genuinely unbounded loops such asi < m_nCount && j > min(m_limit, 16)(or a constant comparison), without proving an upper-bound operator or that the compared induction variable advances toward the cap. Tie the exemption to an updated induction variable and an actual upper-bound comparison, and add negative cases for non-boundingmin()conditions.
not accessesField(comparison, field) and
minCall = comparison.getAChild*() and
minCall.getTarget().getName().matches("%min%") and
bound = minCall.getAnArgument() and
bound.toString().regexpMatch("^[0-9]+$")
- Files reviewed: 30/30 changed files
- Comments generated: 4
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Docker validation is disabled, an artifact sanitizer crosses the trust boundary, and the CodeQL predicate can suppress valid alerts.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
.github/workflows/ci-pr-action.yml:1210
- Disabling this lane removes the only path-gated Docker/container verification while
container_changedis still detected. Container or Docker-workflow changes can now make the aggregate PR result succeed without building or validating the affected image. Restoredocker-ciand require its result whencontainer_changedis true.
# CONTAINER_CHANGED is used only by the disabled Docker PR regression job.
# CONTAINER_CHANGED: ${{ needs.detect-src.outputs.container_changed }}
.github/codeql-queries/unbounded-profile-loop.ql:149
- This treats any field-independent comparison containing
min(..., numericLiteral)as a hard loop bound, but it never verifies that the compared value is an induction variable advanced by the loop update. For example,i < m_nCount && flag < min(limit, 16)would suppress the alert even though the second clause does not boundi. Tie the comparison operand to a variable monotonically updated by thisforstatement and add a negative fixture for an unrelated stable comparison.
not accessesField(comparison, field) and
minCall = comparison.getAChild*() and
minCall.getTarget().getName().matches("%min%") and
bound = minCall.getAnArgument() and
bound.toString().regexpMatch("^[0-9]+$")
- Files reviewed: 30/30 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Docker gating, trusted-summary sanitization, conditional CTest registration, and CodeQL loop-bound detection contain unresolved defects.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
.github/codeql-queries/unbounded-profile-loop.ql:148
- This exemption does not verify that the independently compared variable is advanced by the loop. For example,
for (i=0,j=0; i<m_nCount && j<min(limit,16); ++i)now satisfies this predicate even thoughjnever changes, suppressing an actually unbounded profile-controlled loop. Tie the comparison to an induction variable that is initialized and monotonically updated in the loop, and add a negative query test for the non-advancing case.
comparison = loop.getCondition().getAChild*() and
not accessesField(comparison, field) and
minCall = comparison.getAChild*() and
minCall.getTarget().getName().matches("%min%") and
bound = minCall.getAnArgument() and
.github/workflows/ci-iccdev-tool-tests.yml:1032
- These new PR-produced report lines are written to
GITHUB_STEP_SUMMARYusing sanitizer functions sourced from the PR checkout at line 826, even though this job now checks out a trusted-base helper. A PR can therefore alter both the report and the sanitizer and inject unsanitized Markdown/HTML into the rendered summary. Sourcetrusted-base/.github/scripts/sanitize-sed.shfor this summary step before processing the MCS report.
while IFS= read -r line; do
sanitize_code_line "$line"
echo ""
done < "$mcs_profile_report"
- Files reviewed: 29/29 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
It removes required Docker verification and introduces an unsound CodeQL suppression.
Review details
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
.github/scripts/run-codeql-local.sh:11
- This removes support for the
gh codeqlextension, but.github/codeql-queries/README.md:58-64,81-83still lists that extension as a valid prerequisite and routes users to this script. Those documented users now fail the prerequisite check; update the README and its commands, or retain an explicit extension fallback.
.github/codeql-queries/unbounded-profile-loop.ql:149
- This does not prove an independent hard bound: any numeric argument to any
%min%-named call under an unrelated relational comparison qualifies, regardless of comparison direction or whether that variable advances. For example,i < m_nCount && j > min(limit, 16)remains profile-bounded but will now be suppressed. Restrict this exemption to a proven upper-bound comparison on an advancing induction variable and add a negative query test.
not accessesField(comparison, field) and
minCall = comparison.getAChild*() and
minCall.getTarget().getName().matches("%min%") and
bound = minCall.getAnArgument() and
bound.toString().regexpMatch("^[0-9]+$")
.github/workflows/ci-pr-action.yml:1295
- Dropping the Docker result from the aggregate (together with the removed
docker-cijob/need) letsPR Summarysucceed forcontainer_changed=truewithout running Docker PR verification. This breaks the path-gated contract documented indocs/regression-workflow-governance.md:155-166; restore the Docker job, its result aggregation, and the explicit container-change check.
required_results=("$DETECT_RESULT" "$SETUP_RESULT" "$VALIDATE_RESULT" "$RISK_RESULT" "$GCC15_LTO_RESULT" "$LINT_RESULT" "$SCAN_RESULT" "$TOOL_RESULT" "$WIN_RESULT" "$MACOS_RELEASE_LTO_RESULT" "$MACOS_DEBUG_RESULT" "$EXAMPLE_RESULT")
- Files reviewed: 29/29 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Required Docker validation, trusted sanitizer boundaries, and a CodeQL false-negative need correction.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
.github/workflows/ci-pr-action.yml:1295
- Removing the Docker result from the required aggregate (together with deleting
docker-ci) leavescontainer_changedpull requests without a required pre-merge image build. The remainingci-docker.ymlautomatic triggers are onlymaster/tags, so PR Summary can now succeed before Dockerfile or packaged-container changes have been exercised. Please retain a path-gated Docker PR verification job, or require an equivalent pre-merge check and include its result here.
required_results=("$DETECT_RESULT" "$SETUP_RESULT" "$VALIDATE_RESULT" "$RISK_RESULT" "$GCC15_LTO_RESULT" "$LINT_RESULT" "$SCAN_RESULT" "$TOOL_RESULT" "$WIN_RESULT" "$MACOS_RELEASE_LTO_RESULT" "$MACOS_DEBUG_RESULT" "$EXAMPLE_RESULT")
- Files reviewed: 32/32 changed files
- Comments generated: 3
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Removing pre-merge Docker verification allows broken container changes to pass the PR aggregate check.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 32/32 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
Docker pre-merge verification is removed, and threaded timing always reports zero asynchronous work.
Review details
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
Tools/CmdLine/IccApplyProfiles/iccApplyProfiles.cpp:1054
- This value is incorrect for
-threads 0or-threads N>1: sufficiently wide row calls enterCIccApplyThreadedCmm::Apply()and dispatch background worker strips, but the diagnostic always reports zero. Either expose the actual threaded-call/worker-strip count to this timing output or omit the field so profiling results do not claim that threaded work never occurred.
docs/label-system.md:151 - This no longer matches the workflow:
ci-pr-action.ymlstill setsrun_full=truewhencontainer_changedis true (lines 623-625). Container-only changes therefore still select the full matrix even though the Docker-specific lane was removed.
.github/workflows/ci-pr-action.yml:1151
- Removing this job leaves
ci-docker-pr.ymlwith no caller, so a PR that changes the Dockerfile, container dependencies, or packaged MCP can merge without ever building the exact proposed image. Theci-dockerworkflow onmasteronly validates after merge; please retain this path-gated read-only job and its aggregate requirement, or add equivalent pre-merge container verification.
finalize:
- Files reviewed: 32/32 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Pre-merge container verification is removed and the new diagnostics contain trust-boundary and accuracy defects.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
.github/workflows/ci-pr-action.yml:1295
- Removing the Docker result from the aggregate gate leaves container-surface PRs without any pre-merge image build:
container_changednow only selects the generic matrix, whileci-docker.ymlbuilds/publishes onmasterafter merge. A broken or unsafe Dockerfile can therefore satisfyPR Summaryand be discovered only after it reaches the protected branch. Restore an equivalent read-only container verification job and require its result for container changes.
required_results=("$DETECT_RESULT" "$SETUP_RESULT" "$VALIDATE_RESULT" "$RISK_RESULT" "$GCC15_LTO_RESULT" "$LINT_RESULT" "$SCAN_RESULT" "$TOOL_RESULT" "$WIN_RESULT" "$MACOS_RELEASE_LTO_RESULT" "$MACOS_DEBUG_RESULT" "$EXAMPLE_RESULT")
.github/workflows/ci-iccdev-tool-tests.yml:695
- This sources the PR checkout's sanitizer helper even though a trusted-base checkout was added above. On pull requests, modifying
sanitize-sed.shwould execute additional head-controlled shell code in this step beyond the explicitly reviewed profiling helper. Source the trusted-base copy instead.
source .github/scripts/sanitize-sed.sh
- Files reviewed: 42/42 changed files
- Comments generated: 3
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Pre-merge Docker validation is removed, the CodeQL query has a false-negative path, and the documented timing sample is inconsistent.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
docs/regression-workflow-governance.md:159
container_changednow only selects the host build matrix; the remaining preflight performs static hadolint/Trivy checks but no workflow builds or smokes the proposed Docker image. Deferringci-dockeruntilmastermeans a Dockerfile or packaged-container regression can merge before its first image build. Retain a read-only pre-merge image build/smoke for container-surface changes, or route those changes to an equivalent required check.
- Files reviewed: 42/42 changed files
- Comments generated: 3
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Container-only PRs can skip the advertised matrix, while MCS failure attribution and CodeQL hard-cap detection remain incorrect.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
.github/workflows/ci-pr-action.yml:568
- Container-only changes still do not start the advertised full matrix. This branch sets
image_definition_changed/container_changed, butnative_changedis only set for C/C++ changes at lines 585-590, while the GCC, tool, Windows, and macOS jobs are all gated onnative_changedat lines 998-1044. Withdocker-ciremoved, a Dockerfile- or requirements-only PR can therefore finish with every build job skipped, and the summary accepts those skips. Include image-definition changes in the normal matrix selection before retiring the Docker lane.
if has_changed_file '^(Dockerfile|\.dockerignore|\.github/ci/(docker|requirements)/|iccdev-mcp/|\.github/scripts/(check-workflow-cache-policy|sanitize-sed)\.sh)'; then
image_definition_changed=true
fi
- Files reviewed: 42/42 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Loop-bound false negatives, incomplete trace sanitization, and contradictory CI documentation remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
.github/codeql-queries/unbounded-profile-loop.ql:200
- This exemption neither requires a numeric
min()argument nor proves that the call is the induction variable's actual limit. A loop such asi < min(m_nCount, m_otherProfileCount)is therefore suppressed even though both bounds are profile-controlled. Structurally requirei < min(field, numericLiteral)(in either argument order) and add a direct field-vs-fieldmin()regression.
// A min() call is a bound only when it is the loop's own limit. Nearby,
// unrelated calls formerly suppressed the loop and hid profile-controlled
// iteration.
minCall = loop.getCondition().getAChild*() and
accessesField(minCall, fa.getTarget())
Tools/CmdLine/IccApplyProfiles/iccApplyProfiles.cpp:165
- Bytes above ASCII are emitted verbatim, so UTF-8 control characters such as U+0085/U+2028 or bidi controls can still produce a rendered line break or spoofed trace text in attacker-controlled paths. Escape non-ASCII bytes as well (or decode Unicode and reject its control/separator classes) to preserve the stated one-record-per-line contract.
- Files reviewed: 42/42 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Malformed ApplyProfiles thread counts remain fail-open, and broad CodeQL function matching can miss unbounded loops.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
.github/ci/quality-assurance/scripts/iccApplyProfiles-quick-check.sh:48
- The stated fail-closed thread-count contract is not covered or met for
iccApplyProfiles: its CLI still usesatoi(argv[2])atTools/CmdLine/IccApplyProfiles/iccApplyProfiles.cpp:365, so-threads abcsilently becomes0(automatic threading). These new cases exercise only valid values; use strictstrtolplusCIccThreadedCmm::GetMaxThreads()asiccApplySearchdoes and add malformed/out-of-range rejection cases.
.github/codeql-queries/unbounded-profile-loop.ql:176
- This direct-bound predicate has the same over-broad
%min%match: unrelated functions containingmincan suppress a CWE-400 finding even when they do not enforce the numeric cap. Restrict the target name tomin.
minCall.getTarget().getName().matches("%min%") and
- Files reviewed: 42/42 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
The CodeQL exemptions can suppress genuinely unbounded loops without validating the loop’s actual induction update.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
.github/codeql-queries/unbounded-profile-loop.ql:180
- The direct-
min()exemption does not verify thatinductionis the variable advanced by thisforloop. For example,for (unsigned i = 0, j = 0; j < min(m_nCount, 16); ++i)satisfies this predicate even thoughjnever changes and the loop is unbounded whenever the condition starts true. The shared-AND predicate has the same gap. Tie the compared variable to a monotonic loop update and add negative query tests for a non-updated (and decreasing) bound variable before suppressing the alert.
bound.toString().regexpMatch("^[0-9]+$")
)
}
predicate hasDirectMinHardBound(ForStmt loop, Field field) {
exists(
RelationalOperation comparison, VariableAccess induction,
FunctionCall minCall, Expr fieldArgument, Expr numericCap
- Files reviewed: 42/42 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
The loop-bound query can suppress unbounded loops when induction increments execute only conditionally.
Review details
Suppressed comments (1)
.github/codeql-queries/unbounded-profile-loop.ql:146
getAChild*()treats conditionally executed increments as guaranteed loop progress. For example,for (...; i < min(m_nCount, 16); flag && i++)satisfies this predicate even whenflagis false, so both hard-bound predicates suppress an actually unbounded profile-driven loop. Restrict the match to an increment that executes unconditionally, and add a conditional/short-circuit update regression.
increment = loop.getUpdate().getAChild*() and
increment.getOperand() = access and
access.getTarget() = variable
- Files reviewed: 42/42 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
The batched ApplyProfiles path ignores worker failures and can write incomplete output while exiting successfully.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 31/31 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
The stated Docker-lane retirement is absent, and the still-active Docker verification currently fails.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 31/31 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Profiling duration accounting violates the stated contract, and the CodeQL exemption misses direct numeric hard caps.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 31/31 changed files
- Comments generated: 2
- Review effort level: Balanced
| minCall = capComparison.getGreaterOperand() and | ||
| minCall.getTarget().getName() = "min" and | ||
| bound = minCall.getAnArgument() and | ||
| bound.toString().regexpMatch("^[0-9]+$") |
|
Copilot has not been helpful. Closing this Stack. Complete waste of time, again.... |
There was a problem hiding this comment.
🟡 Changes recommended
Profiling can miss its target, artifact logs are unbounded, and the stated CodeQL changes are absent.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 20/20 changed files
- Comments generated: 4
- Review effort level: Balanced
| for log in "${mcs_profile_dir}"/runs/*.log; do | ||
| [ -f "$log" ] || continue | ||
| copy_sanitized_text_if_present \ | ||
| "$log" \ | ||
| "${data_dir}/mcs-applyprofiles-profile/$(basename "$log")" |
| for log in "${mcs_profile_dir}"/runs/*.log; do | ||
| [ -f "$log" ] || continue | ||
| copy_sanitized_text_if_present \ | ||
| "$log" \ | ||
| "${report_dir}/$(basename "$log")" |
| if (remaining > 0) | ||
| print int((remaining / calibration) + 0.5) |
| - name: Run bounded MCS iccApplyProfiles profile | ||
| id: mcs_profile | ||
| if: ${{ inputs.run_mcs_profile }} |
Post Closure Report2026-09-03 13:38:15 UTC This Code will be presented in a new PR that is slimmed down without any CTest or significant Documentation Changes to avoid Review Churn. The Threading Code is needed to reduce total Engagement Time; Goal to reduce the Elapsed Time for the Build, Profile Creation & Image Processing Pipeline Repro |
PR Summary
Implements bounded MCS profiling, threaded ApplySearch/ApplyProfiles diagnostics, focused CodeQL loop-bound coverage, and associated CI/reporting hardening.
Tracks #2346 and #2348.
Scope and acceptance contract
-threads 2exercises row dispatch; timing reports actual queued background worker strips instead of a fixed value.Testing/mcs/prev.tifquick check requires positive worker-strip output.&&operands orinduction < min(profile field, numeric literal). `trusted-base/.github/scripts/sanitize-sed.sh.ci-docker.yml, while container changes select the normal native platform matrix.ci-docker-pr.ymland its required-gate consumer are removed; GCC, Windows, and macOS validation remain in the PR summary.Validation evidence
codeql test run --additional-packs=.github/codeql-queries .github/codeql-queries/testpasses, including nested-OR and field-onlymin()cap regressions.iccApplyProfiles-quick-check.shpasses threaded output parity, positive worker-strip reporting, and newline/UTF-8 trace encoding.actionlintpasses for the affected workflow changes, andgit diff --checkpasses.