Fix StrcmpFixed matching prefixes in the Vulkan embedder tests by jwinarske · Pull Request #192999 · flutter/flutter

jwinarske · 2026-09-18T15:33:03Z

StrcmpFixed compares N - 1 bytes, which stops at the literal's last character, so it matches any name the literal is a prefix of. "vkQueueSubmit" also claims "vkQueueSubmit2", and the test hands back a vkQueueSubmit wrapper for it.

Nothing in tree resolves both today, so this is latent. A dispatcher that does gets a function with the wrong signature called with vkQueueSubmit2 arguments, and segfaults in the driver. Impeller's dispatcher resolves both — that is how I found it.

Compare N bytes: the terminator is part of the literal, so the match is exact.

No new test. The existing swap tests cover it once something resolves both names; adding a test that asserts a helper's byte count is not worth the line.

First of six, from #187525. Independent — nothing else in the series depends on it landing first.

Pre-launch checklist

No API change, no docs affected.
Existing EmbedderTest.CanSwapOutVulkanCalls passes (host_debug, Linux/SwiftShader).

gemini-code-assist

Code Review

This pull request updates the StrcmpFixed helper function in embedder_vk_unittests.cc to use strncmp with the full length of the string literal, ensuring exact matching and preventing incorrect prefix matches. The reviewer suggests further simplifying the implementation by replacing strncmp with strcmp to improve readability and maintainability.

Comparing N - 1 bytes stops at the literal's last character, so the comparison matches any name the literal is a prefix of. "vkQueueSubmit" therefore also claims "vkQueueSubmit2", and the test hands back a vkQueueSubmit wrapper for it. Nothing resolves both today. A dispatcher that does gets a function with the wrong signature and calls it with vkQueueSubmit2 arguments, which segfaults inside the driver. Both sides are null-terminated, so use strcmp and drop the length arithmetic entirely rather than fixing the off-by-one.

andywolff

LGTM, thanks. One nit

andywolff · 2026-09-24T20:17:50Z

+  return std::strcmp(str1, str2);
 }

+


this extra whitespace is causing a formatting check to fail, please remove

Suggested change

Removed in c73591c.

The formatting check rejects it.

gaaclarke · 2026-09-25T16:54:38Z

+// Exact match. The previous form compared N - 1 bytes of a string literal,
+// which stops at its last character and so matched any name the literal is a
+// prefix of: "vkQueueSubmit" also claimed "vkQueueSubmit2", handing back a
+// function with the wrong signature. Both sides are null-terminated, so strcmp
+// says what is meant and cannot be off by one.
 template <size_t N>
 int StrcmpFixed(const char* str1, const char (&str2)[N]) {
-  return strncmp(str1, str2, N - 1);
+  return std::strcmp(str1, str2);
 }


The comment doesn't make sense in context of being in the code, it more speaks to the diff of the code.

The name is no longer apropos to the meaning of the function. I'd just delete this function and use std:strcmp where you want.

Done in 8dad136 — the function is gone and the three call sites use std::strcmp directly. Agreed on both counts: once it stopped doing the length arithmetic there was nothing left for the name to describe, and the comment was explaining the change rather than the code.

Once it stopped doing length arithmetic the name no longer described it, and the comment spoke to the diff rather than to the code.

…o pr1/strcmp

flar

LGTM

andywolff · 2026-09-25T21:34:25Z

google testing check got stuck due to some kind of infra issue, I'm going to try restarting it by updating the branch

edit: whoops, that was meant for #188855

auto-submit · 2026-09-25T23:02:06Z

autosubmit label was removed for flutter/flutter/192999, because - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

gaaclarke · 2026-09-25T23:12:53Z

@andywolff the google testing is just in a bad state right now, overriding.

andywolff · 2026-09-29T23:02:00Z

the checks seem to have gotten stuck. To unstick them and hopefully allow autosubmit to work, I've pushed a merge

auto-submit · 2026-09-30T02:03:30Z

autosubmit label was removed for flutter/flutter/192999, because - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

andywolff · 2026-09-30T04:17:13Z

Google testing failure looks like a flake to me. I overrode it and am reapplying the autosubmit label

