From ec4833f06dc58cb439feac8ee753c7b60320d8e2 Mon Sep 17 00:00:00 2001 From: Joshua Gross Date: Wed, 17 Feb 2021 17:57:43 -0800 Subject: [PATCH] Introduce RN_DEBUG flag and rn_assert for Cxx Summary: BUCK always defines NDEBUG on Android builds. This is a longstanding issue and it's tricky to work around. Previous attempts to fix this within React Native were difficult because disabling NDEBUG caused lots of issues that were difficult to track down. Instead, I am (1) introducing a new RN_DEBUG flag that can be used cross-platform, (2) whenever NDEBUG is *not* enabled, RN_DEBUG will automatically be defined, (3) enables debug-only code to be compiled on Android, (4) enables us to selectively, slowly migrate `assert` to `rn_assert` in a way that doesn't impact non-Android platforms, but allows us to maintain stability of Android debug builds. Actually enabling the RN_DEBUG flag in debug builds is done in FB-internal code. I assume the NDEBUG issue is not a problem when compiling in open-source without BUCK. Changelog: [Internal] Reviewed By: mdvacca Differential Revision: D26409355 fbshipit-source-id: 285b8073bba3756834925727bfa28d3c6bc06335 --- ReactCommon/React-Fabric.podspec | 11 +++- ReactCommon/react/debug/Android.mk | 27 ++++++++++ ReactCommon/react/debug/BUCK | 67 ++++++++++++++++++++++++ ReactCommon/react/debug/flags.h | 19 +++++++ ReactCommon/react/debug/rn_assert.cpp | 39 ++++++++++++++ ReactCommon/react/debug/rn_assert.h | 50 ++++++++++++++++++ ReactCommon/react/renderer/debug/BUCK | 11 ++++ ReactCommon/react/renderer/debug/flags.h | 2 + packages/rn-tester/Podfile.lock | 15 ++++-- 9 files changed, 237 insertions(+), 4 deletions(-) create mode 100644 ReactCommon/react/debug/Android.mk create mode 100644 ReactCommon/react/debug/BUCK create mode 100644 ReactCommon/react/debug/flags.h create mode 100644 ReactCommon/react/debug/rn_assert.cpp create mode 100644 ReactCommon/react/debug/rn_assert.h diff --git a/ReactCommon/React-Fabric.podspec b/ReactCommon/React-Fabric.podspec index 808158d0a3c..5a1d92d9d1a 100644 --- a/ReactCommon/React-Fabric.podspec +++ b/ReactCommon/React-Fabric.podspec @@ -232,7 +232,16 @@ Pod::Spec.new do |s| end end - s.subspec "debug" do |ss| + s.subspec "debug_core" do |ss| + ss.dependency folly_dep_name, folly_version + ss.compiler_flags = folly_compiler_flags + ss.source_files = "react/debug/**/*.{m,mm,cpp,h}" + ss.exclude_files = "react/debug/tests" + ss.header_dir = "react/debug" + ss.pod_target_xcconfig = { "HEADER_SEARCH_PATHS" => "\"$(PODS_TARGET_SRCROOT)/ReactCommon\" \"$(PODS_ROOT)/RCT-Folly\"" } + end + + s.subspec "debug_renderer" do |ss| ss.dependency folly_dep_name, folly_version ss.compiler_flags = folly_compiler_flags ss.source_files = "react/renderer/debug/**/*.{m,mm,cpp,h}" diff --git a/ReactCommon/react/debug/Android.mk b/ReactCommon/react/debug/Android.mk new file mode 100644 index 00000000000..fed711337bf --- /dev/null +++ b/ReactCommon/react/debug/Android.mk @@ -0,0 +1,27 @@ +# Copyright (c) Facebook, Inc. and its affiliates. +# +# This source code is licensed under the MIT license found in the +# LICENSE file in the root directory of this source tree. + +LOCAL_PATH := $(call my-dir) + +include $(CLEAR_VARS) + +LOCAL_MODULE := react_debug + +LOCAL_C_INCLUDES := $(LOCAL_PATH)/../../../ + +LOCAL_SRC_FILES := $(wildcard $(LOCAL_PATH)/*.cpp) + +LOCAL_EXPORT_C_INCLUDES := $(LOCAL_PATH)/../../../ + +LOCAL_SHARED_LIBRARIES := libfolly_json + +LOCAL_CFLAGS := \ + -DLOG_TAG=\"Fabric\" + +LOCAL_CFLAGS += -fexceptions -frtti -std=c++14 -Wall -llog + +include $(BUILD_SHARED_LIBRARY) + +$(call import-module,folly) diff --git a/ReactCommon/react/debug/BUCK b/ReactCommon/react/debug/BUCK new file mode 100644 index 00000000000..5f1816da378 --- /dev/null +++ b/ReactCommon/react/debug/BUCK @@ -0,0 +1,67 @@ +load( + "//tools/build_defs/oss:rn_defs.bzl", + "ANDROID", + "APPLE", + "CXX", + "get_apple_compiler_flags", + "get_apple_inspector_flags", + "get_preprocessor_flags_for_build_mode", + "rn_xplat_cxx_library", + "subdir_glob", +) + +APPLE_COMPILER_FLAGS = get_apple_compiler_flags() + +rn_xplat_cxx_library( + name = "debug", + srcs = glob( + ["**/*.cpp"], + exclude = glob(["tests/**/*.cpp"]), + ), + headers = glob( + ["**/*.h"], + exclude = glob(["tests/**/*.h"]), + ), + header_namespace = "", + exported_headers = subdir_glob( + [ + ("", "*.h"), + ], + prefix = "react/debug", + ), + compiler_flags = [ + "-fexceptions", + "-frtti", + "-std=c++14", + "-Wall", + ], + exported_platform_linker_flags = [ + ( + "^android.*", + ["-llog"], + ), + ], + fbandroid_linker_flags = [ + # for android rn_assert + "-llog", + ], + fbobjc_compiler_flags = APPLE_COMPILER_FLAGS, + fbobjc_preprocessor_flags = get_preprocessor_flags_for_build_mode() + get_apple_inspector_flags(), + force_static = True, + labels = ["supermodule:xplat/default/public.react_native.infra"], + macosx_tests_override = [], + platforms = (ANDROID, APPLE, CXX), + preprocessor_flags = [ + "-DLOG_TAG=\"ReactNative\"", + "-DWITH_FBSYSTRACE=1", + ], + tests = [], + visibility = ["PUBLIC"], + deps = [ + "//third-party/glog:glog", + "//xplat/fbsystrace:fbsystrace", + "//xplat/folly:headers_only", + "//xplat/folly:memory", + "//xplat/folly:molly", + ], +) diff --git a/ReactCommon/react/debug/flags.h b/ReactCommon/react/debug/flags.h new file mode 100644 index 00000000000..906847d8e47 --- /dev/null +++ b/ReactCommon/react/debug/flags.h @@ -0,0 +1,19 @@ +/* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +#pragma once + +// +// Enable RN_DEBUG if NDEBUG is not defined. +// Due to BUCK defaults in open-source, NDEBUG is always defined for all android +// builds (if you build without BUCK, this isn't an issue). Thus we introduce +// RN_DEBUG that we use internally instead of NDEBUG that we can control and use +// as a more reliable xplat flag. For any build that doesn't have NDEBUG +// defined, we enable RN_DEBUG for convenience. +#ifndef NDEBUG +#define RN_DEBUG 1 +#endif diff --git a/ReactCommon/react/debug/rn_assert.cpp b/ReactCommon/react/debug/rn_assert.cpp new file mode 100644 index 00000000000..f0a7c40b1d2 --- /dev/null +++ b/ReactCommon/react/debug/rn_assert.cpp @@ -0,0 +1,39 @@ +/* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +#ifdef __ANDROID__ + +#include + +// Provide a prototype to silence missing prototype warning in release +// mode. +extern "C" void +rn_assert_fail(const char *func, const char *file, int line, const char *expr); + +extern "C" void +rn_assert_fail(const char *func, const char *file, int line, const char *expr) { + // Print as an error so it shows up in logcat before crash.... + __android_log_print( + ANDROID_LOG_ERROR, + "ReactNative", + "%s:%d: function %s: assertion failed (%s)", + file, + line, + func, + expr); + // Print as a fatal so it crashes and shows up in uploaded logs + __android_log_print( + ANDROID_LOG_FATAL, + "ReactNative", + "%s:%d: function %s: assertion failed (%s)", + file, + line, + func, + expr); +} + +#endif // __ANDROID__ diff --git a/ReactCommon/react/debug/rn_assert.h b/ReactCommon/react/debug/rn_assert.h new file mode 100644 index 00000000000..8f6642c0ff5 --- /dev/null +++ b/ReactCommon/react/debug/rn_assert.h @@ -0,0 +1,50 @@ +/* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +// No header guards since it is legitimately possible to include this file more +// than once with and without RN_DEBUG. + +// rn_assert allows us to opt-in to specific asserts on Android and test before +// moving on. When all issues have been found, maybe we can use `UNDEBUG` flag +// to disable NDEBUG in debug builds on Android. + +#include "flags.h" + +#undef rn_assert + +#ifndef RN_DEBUG + +#define rn_assert(e) ((void)0) + +#else // RN_DEBUG + +#ifdef __ANDROID__ + +#include + +#ifdef __cplusplus +extern "C" { +#endif // __cplusplus +void rn_assert_fail( + const char *func, + const char *file, + int line, + const char *expr); +#ifdef __cplusplus +} +#endif // __cpusplus + +#define rn_assert(e) \ + ((e) ? (void)0 : rn_assert_fail(__func__, __FILE__, __LINE__, #e)) + +#else // __ANDROID__ + +#define rn_assert(e) assert(e) + +#endif // platforms besides __ANDROID__ + +#endif // NDEBUG diff --git a/ReactCommon/react/renderer/debug/BUCK b/ReactCommon/react/renderer/debug/BUCK index c52386c07db..ffed0b6af41 100644 --- a/ReactCommon/react/renderer/debug/BUCK +++ b/ReactCommon/react/renderer/debug/BUCK @@ -37,6 +37,16 @@ rn_xplat_cxx_library( "-std=c++14", "-Wall", ], + exported_platform_linker_flags = [ + ( + "^android.*", + ["-llog"], + ), + ], + fbandroid_linker_flags = [ + # for android rn_assert + "-llog", + ], fbobjc_compiler_flags = APPLE_COMPILER_FLAGS, fbobjc_preprocessor_flags = get_preprocessor_flags_for_build_mode() + get_apple_inspector_flags(), force_static = True, @@ -55,6 +65,7 @@ rn_xplat_cxx_library( "//xplat/folly:memory", "//xplat/folly:molly", react_native_xplat_target("better:better"), + react_native_xplat_target("react/debug:debug"), ], ) diff --git a/ReactCommon/react/renderer/debug/flags.h b/ReactCommon/react/renderer/debug/flags.h index 929f5a845b7..0edd7bfdce1 100644 --- a/ReactCommon/react/renderer/debug/flags.h +++ b/ReactCommon/react/renderer/debug/flags.h @@ -7,6 +7,8 @@ #pragma once +#include + // // This file contains flags that should __never__ be enabled for // release-mode/production builds, unless explicitly noted. You can enable some diff --git a/packages/rn-tester/Podfile.lock b/packages/rn-tester/Podfile.lock index f4e215b80b4..c6a74201064 100644 --- a/packages/rn-tester/Podfile.lock +++ b/packages/rn-tester/Podfile.lock @@ -259,7 +259,8 @@ PODS: - React-Fabric/components (= 1000.0.0) - React-Fabric/config (= 1000.0.0) - React-Fabric/core (= 1000.0.0) - - React-Fabric/debug (= 1000.0.0) + - React-Fabric/debug_core (= 1000.0.0) + - React-Fabric/debug_renderer (= 1000.0.0) - React-Fabric/imagemanager (= 1000.0.0) - React-Fabric/mounting (= 1000.0.0) - React-Fabric/scheduler (= 1000.0.0) @@ -463,7 +464,15 @@ PODS: - React-jsi (= 1000.0.0) - React-jsiexecutor (= 1000.0.0) - ReactCommon/turbomodule/core (= 1000.0.0) - - React-Fabric/debug (1000.0.0): + - React-Fabric/debug_core (1000.0.0): + - RCT-Folly/Fabric (= 2020.01.13.00) + - RCTRequired (= 1000.0.0) + - RCTTypeSafety (= 1000.0.0) + - React-graphics (= 1000.0.0) + - React-jsi (= 1000.0.0) + - React-jsiexecutor (= 1000.0.0) + - ReactCommon/turbomodule/core (= 1000.0.0) + - React-Fabric/debug_renderer (1000.0.0): - RCT-Folly/Fabric (= 2020.01.13.00) - RCTRequired (= 1000.0.0) - RCTTypeSafety (= 1000.0.0) @@ -816,7 +825,7 @@ SPEC CHECKSUMS: React-Core: c63389ffebc383f834a2cae4d1c120968ffb3cdb React-CoreModules: 87f011fa87190ffe979e443ce578ec93ec6ff4d4 React-cxxreact: 14cce64344ab482615dfe82a2cbea6eb73be6481 - React-Fabric: 1744b2e94f5ed2ab247f3a55fd9762d55ed63f3b + React-Fabric: 2e26aa017842a411a0d0f29c1f458e0e91cb3d96 React-graphics: 246b8e6cb4aad51271358767c965e47d692921ab React-jsi: 08c6628096d2025d4085fbaec8fe14a3c9dc667c React-jsiexecutor: 896c41b04121803e4ee61e4c9ed0900fdb420fea