nginx
Some bogus may be used uninitialized [-Wmaybe-uninitialized] warnings when compiling with -Og
When compiling with GCC and -Og we get the following warnings
cc -c -pipe -fPIC -fvisibility=hidden -Og -W -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wmissing-prototypes -g -I src -I build/include \
\
\
\
-o build/src/nxt_main.o \
-MMD -MF build/src/nxt_main.dep -MT build/src/nxt_main.o \
src/nxt_main.c
src/nxt_sprintf.c: In function ‘nxt_vsprintf’:
src/nxt_sprintf.c:564:20: warning: ‘length’ may be used uninitialized [-Wmaybe-uninitialized]
564 | length = nxt_min((size_t) (end - buf), length);
src/nxt_sprintf.c:102:26: note: ‘length’ was declared here
102 | size_t length;
| ^~~~~~
src/nxt_router.c: In function ‘nxt_router_app_prepare_request’:
src/nxt_router.c:5314:12: warning: ‘notify’ may be used uninitialized [-Wmaybe-uninitialized]
5314 | if (notify != 0) {
| ^
src/nxt_router.c:5247:27: note: ‘notify’ was declared here
5247 | int notify;
| ^~~~~~
This looks bogus as the only place we may end up at
557 copy:
558
559 if (nxt_slow_path(p == NULL)) {
560 p = null;
561 length = nxt_length(null);
562
563 } else {
564 length = nxt_min((size_t) (end - buf), length);
565 }
566
567 buf = nxt_cpymem(buf, p, length);
568 continue;
569 }
without setting length is via
153 case 's':
154 fmt++;
155
156 p = va_arg(args, const u_char *);
157
158 if (nxt_slow_path(p == NULL)) {
159 goto copy;
160 }
161
162 while (*p != '\0' && buf < end) {
163 *buf++ = *p++;
164 }
165
166 continue;
However in this case p is NULL, so we won't hit the problematic code.
The second also looks like a bogus warning as notify will get set by
5310 res = nxt_app_queue_send(port->queue, &msg, sizeof(msg),
5311 req_rpc_data->stream, ¬ify,
5312 &req_rpc_data->msg_info.tracking_cookie);
5313 if (nxt_fast_path(res == NXT_OK)) {
5314 if (notify != 0) {
5315 (void) nxt_port_socket_write(task, port,
5316 NXT_PORT_MSG_READ_QUEUE,
5317 -1, req_rpc_data->stream,
5318 reply_port->id, NULL);
I suppose the question is, do we want to suppress these warnings?
The first one was introduced at https://github.com/nginx/unit/commit/caa05887ff70bbd6338b129959a234ef56f1a287. And two ways to fix it.
diff -r 0e6d01d0c23b src/nxt_sprintf.c
--- a/src/nxt_sprintf.c Thu Sep 28 15:14:21 2023 +0100
+++ b/src/nxt_sprintf.c Mon Oct 09 16:06:19 2023 +0800
@@ -156,7 +156,8 @@
p = va_arg(args, const u_char *);
if (nxt_slow_path(p == NULL)) {
- goto copy;
+ buf = nxt_cpymem(buf, null, nxt_length(null));
+ continue;
}
while (*p != '\0' && buf < end) {
@@ -174,6 +175,11 @@
fmt++;
p = va_arg(args, const u_char *);
+ if (nxt_slow_path(p == NULL)) {
+ buf = nxt_cpymem(buf, null, nxt_length(null));
+ continue;
+ }
+
goto copy;
}
@@ -556,14 +562,7 @@
copy:
- if (nxt_slow_path(p == NULL)) {
- p = null;
- length = nxt_length(null);
-
- } else {
- length = nxt_min((size_t) (end - buf), length);
- }
-
+ length = nxt_min((size_t) (end - buf), length);
buf = nxt_cpymem(buf, p, length);
continue;
}
diff -r 0e6d01d0c23b src/nxt_sprintf.c
--- a/src/nxt_sprintf.c Thu Sep 28 15:14:21 2023 +0100
+++ b/src/nxt_sprintf.c Mon Oct 09 16:07:18 2023 +0800
@@ -156,6 +156,7 @@
p = va_arg(args, const u_char *);
if (nxt_slow_path(p == NULL)) {
+ length = 0;
goto copy;
}
I prefer (1) since the second one is a trick to avoid warnings.
About the notify variable, we could just set it with an initial value like this. But it's also a trick like the above.
diff -r 0e6d01d0c23b src/nxt_router.c
--- a/src/nxt_router.c Thu Sep 28 15:14:21 2023 +0100
+++ b/src/nxt_router.c Mon Oct 09 16:23:24 2023 +0800
@@ -5244,7 +5244,7 @@
nxt_int_t res;
nxt_port_t *port, *reply_port;
- int notify;
+ int notify = 0;
struct {
nxt_port_msg_t pm;
nxt_port_mmap_msg_t mm;
diff -r 0e6d01d0c23b src/nxt_sprintf.c
And it has no issue here, since if nxt_app_queue_send() returns NXT_OK, the notify has been set a value.
But the fixes are based on if the GCC option -Og is enabled.
do we want to suppress these warnings?
I rarely modify Makefile manually, but I'm open to it.
The first one was introduced at caa0588. And two ways to fix it.
diff -r 0e6d01d0c23b src/nxt_sprintf.c --- a/src/nxt_sprintf.c Thu Sep 28 15:14:21 2023 +0100 +++ b/src/nxt_sprintf.c Mon Oct 09 16:06:19 2023 +0800 @@ -156,7 +156,8 @@ p = va_arg(args, const u_char *); if (nxt_slow_path(p == NULL)) { - goto copy; + buf = nxt_cpymem(buf, null, nxt_length(null)); + continue; } while (*p != '\0' && buf < end) { @@ -174,6 +175,11 @@ fmt++; p = va_arg(args, const u_char *); + if (nxt_slow_path(p == NULL)) { + buf = nxt_cpymem(buf, null, nxt_length(null)); + continue; + } + goto copy; } @@ -556,14 +562,7 @@ copy: - if (nxt_slow_path(p == NULL)) { - p = null; - length = nxt_length(null); - - } else { - length = nxt_min((size_t) (end - buf), length); - } - + length = nxt_min((size_t) (end - buf), length); buf = nxt_cpymem(buf, p, length); continue; }
Yes, I like this one as it's more explicit than what we currently have.
About the
notifyvariable, we could just set it with an initial value like this. But it's also a trick like the above.diff -r 0e6d01d0c23b src/nxt_router.c --- a/src/nxt_router.c Thu Sep 28 15:14:21 2023 +0100 +++ b/src/nxt_router.c Mon Oct 09 16:23:24 2023 +0800 @@ -5244,7 +5244,7 @@ nxt_int_t res; nxt_port_t *port, *reply_port; - int notify; + int notify = 0; struct { nxt_port_msg_t pm; nxt_port_mmap_msg_t mm; diff -r 0e6d01d0c23b src/nxt_sprintf.c
I'd be inclined not to bother about this. GCC may get better at this at some point and compiling with -O0 doesn't have this issue and generally does OK for debugging.
I'd be inclined not to bother about this.
Do you mean to keep them as is? including the rework on the length warning?
If yes, I agree.
On Mon, 09 Oct 2023 02:47:47 -0700 hongzhidao @.***> wrote:
I'd be inclined not to bother about this.
Do you mean to keep them as is? including the rework on the
lengthwarning? If yes, I agree.
I mean to use your first option for the first problem, I think it improves the readability of the code.
But not bother fixing the second issue which would simply be an appeasement.