From 4cd06cf22b89b7d400c76897d011327b70cee842 Mon Sep 17 00:00:00 2001 From: Jack Rosenthal Date: Thu, 1 Aug 2019 12:49:16 -0600 Subject: common: add STATIC_IF and STATIC_IF_NOT macros A common pattern to use with IS_ENABLED is like this: /* * This var should only be used if CONFIG_FOO. The linker errors if * CONFIG_FOO is not defined is intentional. */ #ifdef CONFIG_FOO static #else extern #endif int some_var; The issue with this is that it leads to an over-verbose and potentially hard to read pattern, and does not have the check that CONFIG_FOO was only defined to blank. Suppose a macro like this existed: STATIC_IF(CONFIG_FOO) int some_var; ... which expands to "static" when CONFIG_FOO is defined to empty, "extern" when CONFIG_FOO is not defined, and errors when CONFIG_FOO is defined to non-empty. This CL implements that, as well as the inverse (STATIC_IF_NOT). BUG=chromium:989786 BRANCH=none TEST=provided unit tests, buildall Change-Id: Ib57aaba62bc184fda9aa782a780d5f13ba44ae88 Signed-off-by: Jack Rosenthal Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/1731859 Reviewed-by: Daisuke Nojiri --- include/common.h | 89 +++++++++++++++++++++++++++++++++---------- test/build.mk | 6 +++ test/static_if.c | 50 ++++++++++++++++++++++++ test/static_if.tasklist | 9 +++++ test/static_if_error.c | 34 +++++++++++++++++ test/static_if_error.sh | 42 ++++++++++++++++++++ test/static_if_error.tasklist | 9 +++++ 7 files changed, 219 insertions(+), 20 deletions(-) create mode 100644 test/static_if.c create mode 100644 test/static_if.tasklist create mode 100644 test/static_if_error.c create mode 100644 test/static_if_error.sh create mode 100644 test/static_if_error.tasklist diff --git a/include/common.h b/include/common.h index 73e4a19abc..fab97f17b4 100644 --- a/include/common.h +++ b/include/common.h @@ -9,6 +9,7 @@ #define __CROS_EC_COMMON_H #include +#include "compile_time_macros.h" /* * Macros to concatenate 2 - 4 tokens together to form a single token. @@ -272,6 +273,41 @@ enum ec_error_list { /* find the most significant bit. Not defined in n == 0. */ #define __fls(n) (31 - __builtin_clz(n)) +/* + * __cfg_select(CONFIG_NAME, EMPTY, OTHERWISE) is a macro used for + * defining other macros which conditionally select code based on a + * config option. It will generate the argument passed as EMPTY + * when CONFIG_NAME was defined to the empty string, and OTHERWISE + * when the argument was not defined or defined to something + * non-empty. + * + * Generally speaking, macros which use this should make some sort of + * context-dependent assertion in OTHERWISE that CONFIG_NAME is + * undefined, rather than defined to something else. This usually + * involves tricks with __builtin_strcmp. + */ +#define __cfg_select(cfg, empty, otherwise) \ + __cfg_select_1(cfg, empty, otherwise) +#define __cfg_select_placeholder_ _, +#define __cfg_select_1(value, empty, otherwise) \ + __cfg_select_2(__cfg_select_placeholder_##value, empty, otherwise) +#define __cfg_select_2(arg1_or_junk, empty, otherwise) \ + __cfg_select_3(arg1_or_junk _, empty, otherwise) +#define __cfg_select_3(_ignore1, _ignore2, select, ...) select + +/* + * This version concatenates a BUILD_ASSERT(...); before OTHERWISE, + * handling the __builtin_strcmp trickery where a BUILD_ASSERT is + * appropriate in the context. + */ +#define __cfg_select_build_assert(cfg, value, empty, undef) \ + __cfg_select( \ + value, \ + empty, \ + BUILD_ASSERT( \ + __builtin_strcmp(cfg, #value) == 0); \ + undef) + /* * Attribute for generating an error if a function is used. * @@ -286,26 +322,18 @@ enum ec_error_list { /* * Getting something that works in C and CPP for an arg that may or may - * not be defined is tricky. Here, if we have "#define CONFIG_FOO" - * we match on the placeholder define, insert the "_, 1," for arg1 and generate - * the triplet (_, 1, _, (...)). Then the last step cherry picks the 2nd arg - * (a one). - * When CONFIG_FOO is not defined, we generate a (_, (...)) pair, and when - * the last step cherry picks the 2nd arg, we get a code block that verifies - * the value of the option. Since the preprocessor won't replace an unknown - * token, we compare the option name with the value string. If they are - * identical we assume that the value was undefined and return 0. If the value - * happens to be anything else we call an undefined method that will raise - * a compiler error. This technique requires that the optimizer be enabled so it - * can remove the undefined function call. + * not be defined is tricky. * + * Compare the option name with the value string in the OTHERWISE to + * __cfg_select. If they are identical we assume that the value was + * undefined and return 0. If the value happens to be anything else we + * call an undefined method that will raise a compiler error. This + * technique requires that the optimizer be enabled so it can remove + * the undefined function call. */ -#define __ARG_PLACEHOLDER_ _, 1, -#define _config_enabled(cfg, value) \ - __config_enabled(__ARG_PLACEHOLDER_##value, cfg, value) -#define __config_enabled(arg1_or_junk, cfg, value) \ - ___config_enabled( \ - arg1_or_junk _, ({ \ +#define __config_enabled(cfg, value) \ + __cfg_select( \ + value, 1, ({ \ int __undefined = __builtin_strcmp(cfg, #value) == 0; \ extern int IS_ENABLED_BAD_ARGS(void) __error( \ cfg " must be , or not defined."); \ @@ -313,7 +341,6 @@ enum ec_error_list { IS_ENABLED_BAD_ARGS(); \ 0; \ })) -#define ___config_enabled(__ignored, val, ...) val /** * Checks if a config option is enabled or disabled @@ -329,6 +356,28 @@ enum ec_error_list { * Note: This macro will only function inside a code block due to the way * it checks for unknown values. */ -#define IS_ENABLED(option) _config_enabled(#option, option) +#define IS_ENABLED(option) __config_enabled(#option, option) + +/** + * Makes a global variable static when a config option is enabled, + * extern otherwise (with the intention to cause linker errors if the + * variable is used outside of a config context, for example thru + * IS_ENABLED, that it should be). + * + * This follows the same constraints as IS_ENABLED, the config option + * should be defined to nothing or undefined. + */ +#define STATIC_IF(option) \ + __cfg_select_build_assert(#option, option, static, extern) + +/** + * STATIC_IF_NOT is just like STATIC_IF, but makes the variable static + * only if the config option is *not* defined, extern if it is. + * + * This is to assert that a variable will go unused with a certain + * config option. + */ +#define STATIC_IF_NOT(option) \ + __cfg_select_build_assert(#option, option, extern, static) #endif /* __CROS_EC_COMMON_H */ diff --git a/test/build.mk b/test/build.mk index 8d68da07f5..5868513962 100644 --- a/test/build.mk +++ b/test/build.mk @@ -61,6 +61,8 @@ test-list-host += sbs_charging_v2 test-list-host += sha256 test-list-host += sha256_unrolled test-list-host += shmalloc +test-list-host += static_if +test-list-host += static_if_error test-list-host += system test-list-host += thermal test-list-host += timer_dos @@ -132,6 +134,7 @@ sbs_charging_v2-y=sbs_charging_v2.o sha256-y=sha256.o sha256_unrolled-y=sha256.o shmalloc-y=shmalloc.o +static_if-y=static_if.o stress-y=stress.o system-y=system.o thermal-y=thermal.o @@ -163,3 +166,6 @@ $(out)/RO/test/nvmem_tpm2_mock.o: CFLAGS += -I$(TPM2_ROOT) host-is_enabled_error: TEST_SCRIPT=is_enabled_error.sh is_enabled_error-y=is_enabled_error.o.cmd + +host-static_if_error: TEST_SCRIPT=static_if_error.sh +static_if_error-y=static_if_error.o.cmd diff --git a/test/static_if.c b/test/static_if.c new file mode 100644 index 0000000000..46e8057d7f --- /dev/null +++ b/test/static_if.c @@ -0,0 +1,50 @@ +/* Copyright 2019 The Chromium OS Authors. All rights reserved. + * Use of this source code is governed by a BSD-style license that can be + * found in the LICENSE file. + */ + +/* Test the STATIC_IF and STATIC_IF_NOT macros. */ + +#include "common.h" +#include "test_util.h" + +#undef CONFIG_UNDEFINED +#define CONFIG_BLANK + +STATIC_IF(CONFIG_UNDEFINED) int this_var_is_extern; +STATIC_IF_NOT(CONFIG_BLANK) int this_var_is_extern_too; +STATIC_IF(CONFIG_BLANK) int this_var_is_static; +STATIC_IF_NOT(CONFIG_UNDEFINED) int this_var_is_static_too; + +static int test_static_if_blank(void) +{ + TEST_ASSERT(this_var_is_static == 0); + TEST_ASSERT(this_var_is_static_too == 0); + + return EC_SUCCESS; +} + +static int test_static_if_unused_no_fail(void) +{ + /* + * This should not cause linker errors because the variables + * go unused (usage is optimized away). + */ + if (IS_ENABLED(CONFIG_UNDEFINED)) + this_var_is_extern = 1; + + if (!IS_ENABLED(CONFIG_BLANK)) + this_var_is_extern_too = 1; + + return EC_SUCCESS; +} + +void run_test(void) +{ + test_reset(); + + RUN_TEST(test_static_if_blank); + RUN_TEST(test_static_if_unused_no_fail); + + test_print_result(); +} diff --git a/test/static_if.tasklist b/test/static_if.tasklist new file mode 100644 index 0000000000..5ffe662d01 --- /dev/null +++ b/test/static_if.tasklist @@ -0,0 +1,9 @@ +/* Copyright 2019 The Chromium OS Authors. All rights reserved. + * Use of this source code is governed by a BSD-style license that can be + * found in the LICENSE file. + */ + +/** + * See CONFIG_TASK_LIST in config.h for details. + */ +#define CONFIG_TEST_TASK_LIST /* No test task */ diff --git a/test/static_if_error.c b/test/static_if_error.c new file mode 100644 index 0000000000..bbe4839981 --- /dev/null +++ b/test/static_if_error.c @@ -0,0 +1,34 @@ +/* Copyright 2019 The Chromium OS Authors. All rights reserved. + * Use of this source code is governed by a BSD-style license that can be + * found in the LICENSE file. + */ + +/* + * Test the STATIC_IF and STATIC_IF_NOT macros fail on unexpected + * input. + */ + +#include "common.h" +#include "test_util.h" + +#define CONFIG_FOO TEST_VALUE + +/* + * At compiler invocation, define TEST_MACRO to STATIC_IF or + * STATIC_IF_NOT. + */ +#ifndef TEST_MACRO +#error "This error should not be seen in the compiler output!" +#endif + +/* This is intended to cause a compilation error. */ +TEST_MACRO(CONFIG_FOO) __maybe_unused int foo; + +void run_test(void) +{ + test_reset(); + + /* Nothing to do, observe compilation error */ + + test_print_result(); +} diff --git a/test/static_if_error.sh b/test/static_if_error.sh new file mode 100644 index 0000000000..efc7cd3e1e --- /dev/null +++ b/test/static_if_error.sh @@ -0,0 +1,42 @@ +#!/bin/bash -e +# Copyright 2019 The Chromium OS Authors. All rights reserved. +# Use of this source code is governed by a BSD-style license that can be +# found in the LICENSE file. + +# This file is implemented similar to is_enabled_error.sh + +TEST_DIR="$(dirname "${BASH_SOURCE[0]}")" +TEST_CMD="$(cat "${TEST_DIR}/RO/test/static_if_error.o.cmd")" +TEST_ERROR_COUNT=0 +BAD_ERROR_MSG="This error should not be seen in the compiler output!" + +fail() { + echo "Fail" + echo "$1" + echo "$BUILD_OUTPUT" + TEST_ERROR_COUNT=$((TEST_ERROR_COUNT+1)) +} + +for test_macro in STATIC_IF STATIC_IF_NOT; do + for test_value in 0 1 2 A "5 + 5"; do + echo -n "Running TEST_MACRO=${test_macro} TEST_VALUE=${test_value}..." + TEST_CMD_COMPLETE=" + ${TEST_CMD} \"-DTEST_MACRO=${test_macro}\" \"-DTEST_VALUE=${test_value}\"" + echo "$TEST_CMD_COMPLETE" + if BUILD_OUTPUT="$(sh -c "$TEST_CMD_COMPLETE" 2>&1)"; then + fail "Compilation should not have succeeded." + continue + fi + + if grep -q "$BAD_ERROR_MSG" <<<"$BUILD_OUTPUT"; then + fail "TEST_MACRO was not defined." + continue + fi + done +done + +if [[ $TEST_ERROR_COUNT -eq 0 ]]; then + echo "Pass!" +else + echo "Fail! (${TEST_ERROR_COUNT} tests)" +fi diff --git a/test/static_if_error.tasklist b/test/static_if_error.tasklist new file mode 100644 index 0000000000..5ffe662d01 --- /dev/null +++ b/test/static_if_error.tasklist @@ -0,0 +1,9 @@ +/* Copyright 2019 The Chromium OS Authors. All rights reserved. + * Use of this source code is governed by a BSD-style license that can be + * found in the LICENSE file. + */ + +/** + * See CONFIG_TASK_LIST in config.h for details. + */ +#define CONFIG_TEST_TASK_LIST /* No test task */ -- cgit v1.2.1