Hi Simon,
On 8/11/24 4:50 PM, Simon Glass wrote:
Hi Quentin,
On Tue, 6 Aug 2024 at 08:16, Quentin Schulz quentin.schulz@cherry.de wrote:
Hi Simon,
On 7/21/24 5:25 PM, Simon Glass wrote:
Unless function names are requested, the logging system should not compile these into the code. Adjust the macros to handle this.
This means that turning on function names at runtime won't work unless CONFIG_LOGF_FUNC is enabled. We could perhaps split this into a separate option if that is a problem.
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.
I now understand the statement in patches earlier in this series :)
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2:
Update commit message to mention the runtime impact
Leave assert() alone since it is only compiled in with LOG_DEBUG
common/log_console.c | 4 ++-- configs/sandbox_defconfig | 1 + include/log.h | 16 +++++++++++----- test/cmd/pinmux.c | 8 +++++++- test/log/log_test.c | 4 ++-- 5 files changed, 23 insertions(+), 10 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 ?: "?");What about setting _log_func to "?" if LOGF_FUNC isn't set?
That would require all call sites to pass a pointer to the string, rather than just NULL. I suspect it would increase code size?
I probably missed something, but log_rec->func doesn't seem to be used in many places (common/log_console.c and common/log_syslog.c).
log_console_emit() is only called through log_dispatch() itself only called from _log().
_log is called from: - cmd/log.c:do_log_rec(), the func parameter is provided by argv[5] which I assume cannot be NULL because argc<7 is checked. - common/log.c:_log_buffer() which is called by log_buffer() - include/log.h:log()/log_buffer() which is modified to swap __func__ with _log_func (so __func__ or <insert placeholder> which is NULL or "?" I suggested there). the rec->func argument cannot be changed from any of the caller.
So I assume we would be fine?
As for the size increase, I would hope that the compiler knows it can store the same string at only one place and then reuse it instead of having it in many places, but I know nothing about compilers so it's just me guessing :)
In any case, either is fine by me!
Cheers, Quentin