[PATCH v3 1/4] debug_uart: Replace debug functions with dummies if CONFIG_DEBUG_UART is not set

[Quentin Schulz]

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