Some bogus may be used uninitialized [-Wmaybe-uninitialized] warnings when compiling with -Og

[ac000]

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, &notify,                
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?

[hongzhidao]

The first one was introduced at https://github.com/nginx/unit/commit/caa05887ff70bbd6338b129959a234ef56f1a287. And two ways to fix it.

1.

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;
     }

2.

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.

[ac000]

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

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.

[hongzhidao]

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.

[ac000]

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 length warning? 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.

[ac000]

A patch addressing the first warning has been merged

[ac000]

With current GCC (14..1.1) the second warning is gone.