Hi Quentin,
On Mon, 12 Aug 2024 at 10:56, Quentin Schulz <quentin.sch...@cherry.de> wrote:
>
> 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.sch...@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 <s...@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 :)
Just a note that I never got back to checking this. Common strings
with the preprocessor end up with a single string in the object file,
or at least they do in my experience. Since the function name is
likely unique within U-Boot, this should be fine, as you say.
Outside a single compilation unit it can be a different story. I
suspect (and hope) that LTO would deal with this OK, but otherwise I'm
really not sure what would happen.
>
> In any case, either is fine by me!
I suspect we may tweak this again, but this patch does resolve the
code-size issue that bugged me.
Regards,
Simon
