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

[Kever Yang]

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.


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
>