[Darwin] Disable NSAsserts in release builds. #860

bc-lee · 2024-05-17T22:10:23Z

Chromium has a similar gn flag to disable asserts in release builds: https://source.chromium.org/chromium/chromium/src/+/main:build/config/BUILD.gn;drc=9dab28144192cefadbb96b778ef866ea3deb74ff;l=148

Related: flutter/flutter#148279

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 the Flutter Style Guide recently, and have followed its advice.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
All existing and new tests are passing.

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

Chromium has a similar gn flag to disable asserts in release builds: https://source.chromium.org/chromium/chromium/src/+/main:build/config/BUILD.gn;drc=9dab28144192cefadbb96b778ef866ea3deb74ff;l=148 Related: flutter/flutter#148279

Hixie · 2024-05-18T01:10:57Z

test-exempt: configuration change

jmagman · 2024-05-20T20:28:59Z

build/config/BUILD.gn

+  if (is_mac || is_ios) {
+    # Disable NSAsserts in release builds.
+    defines += [ "NS_BLOCK_ASSERTIONS=1" ]
+  }


I'm really surprised we didn't already have this on...
@cbracken or @chinmaygarde are you aware of any reason why we wouldn't want to disable NSAssert in release?

Not really, we don't use foundation assertions.

If the assertion needs to be fatal in all modes, there is FML_CHECK. If its a useful signal while developing the engine, there is FML_DCHECK. There are vanishingly few cases where the algorithmic complexity of an operation changes based on the either mode so it mostly just to indicate "actionable to end-users" vs "actionable to engine-developers". If its actionable to neither, there is typically no assertion.

The contributing factor for this suppression seems to be here. I think that code handles the case via an early return.

It depends on whether the macOS/iOS embedder authors expect assertions to be blocked in release modes and whether this is a good assertion to have. At least to my reading, it looks like no one understands how we got into this situation and so we are disabling the assertion. But the ability to reason about the severity of ignoring the assertion and whether ignoring it will cause issues further down the line is perhaps lost now.

Agreed with @chinmaygarde. I don't think we'd left them enabled intentionally, but rather I think mostly assumed we were using the FML macros.

