Skip to content
This repository was archived by the owner on Apr 21, 2025. It is now read-only.

[Darwin] Disable NSAsserts in release builds. #860

Merged
merged 1 commit into from
May 23, 2024
Merged

[Darwin] Disable NSAsserts in release builds. #860

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions build/config/BUILD.gn
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,11 @@ config("debug") {
config("release") {
defines = [ "NDEBUG" ]

if (is_mac || is_ios) {
# Disable NSAsserts in release builds.
defines += [ "NS_BLOCK_ASSERTIONS=1" ]
}
Comment on lines +87 to +90
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@cbracken cbracken May 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@jmagman jmagman May 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


# Sanitizers.
# TODO(GYP) The GYP build has "release_valgrind_build == 0" for this
# condition. When Valgrind is set up, we need to do the same here.
Expand Down