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