To be honest, I thought I had gone through and replaced any I found with FML macros at least in the macOS embedder, but maybe that was just cleaning up Windows assertions (e.g. flutter/engine#38826), and just imagined I'd also got around to it for macOS.

We should probably come to a decision as to whether we want to standardise on one other the other so we don't have a semi-random mix of the two. Personal preference would be to standardise on FML across the board both for consistency and because they provide more indication of intent, but either way is fine.

Happy to send a cleanup patch for the macOS embedder and common code. Happy to do iOS too, but am super familiar with the macOS code and it should be straightforward to reason about whether any should actually be FML_CHECK.

I think standardizing on the macros makes senses, though unless we have a lint forbidding NSAsserts and friends, they will sneak in via reviewers not being aware its disallowed.
NSAssert
NSCAssert

I vote we should turn this on to match the Xcode default (ENABLE_NS_ASSERTIONS set to NO in release) of having NSAsserts fire in debug but not in-the-wild production apps. But that wouldn't prevent us from auditing the embedder code to swap to the FML macros.

jmagman

cc @knopp

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.

bc-lee · 2024-05-26T07:58:00Z

While NSAssert appears consistent with the rest of the Objective-C codebase, FML_DCHECK offers two key advantages:

FML_DCHECK aligns more closely with the engine codebase's conventions.
FML_DCHECK doesn't trigger unused variable warnings in release builds, unlike NSAssert.

The second part is explained in the another PR ( flutter/engine#53048 ). Also, I hope that the linked PR will be reviewed.

jmagman · 2024-05-28T23:36:28Z

Thanks for looking into this, @bc-lee!

bc-lee · 2024-05-29T19:32:13Z

FML_DCHECK doesn't trigger unused variable warnings in release builds, unlike NSAssert.

It turns out the second advantage is not entirely true. While it does not generate compile-time warnings, Flutter uses clang-tidy's clang-analyzer-deadcode.DeadStores (or simply deadcode.DeadStores) to identify unused variables. Although FML_DCHECK employs smart ways to avoid compile-time warnings, clang-tidy is too smart to be fooled by it. Since neither Chromium nor abseil uses clang-tidy to check for dead stores, they don't encounter this issue.

zanderso · 2024-05-30T20:19:42Z

cc @chinmaygarde

…53048) 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: ```c++ FML_DCHECK(status == kCVReturnSuccess && pxbuffer != nullptr) << "Failed to create pixel buffer"; ``` expanded code: ```c++ 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: ```c++ NSAssert(status == kCVReturnSuccess && pxbuffer != NULL, @"Failed to create pixel buffer."); ``` expanded code: ```c++ 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. [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

auto-submit · 2024-05-30T22:57:47Z

Time to revert pull request flutter/buildroot/860 has elapsed.
You need to open the revert manually and process as a regular pull request.

This reverts commit 5b504e1.

#860 isn't cleanly rolling into the engine due to side effects of turning off NSAsserts in release. Revert until the fixes are in the engine.

cbenhagen · 2024-10-29T13:07:45Z

@bc-lee do you intend to re-land this or is there another fix for flutter/flutter#148279?

[Darwin] Disable NSAsserts in release builds.

88d344f

Chromium has a similar gn flag to disable asserts in release builds: https://source.chromium.org/chromium/chromium/src/+/main:build/config/BUILD.gn;drc=9dab28144192cefadbb96b778ef866ea3deb74ff;l=148 Related: flutter/flutter#148279

bc-lee force-pushed the feature/disable-nsassert branch from 1fac750 to 88d344f Compare May 17, 2024 22:11

jmagman reviewed May 20, 2024

View reviewed changes

chinmaygarde approved these changes May 23, 2024

View reviewed changes

jmagman approved these changes May 23, 2024

View reviewed changes

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

auto-submit bot merged commit 5b504e1 into flutter:master May 23, 2024

jmagman mentioned this pull request May 23, 2024

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

Closed

8 tasks

bc-lee deleted the feature/disable-nsassert branch May 23, 2024 22:09

bc-lee mentioned this pull request May 26, 2024

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

Merged

8 tasks

jmagman added the revert Label used to revert changes in a closed and merged pull request. label May 30, 2024

auto-submit bot removed the revert Label used to revert changes in a closed and merged pull request. label May 30, 2024

jmagman added a commit to jmagman/buildroot that referenced this pull request May 30, 2024

Revert "[Darwin] Disable NSAsserts in release builds. (flutter#860)"

501bce6

This reverts commit 5b504e1.

jmagman mentioned this pull request May 30, 2024

Revert "[Darwin] Disable NSAsserts in release builds." #864

Merged

auto-submit bot pushed a commit that referenced this pull request May 31, 2024

Revert "[Darwin] Disable NSAsserts in release builds." (#864)

0576d6d

#860 isn't cleanly rolling into the engine due to side effects of turning off NSAsserts in release. Revert until the fixes are in the engine.

jmagman mentioned this pull request Oct 30, 2024

Turn off NSAsserts in release mode on iOS and macOS flutter/flutter#157837

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Darwin] Disable NSAsserts in release builds. #860

[Darwin] Disable NSAsserts in release builds. #860

Uh oh!

bc-lee commented May 17, 2024 •

edited

Loading

Uh oh!

Hixie commented May 18, 2024

Uh oh!

jmagman May 20, 2024

Uh oh!

chinmaygarde May 20, 2024

Uh oh!

cbracken May 20, 2024 •

edited

Loading

Uh oh!

jmagman May 20, 2024 •

edited

Loading

Uh oh!

jmagman left a comment

Uh oh!

bc-lee commented May 26, 2024

Uh oh!

jmagman commented May 28, 2024

Uh oh!

bc-lee commented May 29, 2024

Uh oh!

zanderso commented May 30, 2024

Uh oh!

auto-submit bot commented May 30, 2024

Uh oh!

cbenhagen commented Oct 29, 2024

Uh oh!

Uh oh!

[Darwin] Disable NSAsserts in release builds. #860

[Darwin] Disable NSAsserts in release builds. #860

Uh oh!

Conversation

bc-lee commented May 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

Hixie commented May 18, 2024

Uh oh!

jmagman May 20, 2024

Choose a reason for hiding this comment

Uh oh!

chinmaygarde May 20, 2024

Choose a reason for hiding this comment

Uh oh!

cbracken May 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmagman May 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmagman left a comment

Choose a reason for hiding this comment

Uh oh!

bc-lee commented May 26, 2024

Uh oh!

jmagman commented May 28, 2024

Uh oh!

bc-lee commented May 29, 2024

Uh oh!

zanderso commented May 30, 2024

Uh oh!

auto-submit bot commented May 30, 2024

Uh oh!

cbenhagen commented Oct 29, 2024

Uh oh!

Uh oh!

bc-lee commented May 17, 2024 •

edited

Loading

cbracken May 20, 2024 •

edited

Loading

jmagman May 20, 2024 •

edited

Loading