[PATCH 07/14] log: Avoid including function names by default

Sean Anderson seanga2 at gmail.com
Sat Jul 20 17:52:33 CEST 2024


On 7/20/24 02:16, Simon Glass wrote:
> Unless function names are requested, the logging system should not
> compile these into the code. Adjust the macros to handle this.
> 
> Enable CONFIG_LOGF_FUNC logging for sandbox since the tests expect the
> function names to be included. Fix up the pinmux test which checks a
> logging statement.
> 
> Signed-off-by: Simon Glass <sjg at chromium.org>
> ---
> 
>   common/log_console.c      |  4 ++--
>   configs/sandbox_defconfig |  1 +
>   include/log.h             | 18 ++++++++++++------
>   test/cmd/pinmux.c         |  8 +++++++-
>   test/log/log_test.c       |  4 ++--
>   5 files changed, 24 insertions(+), 11 deletions(-)
> 
> diff --git a/common/log_console.c b/common/log_console.c
> index c27101b8fe2..9376baad664 100644
> --- a/common/log_console.c
> +++ b/common/log_console.c
> @@ -38,10 +38,10 @@ static int log_console_emit(struct log_device *ldev, struct log_rec *rec)
>   			printf("%d-", rec->line);
>   		if (fmt & BIT(LOGF_FUNC)) {
>   			if (CONFIG_IS_ENABLED(USE_TINY_PRINTF)) {
> -				printf("%s()", rec->func);
> +				printf("%s()", rec->func ?: "?");
>   			} else {
>   				printf("%*s()", CONFIG_LOGF_FUNC_PAD,
> -				       rec->func);
> +				       rec->func ?: "?");
>   			}
>   		}
>   	}
> diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
> index e2db66d4a25..d43d92d0cf2 100644
> --- a/configs/sandbox_defconfig
> +++ b/configs/sandbox_defconfig
> @@ -38,6 +38,7 @@ CONFIG_PRE_CONSOLE_BUFFER=y
>   CONFIG_LOG=y
>   CONFIG_LOG_MAX_LEVEL=9
>   CONFIG_LOG_DEFAULT_LEVEL=6
> +CONFIG_LOGF_FUNC=y
>   CONFIG_DISPLAY_BOARDINFO_LATE=y
>   CONFIG_STACKPROTECTOR=y
>   CONFIG_ANDROID_AB=y
> diff --git a/include/log.h b/include/log.h
> index fc0d5984472..67b6fd70050 100644
> --- a/include/log.h
> +++ b/include/log.h
> @@ -125,7 +125,7 @@ static inline int log_uc_cat(enum uclass_id id)
>    * @level: Level of log record (indicating its severity)
>    * @file: File name of file where log record was generated
>    * @line: Line number in file where log record was generated
> - * @func: Function where log record was generated
> + * @func: Function where log record was generated, NULL if not known
>    * @fmt: printf() format string for log record
>    * @...: Optional parameters, according to the format string @fmt
>    * Return: 0 if log record was emitted, -ve on error
> @@ -141,7 +141,7 @@ int _log(enum log_category_t cat, enum log_level_t level, const char *file,
>    * @level: Level of log record (indicating its severity)
>    * @file: File name of file where log record was generated
>    * @line: Line number in file where log record was generated
> - * @func: Function where log record was generated
> + * @func: Function where log record was generated, NULL if not known
>    * @addr:	Starting address to display at start of line
>    * @data:	pointer to data buffer
>    * @width:	data value width.  May be 1, 2, or 4.
> @@ -193,6 +193,12 @@ int _log_buffer(enum log_category_t cat, enum log_level_t level,
>   #define _LOG_DEBUG	0
>   #endif
>   
> +#ifdef CONFIG_LOGF_FUNC
> +#define _log_func	__func__
> +#else
> +#define _log_func	NULL
> +#endif
> +
>   #if CONFIG_IS_ENABLED(LOG)
>   
>   /* Emit a log record if the level is less that the maximum */
> @@ -201,7 +207,7 @@ int _log_buffer(enum log_category_t cat, enum log_level_t level,
>   	if (_LOG_DEBUG != 0 || _l <= _LOG_MAX_LEVEL) \
>   		_log((enum log_category_t)(_cat), \
>   		     (enum log_level_t)(_l | _LOG_DEBUG), __FILE__, \
> -		     __LINE__, __func__, \
> +		     __LINE__, _log_func, \
>   		      pr_fmt(_fmt), ##_args); \
>   	})
>   
> @@ -211,7 +217,7 @@ int _log_buffer(enum log_category_t cat, enum log_level_t level,
>   	if (_LOG_DEBUG != 0 || _l <= _LOG_MAX_LEVEL) \
>   		_log_buffer((enum log_category_t)(_cat), \
>   			    (enum log_level_t)(_l | _LOG_DEBUG), __FILE__, \
> -			    __LINE__, __func__, _addr, _data, \
> +			    __LINE__, _log_func, _addr, _data, \
>   			    _width, _count, _linelen); \
>   	})
>   #else
> @@ -302,7 +308,7 @@ void __assert_fail(const char *assertion, const char *file, unsigned int line,
>    */
>   #define assert(x) \
>   	({ if (!(x) && _DEBUG) \
> -		__assert_fail(#x, __FILE__, __LINE__, __func__); })
> +		__assert_fail(#x, __FILE__, __LINE__, _log_func); })

Can we keep this one in? It's only compiled for debug and we are including file/line anyway.

>   /*
>    * This one actually gets compiled in even without DEBUG. It doesn't include the
> @@ -314,7 +320,7 @@ void __assert_fail(const char *assertion, const char *file, unsigned int line,
>   #define assert_noisy(x) \
>   	({ bool _val = (x); \
>   	if (!_val) \
> -		__assert_fail(#x, "?", __LINE__, __func__); \
> +		__assert_fail(#x, "?", __LINE__, _log_func); \
>   	_val; \
>   	})
>   
> diff --git a/test/cmd/pinmux.c b/test/cmd/pinmux.c
> index 4253baa5646..7ab7004b684 100644
> --- a/test/cmd/pinmux.c
> +++ b/test/cmd/pinmux.c
> @@ -30,7 +30,13 @@ static int dm_test_cmd_pinmux_status_pinname(struct unit_test_state *uts)
>   
>   	console_record_reset();
>   	run_command("pinmux status P9", 0);
> -	ut_assert_nextlinen("single-pinctrl pinctrl-single-no-width: missing register width");
> +	if (IS_ENABLED(CONFIG_LOGF_FUNC)) {
> +		ut_assert_nextlinen(
> +			"   single_of_to_plat() single-pinctrl pinctrl-single-no-width: missing register width");
> +	} else {
> +		ut_assert_nextlinen(
> +			"single-pinctrl pinctrl-single-no-width: missing register width");
> +	}
>   	ut_assert_nextlinen("P9 not found");
>   	ut_assert_console_end();
>   
> diff --git a/test/log/log_test.c b/test/log/log_test.c
> index 855353a9c40..d756b96dbac 100644
> --- a/test/log/log_test.c
> +++ b/test/log/log_test.c
> @@ -452,8 +452,8 @@ int log_test_buffer(struct unit_test_state *uts)
>   	/* This one should product no output due to the debug level */
>   	log_buffer(LOGC_BOOT, LOGL_DEBUG, 0, buf, 1, 0x12, 0);
>   
> -	ut_assert_nextline("00000000: 00 11 22 33 44 55 66 77 88 99 aa bb cc dd ee ff  ..\"3DUfw........");
> -	ut_assert_nextline("00000010: 10 00                                            ..");
> +	ut_assert_nextline("     log_test_buffer() 00000000: 00 11 22 33 44 55 66 77 88 99 aa bb cc dd ee ff  ..\"3DUfw........");
> +	ut_assert_nextline("     log_test_buffer() 00000010: 10 00                                            ..");
>   	ut_assert_console_end();
>   	free(buf);
>   

Anyway, the rest looks fine. This probably breaks the use case of someone turning on
function names at runtime, so maybe we will need to split this config if someone cares.

--Sean


More information about the U-Boot mailing list