From 218434955533afede08d9efb17fd4a5a7ef56ade Mon Sep 17 00:00:00 2001 From: Dan Albert Date: Fri, 13 Jan 2023 13:27:27 -0800 Subject: [PATCH] Do not pass --gc-sections for debug builds. The user may want to call "unused" functions during debugging. This fix does not cover the new cmake toolchain. afaict CMake does not give us the control needed to fix this bug without entirely reverting the feature in that mode (we can control linker flags, but not per mode). Bug: https://github.com/android/ndk/issues/1813 Test: expanded the gc-sections test Change-Id: I095e2e70f522f4dbd5021f0efab49dfd86cfba51 --- build/cmake/android-legacy.toolchain.cmake | 25 ++++++++++++-- build/cmake/flags.cmake | 3 ++ build/core/default-build-commands.mk | 6 ++-- docs/changelogs/Changelog-r25.md | 10 ++++++ ndk/testing/flag_verifier.py | 10 ++++-- tests/build/gc_sections/test.py | 40 ++++++++++++++++++++-- tests/build/gc_sections/test_config.py | 8 +++++ 7 files changed, 93 insertions(+), 9 deletions(-) create mode 100644 tests/build/gc_sections/test_config.py diff --git a/build/cmake/android-legacy.toolchain.cmake b/build/cmake/android-legacy.toolchain.cmake index 1c0ff729..d7cda9ed 100644 --- a/build/cmake/android-legacy.toolchain.cmake +++ b/build/cmake/android-legacy.toolchain.cmake @@ -332,6 +332,9 @@ set(ANDROID_COMPILER_FLAGS_DEBUG) set(ANDROID_COMPILER_FLAGS_RELEASE) set(ANDROID_LINKER_FLAGS) set(ANDROID_LINKER_FLAGS_EXE) +set(ANDROID_LINKER_FLAGS_RELEASE) +set(ANDROID_LINKER_FLAGS_RELWITHDEBINFO) +set(ANDROID_LINKER_FLAGS_MINSIZEREL) # STL. set(ANDROID_CXX_STANDARD_LIBRARIES) @@ -347,7 +350,7 @@ elseif(ANDROID_STL STREQUAL none) list(APPEND ANDROID_COMPILER_FLAGS_CXX "-nostdinc++") list(APPEND ANDROID_LINKER_FLAGS "-nostdlib++") else() - message(FATAL_ERROR "Invalid Android STL: ${ANDROID_STL}.") + message(FATAL_ERROR "Invalid STL: ${ANDROID_STL}.") endif() if(CMAKE_HOST_SYSTEM_NAME STREQUAL Linux) @@ -459,8 +462,12 @@ if (NOT ANDROID_ALLOW_UNDEFINED_VERSION_SCRIPT_SYMBOLS) endif() list(APPEND ANDROID_LINKER_FLAGS -Wl,--fatal-warnings) -list(APPEND ANDROID_LINKER_FLAGS -Wl,--gc-sections) -list(APPEND ANDROID_LINKER_FLAGS_EXE -Wl,--gc-sections) + +# --gc-sections should not be present for debug builds because that can strip +# functions that the user may want to evaluate while debugging. +list(APPEND ANDROID_LINKER_FLAGS_RELEASE -Wl,--gc-sections) +list(APPEND ANDROID_LINKER_FLAGS_RELWITHDEBINFO -Wl,--gc-sections) +list(APPEND ANDROID_LINKER_FLAGS_MINSIZEREL -Wl,--gc-sections) # Debug and release flags. list(APPEND ANDROID_COMPILER_FLAGS_RELEASE -O3) @@ -544,6 +551,9 @@ string(REPLACE ";" " " ANDROID_COMPILER_FLAGS_DEBUG "${ANDROID_COMPILER_FLAGS_ string(REPLACE ";" " " ANDROID_COMPILER_FLAGS_RELEASE "${ANDROID_COMPILER_FLAGS_RELEASE}") string(REPLACE ";" " " ANDROID_LINKER_FLAGS "${ANDROID_LINKER_FLAGS}") string(REPLACE ";" " " ANDROID_LINKER_FLAGS_EXE "${ANDROID_LINKER_FLAGS_EXE}") +string(REPLACE ";" " " ANDROID_LINKER_FLAGS_RELEASE "${ANDROID_LINKER_FLAGS_RELEASE}") +string(REPLACE ";" " " ANDROID_LINKER_FLAGS_RELWITHDEBINFO "${ANDROID_LINKER_FLAGS_RELWITHDEBINFO}") +string(REPLACE ";" " " ANDROID_LINKER_FLAGS_MINSIZEREL "${ANDROID_LINKER_FLAGS_MINSIZEREL}") if(ANDROID_CCACHE) set(CMAKE_C_COMPILER_LAUNCHER "${ANDROID_CCACHE}") @@ -603,6 +613,15 @@ set(CMAKE_ASM_FLAGS_RELEASE "${ANDROID_COMPILER_FLAGS_RELEASE} ${CMAKE_ASM_FLA set(CMAKE_SHARED_LINKER_FLAGS "${ANDROID_LINKER_FLAGS} ${CMAKE_SHARED_LINKER_FLAGS}") set(CMAKE_MODULE_LINKER_FLAGS "${ANDROID_LINKER_FLAGS} ${CMAKE_MODULE_LINKER_FLAGS}") set(CMAKE_EXE_LINKER_FLAGS "${ANDROID_LINKER_FLAGS} ${ANDROID_LINKER_FLAGS_EXE} ${CMAKE_EXE_LINKER_FLAGS}") +set(CMAKE_SHARED_LINKER_FLAGS_RELEASE "${ANDROID_LINKER_FLAGS_RELEASE} ${CMAKE_SHARED_LINKER_FLAGS_RELEASE}") +set(CMAKE_MODULE_LINKER_FLAGS_RELEASE "${ANDROID_LINKER_FLAGS_RELEASE} ${CMAKE_MODULE_LINKER_FLAGS_RELEASE}") +set(CMAKE_EXE_LINKER_FLAGS_RELEASE "${ANDROID_LINKER_FLAGS_RELEASE} ${CMAKE_EXE_LINKER_FLAGS_RELEASE}") +set(CMAKE_SHARED_LINKER_FLAGS_RELWITHDEBINFO "${ANDROID_LINKER_FLAGS_RELWITHDEBINFO} ${CMAKE_SHARED_LINKER_FLAGS_RELWITHDEBINFO}") +set(CMAKE_MODULE_LINKER_FLAGS_RELWITHDEBINFO "${ANDROID_LINKER_FLAGS_RELWITHDEBINFO} ${CMAKE_MODULE_LINKER_FLAGS_RELWITHDEBINFO}") +set(CMAKE_EXE_LINKER_FLAGS_RELWITHDEBINFO "${ANDROID_LINKER_FLAGS_RELWITHDEBINFO} ${CMAKE_EXE_LINKER_FLAGS_RELWITHDEBINFO}") +set(CMAKE_SHARED_LINKER_FLAGS_MINSIZEREL "${ANDROID_LINKER_FLAGS_MINSIZEREL} ${CMAKE_SHARED_LINKER_FLAGS_MINSIZEREL}") +set(CMAKE_MODULE_LINKER_FLAGS_MINSIZEREL "${ANDROID_LINKER_FLAGS_MINSIZEREL} ${CMAKE_MODULE_LINKER_FLAGS_MINSIZEREL}") +set(CMAKE_EXE_LINKER_FLAGS_MINSIZEREL "${ANDROID_LINKER_FLAGS_MINSIZEREL} ${CMAKE_EXE_LINKER_FLAGS_MINSIZEREL}") # Compatibility for read-only variables. # Read-only variables for compatibility with the other toolchain file. diff --git a/build/cmake/flags.cmake b/build/cmake/flags.cmake index ba3ed5fa..0f6a3063 100644 --- a/build/cmake/flags.cmake +++ b/build/cmake/flags.cmake @@ -55,6 +55,9 @@ if (NOT ANDROID_ALLOW_UNDEFINED_VERSION_SCRIPT_SYMBOLS) endif() string(APPEND _ANDROID_NDK_INIT_LDFLAGS " -Wl,--fatal-warnings") +# This should only be set for release modes, but CMake doesn't provide a way for +# us to be that specific in the new toolchain file. +# https://github.com/android/ndk/issues/1813 string(APPEND _ANDROID_NDK_INIT_LDFLAGS " -Wl,--gc-sections") string(APPEND _ANDROID_NDK_INIT_LDFLAGS_EXE " -Wl,--gc-sections") diff --git a/build/core/default-build-commands.mk b/build/core/default-build-commands.mk index 6a4e00db..bed97417 100644 --- a/build/core/default-build-commands.mk +++ b/build/core/default-build-commands.mk @@ -43,7 +43,6 @@ TARGET_DISABLE_FORMAT_STRING_CFLAGS := -Wno-error=format-security define cmd-build-shared-library $(PRIVATE_CXX) \ - -Wl,--gc-sections \ -Wl,-soname,$(notdir $(LOCAL_BUILT_MODULE)) \ -shared \ $(PRIVATE_LINKER_OBJECTS_AND_LIBRARIES) \ @@ -59,7 +58,6 @@ endef # this buggy behavior. define cmd-build-executable $(PRIVATE_CXX) \ - -Wl,--gc-sections \ -Wl,-rpath-link=$(call host-path,$(PRIVATE_SYSROOT_API_LIB_DIR)) \ -Wl,-rpath-link=$(call host-path,$(TARGET_OUT)) \ $(PRIVATE_LINKER_OBJECTS_AND_LIBRARIES) \ @@ -128,6 +126,10 @@ GLOBAL_LDFLAGS = \ -target $(LLVM_TRIPLE)$(TARGET_PLATFORM_LEVEL) \ -no-canonical-prefixes \ +ifeq ($(APP_OPTIM),release) + GLOBAL_LDFLAGS += -Wl,--gc-sections +endif + GLOBAL_CXXFLAGS = $(GLOBAL_CFLAGS) -fno-exceptions -fno-rtti TARGET_CFLAGS = diff --git a/docs/changelogs/Changelog-r25.md b/docs/changelogs/Changelog-r25.md index ef0c538e..46387342 100644 --- a/docs/changelogs/Changelog-r25.md +++ b/docs/changelogs/Changelog-r25.md @@ -20,6 +20,16 @@ directly, see the [build system maintainers guide]. [Issue 1751]: https://github.com/android/ndk/issues/1751 +## r25c + +* [Issue 1813]: `-Wl,--gc-sections` is no longer set by default for debug + builds. This behavior was removed because it could cause the linker to remove + functions that may be useful to evaluate during debugging. The new CMake + toolchain file (`-DANDROID_USE_LEGACY_TOOLCHAIN_FILE=OFF`, not the default + behavior) does not include this fix because it requires a CMake fix first. + +[Issue 1813]: https://github.com/android/ndk/issues/1813 + ## r25b * [Issue 1739]: Fixed C compatibility issue in `amidi/AMidi.h`. diff --git a/ndk/testing/flag_verifier.py b/ndk/testing/flag_verifier.py index 12615f7e..3a99b928 100644 --- a/ndk/testing/flag_verifier.py +++ b/ndk/testing/flag_verifier.py @@ -36,9 +36,15 @@ def failed(self) -> bool: """Returns True if verification failed.""" raise NotImplementedError - def make_test_result_tuple(self) -> tuple[bool, Optional[str]]: + def make_test_result_tuple( + self, message_prefix: str | None = None + ) -> tuple[bool, Optional[str]]: """Creates a test result tuple in the format expect by run_test.""" - return not self.failed(), self.error_message + if message_prefix is None: + message = self.error_message + else: + message = f"{message_prefix}\n{self.error_message}" + return not self.failed(), message class FlagVerifierSuccess(FlagVerifierResult): diff --git a/tests/build/gc_sections/test.py b/tests/build/gc_sections/test.py index 419cae12..8bb436a5 100644 --- a/tests/build/gc_sections/test.py +++ b/tests/build/gc_sections/test.py @@ -13,9 +13,13 @@ # See the License for the specific language governing permissions and # limitations under the License. # -"""Check that -Wl,--gc-sections is used. +"""Check that -Wl,--gc-sections is used, but only on release builds. + +This flag should not be present for debug builds because that can strip functions that +the user may want to evaluate while debugging. https://github.com/android/ndk/issues/1717 +https://github.com/android/ndk/issues/1813 """ from pathlib import Path from typing import Optional @@ -27,5 +31,37 @@ def run_test(ndk_path: str, config: BuildConfiguration) -> tuple[bool, Optional[str]]: """Checks correct --gc-sections use.""" verifier = FlagVerifier(Path("project"), Path(ndk_path), config) + verifier.with_cmake_flag("-DCMAKE_BUILD_TYPE=Release") + verifier.with_ndk_build_flag("APP_DEBUG=false") verifier.expect_flag("-Wl,--gc-sections") - return verifier.verify().make_test_result_tuple() + passed, message = verifier.verify().make_test_result_tuple( + "With -DCMAKE_BUILD_TYPE=Release and APP_DEBUG=false" + ) + if not passed: + return passed, message + + verifier = FlagVerifier(Path("project"), Path(ndk_path), config) + verifier.with_cmake_flag("-DCMAKE_BUILD_TYPE=RelWithDebInfo") + verifier.expect_flag("-Wl,--gc-sections") + passed, message = verifier.verify_cmake().make_test_result_tuple( + "With -DCMAKE_BUILD_TYPE=RelWithDebInfo" + ) + if not passed: + return passed, message + + verifier = FlagVerifier(Path("project"), Path(ndk_path), config) + verifier.with_cmake_flag("-DCMAKE_BUILD_TYPE=MinSizeRel") + verifier.expect_flag("-Wl,--gc-sections") + passed, message = verifier.verify_cmake().make_test_result_tuple( + "With -DCMAKE_BUILD_TYPE=MinSizeRel" + ) + if not passed: + return passed, message + + verifier = FlagVerifier(Path("project"), Path(ndk_path), config) + verifier.with_cmake_flag("-DCMAKE_BUILD_TYPE=Debug") + verifier.with_ndk_build_flag("APP_DEBUG=true") + verifier.expect_not_flag("-Wl,--gc-sections") + return verifier.verify().make_test_result_tuple( + "With -DCMAKE_BUILD_TYPE=Debug and APP_DEBUG=true" + ) diff --git a/tests/build/gc_sections/test_config.py b/tests/build/gc_sections/test_config.py new file mode 100644 index 00000000..869627d3 --- /dev/null +++ b/tests/build/gc_sections/test_config.py @@ -0,0 +1,8 @@ +from ndk.test.buildtest.case import Test +from ndk.test.spec import CMakeToolchainFile + + +def build_broken(test: Test) -> tuple[str | None, str | None]: + if test.config.toolchain_file is CMakeToolchainFile.Default: + return "new CMake toolchain", "https://github.com/android/ndk/issues/1813" + return None, None