Replace several NSAssert with FML_CHECK/DCHECK to unblock the build #53048

bc-lee · 2024-05-26T07:51:03Z

In flutter/buildroot#860, disabling NSAssert in release build was added, and #53005 tries to roll the change into the engine. However, the change is blocked due to several compilation errors in the engine.

This is due to the fact that Google's DCHECK(in flutter engine, FML_DCHECK) does not make unused variable warning even in release build, while traditional assertion macro like assert or NSAssert does. For example, the following code will be expanded like this:
code:

FML_DCHECK(status == kCVReturnSuccess && pxbuffer != nullptr) <<
   "Failed to create pixel buffer";

expanded code:

true || (status == kCVReturnSuccess && pxbuffer != nullptr)
    ? (void)0 ::
    : fml::LogMessageVoidify() &
          ::fml::LogMessage(::fml::kLogFatal, 0, 0, nullptr).stream()

However, equivalent code with NSAssert will make unused variable warning, since the expanded code doesn't have any reference to the variable:
code:

NSAssert(status == kCVReturnSuccess && pxbuffer != NULL, @"Failed to create pixel buffer.");

expanded code:

do {
} while (0)

To unblock the build, this CL replaces several NSAssert with FML_DCHECK in the engine codebase. This CL isn't aimed to replace all NSAssert with FML_DCHECK since discussions in flutter/buildroot#860 are not finalized whether to replace all NSAssert with FML_DCHECK or not.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

In flutter/buildroot#860 , disabling NSAssert in release build was added, and flutter#53005 tries to roll the change into the engine. However, the change is blocked due to several compilation errors in the engine. This is due to the fact that Google's DCHECK(in flutter engine, FML_DCHECK) does not make unused variable warning even in release build, while traditional assertion macro like assert or NSAssert does. For example, the following code will be expanded like this: code: FML_DCHECK(status == kCVReturnSuccess && pxbuffer != nullptr) << "Failed to create pixel buffer"; expanded code: true || (status == kCVReturnSuccess && pxbuffer != nullptr) ? (void)0 :: : fml::LogMessageVoidify() & ::fml::LogMessage(::fml::kLogFatal, 0, 0, nullptr).stream() However, equivalent code with NSAssert will make unused variable warning, since the expanded code doesn't have any reference to the variable: code: NSAssert(status == kCVReturnSuccess && pxbuffer != NULL, @"Failed to create pixel buffer."); expanded code: do { } while (0) To unblock the build, this CL replaces several NSAssert with FML_DCHECK in the engine codebase. This CL isn't aimed to replace all NSAssert with FML_DCHECK since discussions in flutter/buildroot#860 are not finalized whether to replace all NSAssert with FML_DCHECK or not.

jmagman · 2024-05-28T22:44:47Z

shell/platform/darwin/macos/framework/Source/FlutterEmbedderExternalTextureTest.mm

  NSAssert(status == kCVReturnSuccess && pxbuffer != NULL, @"Failed to create pixel buffer.");
+  FML_DCHECK(status == kCVReturnSuccess && pxbuffer != nullptr) << "Failed to create pixel buffer";


Any reason not to switch this just to a FML_DCHECK?

Could you remove the NSAssert then? It sounds like we would ideally migrate them all to FML_*CHECK.

Yes. I forgot to remove that line in the last commit.
Whether to migrate all NSAssert to FML_*CHECK or not is up to the flutter team's decision :)

I forgot to delete the line with NSAssert in the previous commit :)

jmagman · 2024-05-29T18:13:14Z

shell/platform/darwin/macos/framework/Source/FlutterEmbedderExternalTextureTest.mm