flutter/flutter@55b8f88...d649d2b 2026-09-30 [email protected] [Android] Update the CLI to reject passing engine configuration flags with a prebuilt binary in release mode (flutter/flutter#190870) 2026-09-30 [email protected] Roll Skia from af1e8b356dd1 to c7b323126bf0 (1 revision) (flutter/flutter#193569) 2026-09-30 [email protected] [web] Respect text affinity in getLineBoundary at a soft wrap (flutter/flutter#192664) 2026-09-30 [email protected] [tool] Migrate TestCommand and platform runner to modular dependency injection (flutter/flutter#190789) 2026-09-30 [email protected] Roll Skia from 3a20a0464d25 to af1e8b356dd1 (1 revision) (flutter/flutter#193564) 2026-09-30 [email protected] Roll Skia from 554ef62d11a9 to 3a20a0464d25 (6 revisions) (flutter/flutter#193556) 2026-09-30 [email protected] Use null aware elements in `platform_views.dart` (flutter/flutter#193214) 2026-09-30 [email protected] Replace deprecated `withOpacity` in `flutter_logo.dart` (flutter/flutter#192583) 2026-09-30 [email protected] Roll Skia from f441ca223b2b to 554ef62d11a9 (72 revisions) (flutter/flutter#193548) 2026-09-30 [email protected] Fix StrcmpFixed matching prefixes in the Vulkan embedder tests (flutter/flutter#192999) 2026-09-30 [email protected] Update Engine to test Android 17 AVD (flutter/flutter#193239) 2026-09-29 [email protected] Give the Vulkan test context the extensions Impeller requires (flutter/flutter#193002) 2026-09-29 [email protected] Rename "subtext" to "supportingTextPadding" in `FormField`'s documentation (flutter/flutter#183582) 2026-09-29 [email protected] Set --no-js-strongly-connected-components for DDC compiles by default (flutter/flutter#193485) 2026-09-29 [email protected] Split pre-AGP 8.3 module AAR test into a Java 17 pinned target (flutter/flutter#193467) 2026-09-29 [email protected] Remove no-shuffle from gen_defaults_test (flutter/flutter#192280) 2026-09-29 [email protected] Update guidance on bumping Dart (flutter/flutter#193131) 2026-09-29 [email protected] Remove vestigial `download_jdk` gclient var (flutter/flutter#188571) 2026-09-29 [email protected] Roll pub packages (flutter/flutter#193522) 2026-09-29 [email protected] [flutter_tools] Lazily initialize AndroidSdk platform and build-tools discovery (flutter/flutter#191972) 2026-09-29 [email protected] [tool] Migrate Web build subcommands and toolchain to modular dependency injection (flutter/flutter#190783) 2026-09-29 [email protected] Pin androidx.test dependencies in integration_test (flutter/flutter#193316) 2026-09-29 [email protected] [flutter_tools] Include flutter.js.map in web builds (flutter/flutter#192257) 2026-09-29 [email protected] [WebParagraph] Fixing edge cases for wrapping text (with newlines) (flutter/flutter#189858) 2026-09-29 [email protected] Bump customer testing version for flutter/devtools update (flutter/flutter#193511) 2026-09-29 [email protected] Roll Packages from ba0364a to 3c6ce59 (14 revisions) (flutter/flutter#193507) 2026-09-29 [email protected] [flutter_tools] Guard socket streams and done futures against socket reset errors (flutter/flutter#192941) If this roll has caused a breakage, revert this CL and set the roller to dry run mode using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

flutter-dashboard Bot added the CICD Run CI/CD label Sep 18, 2026

This was referenced Sep 18, 2026

Run the shared Vulkan embedder tests under Impeller as well as Skia #193004

Open

[Embedder API] Support Making a Render Target from Vulkan backing store #187525

Open

gemini-code-assist Bot reviewed Sep 18, 2026

View reviewed changes

Comment thread engine/src/flutter/shell/platform/embedder/tests/embedder_vk_unittests.cc Outdated

github-actions Bot added the engine flutter/engine related. See also e: labels. label Sep 18, 2026

jwinarske force-pushed the pr1/strcmp branch from 10abc91 to ade9874 Compare September 18, 2026 16:15

jwinarske mentioned this pull request Sep 18, 2026

Make an Impeller render target from a Vulkan backing store #193003

Open

gaaclarke requested a review from andywolff September 21, 2026 18:13

andywolff reviewed Sep 24, 2026

View reviewed changes

jwinarske and others added 2 commits September 24, 2026 15:45

Drop the doubled blank line after StrcmpFixed

c73591c

The formatting check rejects it.

Merge branch 'master' into pr1/strcmp

f1d1832

andywolff previously approved these changes Sep 25, 2026

View reviewed changes

gaaclarke requested changes Sep 25, 2026

View reviewed changes

jwinarske added 2 commits September 25, 2026 11:22

Drop StrcmpFixed and call std::strcmp directly

8dad136

Once it stopped doing length arithmetic the name no longer described it, and the comment spoke to the diff rather than to the code.

Merge branch 'pr1/strcmp' of https://github.com/jwinarske/flutter int…

3c8d5a7

…o pr1/strcmp

jwinarske dismissed andywolff’s stale review via 3c8d5a7 September 25, 2026 18:23

gaaclarke approved these changes Sep 25, 2026

View reviewed changes

gaaclarke requested a review from andywolff September 25, 2026 18:28

flar approved these changes Sep 25, 2026

View reviewed changes

andywolff approved these changes Sep 25, 2026

View reviewed changes

andywolff added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 25, 2026

Merge branch 'master' into pr1/strcmp

364bb54

auto-submit Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Sep 25, 2026

gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 25, 2026

Merge branch 'master' into pr1/strcmp

7fdd836

auto-submit Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Sep 30, 2026

andywolff added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 30, 2026

auto-submit Bot added this pull request to the merge queue Sep 30, 2026

Merged via the queue into flutter:master with commit 2741d83 Sep 30, 2026
24 checks passed

flutter-dashboard Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Sep 30, 2026

Conversation

jwinarske commented Sep 18, 2026

Pre-launch checklist

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

andywolff left a comment

Choose a reason for hiding this comment

Uh oh!

andywolff Sep 24, 2026

Choose a reason for hiding this comment

Uh oh!

jwinarske Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

gaaclarke Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

jwinarske Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

flar left a comment

Choose a reason for hiding this comment

Uh oh!

andywolff commented Sep 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

auto-submit Bot commented Sep 25, 2026

Uh oh!

gaaclarke commented Sep 25, 2026

Uh oh!

andywolff commented Sep 29, 2026

Uh oh!

auto-submit Bot commented Sep 30, 2026

Uh oh!

andywolff commented Sep 30, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

andywolff commented Sep 25, 2026 •

edited

Loading