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

[Quentin Schulz]

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