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

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@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