@@ -59,7 +59,7 @@ - (CVPixelBufferRef)pixelBuffer {
  CVPixelBufferRef pxbuffer = NULL;
  CVReturn status = CVPixelBufferCreate(kCFAllocatorDefault, _width, _width, _pixelFormatType,
                                        (__bridge CFDictionaryRef)options, &pxbuffer);
-  NSAssert(status == kCVReturnSuccess && pxbuffer != NULL, @"Failed to create pixel buffer.");
+  FML_DCHECK(status == kCVReturnSuccess && pxbuffer != nullptr) << "Failed to create pixel buffer";


"Value stored to 'status' during its initialization is never read"

/Volumes/Work/s/w/ir/cache/builder/src/flutter/shell/platform/darwin/macos/framework/Source/FlutterEmbedderExternalTextureTest.mm:60:12: error: Value stored to 'status' during its initialization is never read [clang-analyzer-deadcode.DeadStores,-warnings-as-errors] 60 | CVReturn status = CVPixelBufferCreate(kCFAllocatorDefault, _width, _width, _pixelFormatType, | ^~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 61 | (__bridge CFDictionaryRef)options, &pxbuffer); | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Hmm, why would this be happening with the FML_DCHECK since, as you point out, it shouldn't cause unused variable warnings?
https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8746648441899682977/+/u/test:_test:_lint_host_debug/stdout

Well, I'll try to run clang-tidy locally to check if there are any problems.

I added another comment w.r.t. the advantages of FML_DCHECK over NSAssert in the context of the Flutter engine codebase. Link to the comment

jmagman

LGTM, thank you!

cbracken

…he build (flutter/engine#53048)

…149367) flutter/engine@bf2e32d...4956e98 2024-05-30 yjbanov@google.com [semantics] fix dartdoc grammar (flutter/engine#53121) 2024-05-30 skia-flutter-autoroll@skia.org Roll Skia from 188ad395c3e7 to a058f601e1fb (1 revision) (flutter/engine#53131) 2024-05-30 daniel.l@hpcnt.com Replace several NSAssert with FML_CHECK/DCHECK to unblock the build (flutter/engine#53048) 2024-05-30 30870216+gaaclarke@users.noreply.github.com [Impeller] make sure buffers are 4 aligned for foreground color blending (flutter/engine#53077) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC jacksongardner@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: 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

…lutter#149367) flutter/engine@bf2e32d...4956e98 2024-05-30 yjbanov@google.com [semantics] fix dartdoc grammar (flutter/engine#53121) 2024-05-30 skia-flutter-autoroll@skia.org Roll Skia from 188ad395c3e7 to a058f601e1fb (1 revision) (flutter/engine#53131) 2024-05-30 daniel.l@hpcnt.com Replace several NSAssert with FML_CHECK/DCHECK to unblock the build (flutter/engine#53048) 2024-05-30 30870216+gaaclarke@users.noreply.github.com [Impeller] make sure buffers are 4 aligned for foreground color blending (flutter/engine#53077) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC jacksongardner@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: 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

github-actions bot added affects: desktop platform-macos labels May 26, 2024

bc-lee mentioned this pull request May 26, 2024

[Darwin] Disable NSAsserts in release builds. flutter/buildroot#860

Merged

8 tasks

jmagman reviewed May 28, 2024

View reviewed changes

Remove NSAssert which should be replaced with FML_DCHECK

cdbd40e

I forgot to delete the line with NSAssert in the previous commit :)

jmagman reviewed May 29, 2024

View reviewed changes

Use FML_CHECK instead of FML_DCHECK to work around clang-tidy warnings

2e3cef5

bc-lee changed the title ~~Replace several NSAssert with FML_DCHECK to unblock the build~~ Replace several NSAssert with FML_CHECK to unblock the build May 29, 2024

bc-lee changed the title ~~Replace several NSAssert with FML_CHECK to unblock the build~~ Replace several NSAssert with FML_CHECK/DCHECK to unblock the build May 29, 2024

jmagman approved these changes May 30, 2024

View reviewed changes

jmagman requested a review from cbracken May 30, 2024 20:39

cbracken approved these changes May 30, 2024

View reviewed changes

jmagman added the autosubmit Merge PR when tree becomes green via auto submit App label May 30, 2024

auto-submit bot merged commit 3c7a5c6 into flutter:main May 30, 2024
28 checks passed

jmagman mentioned this pull request May 30, 2024

Roll buildroot to pick up disable NSAsserts in release builds #53005

Closed

8 tasks

engine-flutter-autoroll mentioned this pull request May 30, 2024

Roll Flutter Engine from bf2e32dfd5a2 to 2879ce28a900 (3 revisions) flutter/flutter#149365

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 30, 2024

3c7a5c6b5 Replace several NSAssert with FML_CHECK/DCHECK to unblock t…

e50d936

…he build (flutter/engine#53048)

bc-lee deleted the feature/fix-buildroot-roll branch May 30, 2024 23:07

engine-flutter-autoroll mentioned this pull request May 30, 2024

Roll Flutter Engine from bf2e32dfd5a2 to 4956e98ab4cc (4 revisions) flutter/flutter#149367

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 30, 2024

3c7a5c6b5 Replace several NSAssert with FML_CHECK/DCHECK to unblock t…

2ebc636

…he build (flutter/engine#53048)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace several NSAssert with FML_CHECK/DCHECK to unblock the build #53048

Replace several NSAssert with FML_CHECK/DCHECK to unblock the build #53048

bc-lee commented May 26, 2024 •

edited

Loading

jmagman May 28, 2024

bc-lee May 28, 2024

jmagman May 28, 2024

bc-lee May 29, 2024

jmagman May 29, 2024

bc-lee May 29, 2024

bc-lee May 29, 2024

jmagman left a comment

cbracken left a comment

		NSAssert(status == kCVReturnSuccess && pxbuffer != NULL, @"Failed to create pixel buffer.");
		FML_DCHECK(status == kCVReturnSuccess && pxbuffer != nullptr) << "Failed to create pixel buffer";

Replace several NSAssert with FML_CHECK/DCHECK to unblock the build #53048

Replace several NSAssert with FML_CHECK/DCHECK to unblock the build #53048

Conversation

bc-lee commented May 26, 2024 • edited Loading

Pre-launch Checklist

jmagman May 28, 2024

Choose a reason for hiding this comment

bc-lee May 28, 2024

Choose a reason for hiding this comment

jmagman May 28, 2024

Choose a reason for hiding this comment

bc-lee May 29, 2024

Choose a reason for hiding this comment

jmagman May 29, 2024

Choose a reason for hiding this comment

bc-lee May 29, 2024

Choose a reason for hiding this comment

bc-lee May 29, 2024

Choose a reason for hiding this comment

jmagman left a comment

Choose a reason for hiding this comment

cbracken left a comment

Choose a reason for hiding this comment

bc-lee commented May 26, 2024 •

edited

Loading