From 2b940fce2f864370ce6acf203f6057e9581accfc Mon Sep 17 00:00:00 2001 From: Dave Roth Date: Mon, 24 Jun 2024 17:03:34 +0000 Subject: [PATCH] pw_toolchain: Closer align the bazel arm-gcc flags with GN MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - define a common set of compiler flags for gcc and clang - add the internal_strict_warnings_flags feature to match GN Change-Id: Ib62403086a941d2885df7cd2d6e45cbaab41d741 Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/215734 Reviewed-by: Armando Montanez Lint: Lint 🤖 Presubmit-Verified: CQ Bot Account Reviewed-by: Ted Pudlik Commit-Queue: Dave Roth --- .bazelrc | 6 ++ pw_tokenizer/generate_decoding_test_data.cc | 4 +- pw_toolchain/arm_gcc/BUILD.bazel | 30 ++++++--- pw_toolchain/cc/BUILD.bazel | 74 +++++++++++++++++++++ pw_toolchain/host_clang/BUILD.bazel | 21 +----- pw_toolchain_bazel/flag_sets/BUILD.bazel | 11 +++ targets/stm32f429i_disc1/boot.cc | 2 +- 7 files changed, 114 insertions(+), 34 deletions(-) create mode 100644 pw_toolchain/cc/BUILD.bazel diff --git a/.bazelrc b/.bazelrc index 1acb63f54..dac5aa566 100644 --- a/.bazelrc +++ b/.bazelrc @@ -75,6 +75,12 @@ build --repo_env=JAVA_HOME=../bazel_tools/jdk # toolchain, but not yet by Pigweed's own toolchains. build --features=external_include_paths +# Turn on internal warnings for upstream Pigweed. DO NOT enable in downstream +# projects to allow for warnings to be added in the future without breaking +# downstream. +# +build --features=internal_strict_warnings + # This feature can't be enabled until __unordtf2 and __letf2 are implemented by # compiler-rt. See https://reviews.llvm.org/D53608. # build --features=fully_static_link diff --git a/pw_tokenizer/generate_decoding_test_data.cc b/pw_tokenizer/generate_decoding_test_data.cc index 34e6dd232..7f8aece9f 100644 --- a/pw_tokenizer/generate_decoding_test_data.cc +++ b/pw_tokenizer/generate_decoding_test_data.cc @@ -384,8 +384,8 @@ void OutputVarintTest(TestDataFile* file, T i) { // All integers are encoded as signed for tokenization. size_t size = pw::varint::Encode(i, pw::as_writable_bytes(pw::span(buffer))); - for (size_t i = 0; i < size; ++i) { - file->printf("\\x%02x", buffer[i]); + for (size_t j = 0; j < size; ++j) { + file->printf("\\x%02x", buffer[j]); } file->printf("%s),\n", file->fmt().binary_string_suffix); diff --git a/pw_toolchain/arm_gcc/BUILD.bazel b/pw_toolchain/arm_gcc/BUILD.bazel index f55a6a93c..09690f5b9 100644 --- a/pw_toolchain/arm_gcc/BUILD.bazel +++ b/pw_toolchain/arm_gcc/BUILD.bazel @@ -43,9 +43,7 @@ cc_library( ], ) -# Although we use similar warnings for clang and arm_gcc, we don't have one -# centralized list, since we might want to use different warnings based on the -# compiler in the future. +# Additional arm_gcc specific warning flags pw_cc_flag_set( name = "warnings", actions = [ @@ -53,13 +51,10 @@ pw_cc_flag_set( "@pw_toolchain//actions:all_cpp_compiler_actions", ], flags = [ - "-Wall", - "-Wextra", - # Make all warnings errors, except for the exemptions below. - "-Werror", - "-Wno-error=cpp", # preprocessor #warning statement - "-Wno-error=deprecated-declarations", # [[deprecated]] attribute - "-Wno-psabi", # Silence the really verbose ARM warnings. + # This can't be in common, because proto headers in host builds trigger + "-Wundef", + # Silence the really verbose ARM warnings. + "-Wno-psabi", ], ) @@ -173,6 +168,10 @@ pw_cc_flag_set( flags = [ "-mcpu=cortex-m4", "-mfloat-abi=hard", + "-mfpu=fpv4-sp-d16", + # Used by some pigweed tests/targets to correctly handle hardware FPU + # behavior. + "-DPW_ARMV7M_ENABLE_FPU=1", ], ) @@ -201,6 +200,10 @@ pw_cc_flag_set( flags = [ "-mcpu=cortex-m7", "-mfloat-abi=hard", + "-mfpu=fpv5-d16", + # Used by some pigweed tests/targets to correctly handle hardware FPU + # behavior. + "-DPW_ARMV7M_ENABLE_FPU=1", ], ) @@ -215,6 +218,10 @@ pw_cc_flag_set( flags = [ "-mcpu=cortex-m33", "-mfloat-abi=hard", + "-mfpu=fpv5-sp-d16", + # Used by some pigweed tests/targets to correctly handle hardware FPU + # behavior. + "-DPW_ARMV7M_ENABLE_FPU=1", ], ) @@ -256,7 +263,7 @@ pw_cc_toolchain( "%sysroot%/arm-none-eabi/include", ], flag_sets = [ - "@pw_toolchain//flag_sets:o2", + "@pw_toolchain//flag_sets:oz", "@pw_toolchain//flag_sets:c++17", "@pw_toolchain//flag_sets:debugging", "@pw_toolchain//flag_sets:reduced_size", @@ -264,6 +271,7 @@ pw_cc_toolchain( "@pw_toolchain//flag_sets:no_rtti", "@pw_toolchain//flag_sets:wno_register", "@pw_toolchain//flag_sets:wnon_virtual_dtor", + "//pw_toolchain/cc:common_warnings", ] + select({ "@pw_toolchain//constraints/arm_mcpu:cortex-m0": [":cortex-m0"], "@pw_toolchain//constraints/arm_mcpu:cortex-m0plus": [":cortex-m0plus"], diff --git a/pw_toolchain/cc/BUILD.bazel b/pw_toolchain/cc/BUILD.bazel new file mode 100644 index 000000000..b2eb3dcab --- /dev/null +++ b/pw_toolchain/cc/BUILD.bazel @@ -0,0 +1,74 @@ +# Copyright 2024 The Pigweed Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may not +# use this file except in compliance with the License. You may obtain a copy of +# the License at +# +# https://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations under +# the License. + +load( + "@pw_toolchain//cc_toolchain:defs.bzl", + "pw_cc_feature", + "pw_cc_flag_set", +) + +package(default_visibility = ["//visibility:public"]) + +# The common set of warnings for clang and arm_gcc. +pw_cc_flag_set( + name = "common_warnings", + actions = [ + "@pw_toolchain//actions:all_c_compiler_actions", + "@pw_toolchain//actions:all_cpp_compiler_actions", + ], + flags = [ + "-Wall", + "-Wextra", + "-Wimplicit-fallthrough", + "-Wcast-qual", + "-Wpointer-arith", + "-Wshadow", + "-Wredundant-decls", + # TODO: b/332954913 - enable once code fixed + # "-Wsign-conversion", + # Make all warnings errors, except for the exemptions below. + "-Werror", + "-Wno-error=cpp", # preprocessor #warning statement + "-Wno-error=deprecated-declarations", # [[deprecated]] attribute + ], +) + +# This config contains warnings that are enabled for upstream Pigweed. +# This config MUST NOT be used downstream to allow for warnings to be +# added in the future without breaking downstream. +pw_cc_flag_set( + name = "internal_strict_warnings_flags", + actions = [ + "@pw_toolchain//actions:all_c_compiler_actions", + "@pw_toolchain//actions:all_cpp_compiler_actions", + ], + flags = [ + "-Wswitch-enum", + "-Wextra-semi", + ] + select({ + "@platforms//os:windows": [], + # TODO: b/243069432 - Enable pedantic warnings on Windows once passing. + "//conditions:default": [":pedantic_warnings"], + }), + visibility = ["//:__subpackages__"], +) + +# Add `--features=internal_strict_warnings` to your Bazel invocation to enable. +pw_cc_feature( + name = "internal_strict_warnings", + enabled = False, + feature_name = "internal_strict_warnings", + flag_sets = [":internal_strict_warnings_flags"], + visibility = ["//:__subpackages__"], +) diff --git a/pw_toolchain/host_clang/BUILD.bazel b/pw_toolchain/host_clang/BUILD.bazel index b201d2cbc..4950aa91f 100644 --- a/pw_toolchain/host_clang/BUILD.bazel +++ b/pw_toolchain/host_clang/BUILD.bazel @@ -59,25 +59,6 @@ pw_cc_flag_set( flags = ["-no_warning_for_no_symbols"], ) -# Although we use similar warnings for clang and arm_gcc, we don't have one -# centralized list, since we might want to use different warnings based on the -# compiler in the future. -pw_cc_flag_set( - name = "warnings", - actions = [ - "@pw_toolchain//actions:all_c_compiler_actions", - "@pw_toolchain//actions:all_cpp_compiler_actions", - ], - flags = [ - "-Wall", - "-Wextra", - # Make all warnings errors, except for the exemptions below. - "-Werror", - "-Wno-error=cpp", # preprocessor #warning statement - "-Wno-error=deprecated-declarations", # [[deprecated]] attribute - ], -) - # Thread safety warnings are only supported by Clang. pw_cc_flag_set( name = "thread_safety_warnings", @@ -232,7 +213,6 @@ pw_cc_toolchain( ], "//conditions:default": [], }) + [ - ":warnings", ":thread_safety_warnings", "@pw_toolchain//flag_sets:c++17", "@pw_toolchain//flag_sets:debugging", @@ -241,6 +221,7 @@ pw_cc_toolchain( "@pw_toolchain//flag_sets:no_rtti", "@pw_toolchain//flag_sets:wno_register", "@pw_toolchain//flag_sets:wnon_virtual_dtor", + "//pw_toolchain/cc:common_warnings", ] + select({ "//pw_build:kythe": [":no_unknown_warning_option"], "//conditions:default": [], diff --git a/pw_toolchain_bazel/flag_sets/BUILD.bazel b/pw_toolchain_bazel/flag_sets/BUILD.bazel index d4bd851ee..4df176102 100644 --- a/pw_toolchain_bazel/flag_sets/BUILD.bazel +++ b/pw_toolchain_bazel/flag_sets/BUILD.bazel @@ -32,6 +32,17 @@ pw_cc_flag_set( flags = ["-O2"], ) +# Optimization aggressively for size rather than speed option +pw_cc_flag_set( + name = "oz", + actions = [ + "@pw_toolchain//actions:all_c_compiler_actions", + "@pw_toolchain//actions:all_cpp_compiler_actions", + "@pw_toolchain//actions:all_link_actions", + ], + flags = ["-Oz"], +) + # Prevent relative paths from being converted to absolute paths. pw_cc_flag_set( name = "no_canonical_prefixes", diff --git a/targets/stm32f429i_disc1/boot.cc b/targets/stm32f429i_disc1/boot.cc index e47dec0d3..0c57fe569 100644 --- a/targets/stm32f429i_disc1/boot.cc +++ b/targets/stm32f429i_disc1/boot.cc @@ -26,7 +26,7 @@ // have to be incredibly careful that this does not end up in the .data section. void pw_boot_PreStaticMemoryInit() { // TODO: b/264897542 - Whether the FPU is enabled should be an Arm target trait. -#if PW_ARMV7M_ENABLE_FPU +#if defined(PW_ARMV7M_ENABLE_FPU) && PW_ARMV7M_ENABLE_FPU // Enable FPU if built using hardware FPU instructions. // CPCAR mask that enables FPU. (ARMv7-M Section B3.2.20) constexpr uint32_t kFpuEnableMask = (0xFu << 20);