[PATCH v3 1/4] debug_uart: Replace debug functions with dummies if CONFIG_DEBUG_UART is not set
Łukasz Czechowski
lukasz.czechowski at thaumatec.com
Fri Nov 8 21:38:39 CET 2024
Hi Kever,
On 2024/10/31 10:33 Kever Yang <kever.yang at rock-chips.com> wrote:
>
> Hi Lukasz,
>
> On 2024/10/31 17:01, Kever Yang wrote:
> > Hi Lukasz, Quentin,
> >
> > On 2024/10/25 22:56, Quentin Schulz wrote:
> >> Hi Lukasz,
> >>
> >> On 10/25/24 4:27 PM, Łukasz Czechowski wrote:
> >>> Hi,
> >>>
> >>> On 2024/10/25 14:30, Kever Yang wrote:
> >>>>
> >>>> Hi Tom,
> >>>>
> >>>> This is regression of "#ifdef CONFIG", is it possible for us
> >>>> to go
> >>>> back to use "#ifdef CONFIG" in this case?
> >>>>
> >>>> Or do you have any other suggestion for this issue?
> >>>>
> >>>> On 2024/9/30 16:55, Quentin Schulz wrote:
> >>>>> Hi Kever,
> >>>>>
> >>>>> On 9/29/24 3:53 AM, Kever Yang wrote:
> >>>>>> Hi Lukasz,
> >>>>>>
> >>>>>> I think this will make the error happen like this:
> >>>>>>
> >>>>>> +common/console.c: In function 'puts':
> >>>>>> +common/console.c:746:29: error: unused variable 'ch'
> >>>>>> [-Werror=unused- variable]
> >>>>>> + 746 | int ch = *s++;
> >>>>>> + | ^~
> >>>>>> +cc1: all warnings being treated as errors
> >>>>>> +make[2]: *** [scripts/Makefile.build:257: common/console.o] Error 1
> >>>>>>
> >>>>>>
> >>>>>> The main reason is that below patch removes "#ifdef":
> >>>>>>
> >>>>>> c04f856822a console: remove #ifdef CONFIG when it is possible
> >>>>>>
> >>>>>
> >>>>> Can you please always share the link to the pipelines that fail so
> >>>>> people have an idea on how to reproduce it locally?
> >>>>>
> >>>>> Here I assume it is:
> >>>>> https://source.denx.de/u-boot/custodians/u-boot-rockchip/-/pipelines/22455
> >>>>>
> >>>>>
> >>>>> A simple way is to apply the patches, build the pine64-lts for
> >>>>> example
> >>>>> and then you'll see warnings (which aren't failing builds locally I
> >>>>> believe but in CI, yes).
> >>>>>
> >>>>> I think we can fool the compiler with the following:
> >>>>>
> >>>>> diff --git a/include/debug_uart.h b/include/debug_uart.h
> >>>>> index dc0f1aa4c98..b19e44d6d0f 100644
> >>>>> --- a/include/debug_uart.h
> >>>>> +++ b/include/debug_uart.h
> >>>>> @@ -204,12 +204,12 @@ void printdec(unsigned int value);
> >>>>> #define DEBUG_UART_FUNCS \
> >>>>> #warning "DEBUG_UART not defined!"
> >>>>>
> >>>>> -#define printch(ch) do{}while(0);
> >>>>> -#define printascii(str) do{}while(0);
> >>>>> -#define printhex2(value) do{}while(0);
> >>>>> -#define printhex4(value) do{}while(0);
> >>>>> -#define printhex8(value) do{}while(0);
> >>>>> -#define printdec(value) do{}while(0);
> >>>>> +#define printch(ch) do{ (void)(ch); }while(0);
> >>>>> +#define printascii(str) do{ (void)(str); }while(0);
> >>>>> +#define printhex2(value) do{ (void)(value); }while(0);
> >>>>> +#define printhex4(value) do{ (void)(value); }while(0);
> >>>>> +#define printhex8(value) do{ (void)(value); }while(0);
> >>>>> +#define printdec(value) do{ (void)(value); }while(0);
> >>>>>
> >>>>> #endif
> >>>>>
> >>>>> Does this make sense?
> >>>>
> >>>> Hi Quentin,
> >>>>
> >>>> Thanks for your information about pipeline and suggestion for
> >>>> code
> >>>> change, but this workaround does not looks good :(
> >>>>
> >>>
> >>> Thanks for the suggestions. I think this code can be even simplified
> >>> to just i.e.:
> >>>
> >>> @@ -204,12 +204,12 @@ void printdec(unsigned int value);
> >>> #define DEBUG_UART_FUNCS \
> >>> #warning "DEBUG_UART not defined!"
> >>>
> >>> -#define printch(ch) do{}while(0);
> >>> -#define printascii(str) do{}while(0);
> >>> -#define printhex2(value) do{}while(0);
> >>> -#define printhex4(value) do{}while(0);
> >>> -#define printhex8(value) do{}while(0);
> >>> -#define printdec(value) do{}while(0);
> >>> +#define printch(ch) (void)ch;
> >>> +#define printascii(str) (void)str;
> >>> +#define printhex2(value) (void)value;
> >>> +#define printhex4(value) (void)value;
> >>> +#define printhex8(value) (void)value;
> >>> +#define printdec(value) (void)value;
> >>>
> >>> #endif
> >>>
> >>> That will allow us to get rid of warnings. What do you think? I can't
> >>> think of another method besides creating a lot of #ifdefs in every
> >>> place debug functions are used, which will look even worse than the
> >>> dummy macros.
> >>>
> >>
> >> You need to surround value/str/ch with parentheses though.
> >>
> >> And we should remove the trailing semi-colon too as we cannot
> >> guarantee it won't be used in contexts in which the semi-colon is not
> >> allowed.
> >>
> >> But I guess that could work? I'm not too verse in C macros so maybe
> >> I'm missing some corner-cases.
> >>
> >> I think Kever is suggesting we revert the commit he linked earlier so
> >> that the functions used by the macros modified in this commit are
> >> always defined, just empty.
> >
> > At first, what I think first is:
> >
> > We go back to use #ifdef in the C code, and then we can apply this
> > patch directly with below change:
> >
> > +++ b/common/console.c
> > @@ -744,7 +744,8 @@ void puts(const char *s)
> > return;
> > }
> >
> > - if (IS_ENABLED(CONFIG_DEBUG_UART) && !(gd->flags &
> > GD_FLG_SERIAL_READY)) {
> > +#ifdef CONFIG_DEBUG_UART
> > + if (!(gd->flags & GD_FLG_SERIAL_READY)) {
> > while (*s) {
> > int ch = *s++;
> >
> > @@ -752,6 +753,7 @@ void puts(const char *s)
> > }
> > return;
> > }
> > +#endif
> >
> >
> > Since we need to change code in function puts, then below change looks
> > better, :
> >
> > +++ b/common/console.c
> > @@ -745,11 +745,7 @@ void puts(const char *s)
> > }
> >
> > if (IS_ENABLED(CONFIG_DEBUG_UART) && !(gd->flags &
> > GD_FLG_SERIAL_READY)) {
> > - while (*s) {
> > - int ch = *s++;
> > -
> > - printch(ch);
> > - }
> > + printascii(s);
> > return;
> > }
> >
> > But I don't know why use 'printch' at the beginning, maybe try to
> > convert "(ch == '\n')" to '\r' which later move into "_printch", so
> > we can use printascii() now.
>
> I have send this change to mailing list[1], if it's accepted, then I
> will apply this patch set directly without change.
>
>
> Thanks,
>
> - Kever
>
> [1]
> https://patchwork.ozlabs.org/project/uboot/patch/20241108163612.1.Ib408a6723ba954c932968419678bd45b0767a470@changeid/
>
Thank you, that sounds perfect to me.
Best regards,
Lukasz
> >
> >
> > Thanks,
> >
> > - Kever
> >
> >> Is this something you could test? The downside to this is that we
> >> would have a lot of unnecessary dead-code with loops calling empty
> >> functions instead of just not calling functions at all.
> >>
> >> Cheers,
> >> Quentin
> >>
> >
More information about the U-Boot
mailing list