[PATCH v3 1/4] debug_uart: Replace debug functions with dummies if CONFIG_DEBUG_UART is not set
Kever Yang
kever.yang at rock-chips.com
Fri Nov 8 10:33:20 CET 2024
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/
>
>
> 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