[et] Refactor gn post-processing, fix minor clang[++] regex match bug#189685
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request extracts the logic for stripping compiler wrappers from compilation database commands into a dedicated utility file (update_compdb.dart) and introduces unit tests for this functionality. Feedback highlights a potential issue where the greedy regular expression used to match compiler commands can cause truncation if subsequent arguments contain the string "clang". It is recommended to update the regular expression to be non-greedy and to add test cases verifying that commands with such arguments are not truncated.
| // Use of this source code is governed by a BSD-style license that can be | ||
| // found in the LICENSE file. | ||
|
|
||
| final RegExp _clangRegex = RegExp(r'("command"\s*:\s*").*(\s(?:\S*/)?clang(\+\+)?)(?=[\s"])'); |
There was a problem hiding this comment.
The current regex still uses a greedy .* match, which can cause severe command truncation bugs if any subsequent argument or path in the command ends with /clang or is exactly clang (for example, an include path like -I /path/to/other/clang).
How it fails:
For an input like:
"command": "/path/to/rewrapper --cfg=... ../../clang/bin/clang++ -I /path/to/other/clang"The greedy .* matches all the way to the second clang, truncating the command to:
"command": "/path/to/other/clang"Even if there is no wrapper, a command like:
"command": "../../clang/bin/clang++ -I /path/to/other/clang"will be truncated because the first clang++ is not preceded by a space (so the \s in your regex doesn't match it), forcing the regex to match the second clang and truncating the command.
Solution:
We can make the match non-greedy ([^"]*?) and allow the compiler path to be preceded by either a space (\s) or the opening quote of the command ((?<=")). This ensures we always match the first compiler invocation (the actual executable) and never truncate subsequent arguments.
| final RegExp _clangRegex = RegExp(r'("command"\s*:\s*").*(\s(?:\S*/)?clang(\+\+)?)(?=[\s"])'); | |
| final RegExp _clangRegex = RegExp(r'("command"\s*:\s*")[^"]*?((?:\s|(?<="))(?:\S*/)?clang(\+\+)?)(?=[\s"])'); |
There was a problem hiding this comment.
First off, this isn't a new issue in this PR; it's an existing issue.
Second, this will never happen in the current codebase. This issue that my patch fixes does happen in the current codebase.
Third (but irrelevant), the proposed fix here will get the -I /blah/clang case right, but it'll fail on the equally silly case where we precede the correct clang++ with another clang.
Personally I think the code is fine as-is, but my followup Swift LSP patch actually does tokenise lines using swift.py (since it looks at individual flags), which avoids the whole issue of regular expressions. I'll push that.
There was a problem hiding this comment.
Turns out the tokenisation code in my followup isn't easily re-usable for this.
My vote would be to just keep the current regex-based approach.
This patch as-is keeps the existing approach, fixes an existing broken case, fixes the usecase for pure Obj-C translation units (using clang instead of clang++), and doesn't break anything that works today.
I scraped our whole compile_commands.json for anything even remotely resembling the issue raised here and it doesn't exist. For this to break we would need to add a third-party lib literally named clang.
I did write a straight parser-based approach that fixes this in about 30 lines of code. I can send that out in a later PR, but I'd prefer not to make this patch any more complicated with an orthogonal fix for an existing issue.
@jtmcdole if you disagree, let me know and I can send that out as part of this.
| expect(stripCompilerWrappers(input), equals(input)); | ||
| }); |
There was a problem hiding this comment.
To prevent regressions and verify that paths ending in clang or commands without wrappers are not truncated, please add these test cases.
expect(stripCompilerWrappers(input), equals(input));
});
test('does not truncate the command when a later argument is a path ending in clang', () {
const input = r'''
[
{
"file": "../../flutter/foo.cc",
"directory": "/out/config",
"command": "/path/to/rewrapper --cfg=... ../../clang/bin/clang++ -I /path/to/other/clang"
}
]
''';
const expected = r'''
[
{
"file": "../../flutter/foo.cc",
"directory": "/out/config",
"command": "../../clang/bin/clang++ -I /path/to/other/clang"
}
]
''';
expect(stripCompilerWrappers(input), equals(expected));
});
test('does not truncate the command when there is no wrapper but a later argument is a path ending in clang', () {
const input = r'''
[
{
"file": "../../flutter/foo.cc",
"directory": "/out/config",
"command": "../../clang/bin/clang++ -I /path/to/other/clang"
}
]
''';
expect(stripCompilerWrappers(input), equals(input));
});There was a problem hiding this comment.
Similar to above, this doesn't happen at all in the existing code, so no -- for now. We could consider this in a followup, but it's orthogonal to the refactoring.
d452521 to
7bb3f4b
Compare
This is pre-factoring for a followup patch that adds SourceKit-LSP support for Swift, which needs its own post-processing pass over `compile_commands.json` living in the same file. This also tweaks the existing clang/clang++ match regex to handle both clang and clang++ and adds tests. In the existing code, `_postGn()` strips `rewrapper`/`ccache` wrapper prefixes off compiler invocations in `compile_commands.json` so `clangd` doesn't choke on RBE wrapper commands, but the regex only matched `clang++`, so C compiles were never stripped, and a greedy submatch meant it could latch onto the wrong occurrence of `clang` on the line and truncate the whole command. `.*` could match the *rightmost* occurrence of `clang`/`clang++` on the line rather than the actual compiler invocation: either a later `-Xclang` flag, or a source file whose name happens to contain "clang". As it turns out, there's already such a file in the repo: `clang_static_analyzer_wrapper_test.cc`. Either would silently truncate the whole command down to whatever came after that spurious match. I've fixed this requiring `clang`/`clang++` be preceded by `/` (or start-of-word) and followed by a word boundary, so it can only match an actual compiler path. I also updated `_postGn()`'s write-back check to be a plain before/after string comparison instead of counting regex matches. This is equivalent here since the regex never matches zero-length. More importantly (for me), I've extracted the logic into a `stripCompilerWrappers()` function in a new file. In a followup patch, I'll add support for rewriting our current `swiftc.py` invocations to `swiftc` invocations so that Apple's SourceKit LSP understands them. See: flutter#147767 Issue: flutter#185741
7bb3f4b to
dcd9f7f
Compare
ievdokdm
left a comment
ievdokdm
left a comment
There was a problem hiding this comment.
LGTM of refactoring, make sure regex change is sufficient
flutter/flutter@cab057d...1ac2e82 2026-07-21 [email protected] Add BaseWindowController.isDestroyed flag (flutter/flutter#189061) 2026-07-21 [email protected] Roll Dart SDK from 666e1e2133b7 to 3b2f5ad7718d (1 revision) (flutter/flutter#189745) 2026-07-21 [email protected] Roll Skia from 3c52d80c960d to 569534e9fa59 (5 revisions) (flutter/flutter#189766) 2026-07-21 [email protected] Roll Skia from 11426cf7aaa1 to 3c52d80c960d (2 revisions) (flutter/flutter#189758) 2026-07-21 [email protected] Roll pub packages (flutter/flutter#189760) 2026-07-21 [email protected] [iOS] Inject DisplayLinkManager into FlutterKeyboardInsetManager (flutter/flutter#189751) 2026-07-21 [email protected] Treat package:stack_trace async-gap marker as asynchronous suspension (flutter/flutter#185791) 2026-07-21 [email protected] [iOS] Inject DisplayLinkManager into VsyncWaiterIOS (flutter/flutter#189749) 2026-07-20 [email protected] Use String builder in `InputConnectionAdaptorTest.java` (flutter/flutter#189281) 2026-07-20 [email protected] Fix non-independant tests in draggable_test.dart (flutter/flutter#186898) 2026-07-20 [email protected] Stop using ReLinker to load libflutter.so on Android 17 (API 37+) (flutter/flutter#189146) 2026-07-20 [email protected] [et] Refactor gn post-processing, fix minor clang[++] regex match bug (flutter/flutter#189685) 2026-07-20 [email protected] Take plugin_test_darwin out of bringup (flutter/flutter#189594) 2026-07-20 [email protected] Roll pub packages (flutter/flutter#189596) 2026-07-20 [email protected] widgets layer re-export ScrollCacheExtent (flutter/flutter#189483) 2026-07-20 [email protected] Roll Skia from 47143b6fa402 to 11426cf7aaa1 (7 revisions) (flutter/flutter#189740) 2026-07-20 [email protected] Sync CHANGELOG.md from stable (flutter/flutter#189735) 2026-07-20 [email protected] [Impeller] In the AHBTextureSourceVK destructor, do not call Vulkan APIs if the VkDevice has been destroyed (flutter/flutter#189666) 2026-07-20 [email protected] Add AutofillHints.emailOTPCode for Android AUTOFILL_HINT_EMAIL_OTP (flutter/flutter#188123) 2026-07-20 [email protected] Add a github workflow to remove flutteractionsbot branches after the PRs are merged or closed. (flutter/flutter#189668) 2026-07-20 [email protected] Roll ANGLE to cc08479fbcc1 (flutter/flutter#189595) 2026-07-20 [email protected] Roll Fuchsia Linux SDK from NL8xtzr8cxr5E8r8E... to GswhlPRO-D1qSNclx... (flutter/flutter#189715) 2026-07-20 [email protected] Roll Skia from ba90f98535de to 47143b6fa402 (4 revisions) (flutter/flutter#189721) If this roll has caused a breakage, revert this CL and stop the roller 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
2 participants
This is pre-factoring for a followup patch that adds SourceKit-LSP support for Swift, which needs its own post-processing pass over
compile_commands.jsonliving in the same file. This also tweaks the existing clang/clang++ match regex to handle both clang and clang++ and adds tests.In the existing code,
_postGn()stripsrewrapper/ccachewrapper prefixes off compiler invocations incompile_commands.jsonsoclangddoesn't choke on RBE wrapper commands, but the regex only matchedclang++, so C compiles were never stripped, and a greedy submatch meant it could latch onto the wrong occurrence ofclangon the line and truncate the whole command..*could match the rightmost occurrence ofclang/clang++on the line rather than the actual compiler invocation: either a later-Xclangflag, or a source file whose name happens to contain "clang". As it turns out, there's already such a file in the repo:clang_static_analyzer_wrapper_test.cc. Either would silently truncate the whole command down to whatever came after that spurious match.I've fixed this requiring
clang/clang++be preceded by/(or start-of-word) and followed by a word boundary, so it can only match an actual compiler path. I also updated_postGn()'s write-back check to be a plain before/after string comparison instead of counting regex matches. This is equivalent here since the regex never matches zero-length.More importantly (for me), I've extracted the logic into a
stripCompilerWrappers()function in a new file. In a followup patch, I'll add support for rewriting our currentswiftc.pyinvocations toswiftcinvocations so that Apple's SourceKit LSP understands them.See: #147767
Issue: #185741
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.