GNU bug report logs - #55895
[PATCH] maint: Fix ptr_align signature to silence -Wmaybe-uninitialized

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 55895 in the body.
You can then email your comments to 55895 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Anders Kaseorg <andersk <at> mit.edu>
To: bug-coreutils <at> gnu.org
Subject: [PATCH] maint: Fix ptr_align signature to silence
 -Wmaybe-uninitialized
Date: Fri, 10 Jun 2022 18:24:15 -0400 (EDT)

ptr_align is always called with a pointer to uninitialized memory, so
it does not make sense for that pointer to be const.  This change
avoids -Wmaybe-uninitialized warnings from GCC 11.

Signed-off-by: Anders Kaseorg <andersk <at> mit.edu>
---

Some of the warnings from GCC 11.3.0 without this patch:

  CC       src/cksum-digest.o
src/digest.c: In function 'digest_check':
src/digest.c:1036:31: error: 'bin_buffer_unaligned' may be used uninitialized [-Werror=maybe-uninitialized]
 1036 |   unsigned char *bin_buffer = ptr_align (bin_buffer_unaligned, DIGEST_ALIGN);
      |                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from src/digest.c:24:
src/system.h:493:1: note: by argument 1 of type 'const void *' to 'ptr_align' declared here
  493 | ptr_align (void const *ptr, size_t alignment)
      | ^~~~~~~~~
src/digest.c:1034:17: note: 'bin_buffer_unaligned' declared here
 1034 |   unsigned char bin_buffer_unaligned[DIGEST_BIN_BYTES + DIGEST_ALIGN];
      |                 ^~~~~~~~~~~~~~~~~~~~
src/digest.c: In function 'main':
src/digest.c:1247:31: error: 'bin_buffer_unaligned' may be used uninitialized [-Werror=maybe-uninitialized]
 1247 |   unsigned char *bin_buffer = ptr_align (bin_buffer_unaligned, DIGEST_ALIGN);
      |                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from src/digest.c:24:
src/system.h:493:1: note: by argument 1 of type 'const void *' to 'ptr_align' declared here
  493 | ptr_align (void const *ptr, size_t alignment)
      | ^~~~~~~~~
src/digest.c:1245:17: note: 'bin_buffer_unaligned' declared here
 1245 |   unsigned char bin_buffer_unaligned[DIGEST_BIN_BYTES + DIGEST_ALIGN];
      |                 ^~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make[2]: *** [Makefile:20574: src/cksum-digest.o] Error 1

 src/system.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/system.h b/src/system.h
index 0c5c9b900..120fd15e4 100644
--- a/src/system.h
+++ b/src/system.h
@@ -490,7 +490,7 @@ lcm (size_t u, size_t v)
    locations.  */
 
 static inline void *
-ptr_align (void const *ptr, size_t alignment)
+ptr_align (void *ptr, size_t alignment)
 {
   char const *p0 = ptr;
   char const *p1 = p0 + alignment - 1;
-- 
2.36.1

Message #8 received at 55895 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Anders Kaseorg <andersk <at> mit.edu>
Cc: 55895 <at> debbugs.gnu.org
Subject: Re: bug#55895: [PATCH] maint: Fix ptr_align signature to silence
 -Wmaybe-uninitialized
Date: Fri, 10 Jun 2022 20:23:19 -0700

On 6/10/22 15:24, Anders Kaseorg wrote:
> ptr_align is always called with a pointer to uninitialized memory, so
> it does not make sense for that pointer to be const.

I don't see why not. ptr_align does not modify the referenced storage so 
"void const *" is appropriate. If we changed the type to "void *" we'd 
unnecessarily limit ptr_align's applicability.


> This change avoids -Wmaybe-uninitialized warnings from GCC 11.

I can't reproduce the problem with the latest coreutils commit 
93e099e4c3b659b2e329f655fbdc73fdf594a66e on Savannah master on Ubuntu 
22.04 LTS x86-64, I don't get warnings with either gcc (Ubuntu 
11.2.0-19ubuntu1) 11.2.0 or with gcc-12 (Ubuntu 12-20220319-1ubuntu1) 
12.0.1 20220319 (experimental) [master r12-7719-g8ca61ad148f]. I also 
don't see a problem on Fedora 36 x86-64 with gcc (GCC) 12.1.1 20220507 
(Red Hat 12.1.1-1)
. In all cases I configured and built with "./configure 
--enable-gcc-warnings; make".

Perhaps the problem (whatever it was) has already been fixed in 
coreutils. If not, I guess that the issue has something to do with the 
particular platform and options you configured with. If it's an older 
compiler like GCC 11.3.0 I wouldn't worry too much about it, as older 
GCCs are notorious for false alarms and not worth the trouble of pacifying.

Most likely the warnings are false positives (though I haven't checked 
this).

Message #11 received at 55895 <at> debbugs.gnu.org (full text, mbox):

From: Anders Kaseorg <andersk <at> mit.edu>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 55895 <at> debbugs.gnu.org
Subject: Re: bug#55895: [PATCH] maint: Fix ptr_align signature to silence
 -Wmaybe-uninitialized
Date: Fri, 10 Jun 2022 21:11:48 -0700

Thanks for checking. I should have nailed down the reproduction recipe 
more precisely; sorry for that.  It seems the important step I should 
have included was CFLAGS=-O0.  Full recipe, using the same systems you 
checked:

Ubuntu 22.04 LTS x86-64
gcc (Ubuntu 11.2.0-19ubuntu1) 11.2.0
coreutils master, commit 93e099e4c3b659b2e329f655fbdc73fdf594a66e
./bootstrap --skip-po
./configure CFLAGS=-O0 (with or without --enable-gcc-warnings)
make

or

Fedora 36 x86-64
gcc (GCC) 12.1.1 20220507 (Red Hat 12.1.1-1)
coreutils master, commit 93e099e4c3b659b2e329f655fbdc73fdf594a66e
./bootstrap --skip-po
./configure CFLAGS=-O0 (with or without --enable-gcc-warnings)
make -k

(With GCC 12.1.1 I get the same error and also additional errors that 
might merit further investigation.)

Can you reproduce with CFLAGS=-O0?

On 6/10/22 20:23, Paul Eggert wrote:
> On 6/10/22 15:24, Anders Kaseorg wrote:
>> ptr_align is always called with a pointer to uninitialized memory, so
>> it does not make sense for that pointer to be const.
> 
> I don't see why not. ptr_align does not modify the referenced storage so 
> "void const *" is appropriate. If we changed the type to "void *" we'd 
> unnecessarily limit ptr_align's applicability.

My claim is that there’s never a reason to call ptr_align with a const 
pointer, because if the memory is initialized the pointer would have 
already been aligned, and if the memory is uninitialized you’re going to 
need to write to it somewhere.

Also, the current signature converts a const pointer to a mutable 
pointer.  Given the squishy nature of const in C (and the general lack 
of language features that would make it a more reliably useful signal), 
I know this kind of “safety hole” is so common that I hesitate to 
suggest it’s really a hole.  But given that it’s unnecessary we might as 
well avoid it.

GCC’s -Wmaybe-uninitialized seems to assume that a const pointer might 
be read, in the absense of better information from higher optimization 
levels.  Although this heuristic isn’t perfect (I’m not suggesting that 
the code is actually _wrong_), it seems reasonable enough to be worth 
appeasing, given that there doesn’t seem to be a notable downside.

Anders

Message #16 received at 55895-done <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Anders Kaseorg <andersk <at> mit.edu>
Cc: 55895-done <at> debbugs.gnu.org
Subject: Re: bug#55895: [PATCH] maint: Fix ptr_align signature to silence
 -Wmaybe-uninitialized
Date: Sat, 11 Jun 2022 09:12:08 -0700

On 6/10/22 21:11, Anders Kaseorg wrote:
> It seems the important step I should 
> have included was CFLAGS=-O0.

Ah, OK. Since you're building from Git, I can refer you to 
README-hacking which is intended for that. It says, "If you get warnings 
with other configurations, you can run
 './configure --disable-gcc-warnings' or 'make WERROR_CFLAGS='
 to build quietly or verbosely, respectively.
" Here, "other configurations" refers to what you're doing.

> (With GCC 12.1.1 I get the same error and also additional errors that might merit further investigation.) 

Like most static analysis tools, GCC generates a bunch of false 
positives unless you baby it just right. We do the babying only for the 
latest GCC with the default configuration; otherwise, it's typically not 
worth the trouble. Feel free to investigate the other warnings, but 
they're important only if they're true positives (and most likely 
they're not, because gcc -O0 is dumber than gcc -O2).

> there’s never a reason to call ptr_align with a const pointer, because if the memory is initialized the pointer would have already been aligned

First, a const pointer can point to uninitialized storage. Second, even 
if the referenced memory is initialized the pointer need not be aligned 
already. For example, this is valid:

    char *p = malloc (1024);
    if (!p) return;
    char const *q = p; // q points to uninitialized storage
    char const *r = ptr_align (q, 512); // q is not aligned already
    memset (p, 127, 1024);
    ...

Replacing 'malloc (1024)' with 'calloc (1024, 1)' (thus initializing the 
storage before aligning the pointer) wouldn't affect the validity of the 
code.

> Also, the current signature converts a const pointer to a mutable pointer.

Yes, it's like strchr which is annoying but that's the best C can do.

You're right that changing it from void const * to void * won't hurt 
coreutils' current callers but I'd rather not massage the code merely to 
pacify nondefault configurations. There are too many nondefault 
configurations to worry about and massaging the code to pacify them all 
would waste our time and confuse the code. Instead, we pacify only 
default configurations with current GCC.

Message #19 received at 55895 <at> debbugs.gnu.org (full text, mbox):

From: Anders Kaseorg <andersk <at> mit.edu>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 55895 <at> debbugs.gnu.org
Subject: Re: bug#55895: [PATCH] maint: Fix ptr_align signature to silence
 -Wmaybe-uninitialized
Date: Sat, 11 Jun 2022 09:30:28 -0700

On 6/11/22 09:12, Paul Eggert wrote:
> First, a const pointer can point to uninitialized storage.

Of course it can, but my claim was there’s never a reason.  A pointer to 
uninitialized (or zero-initialized) memory that won’t be written is 
valid but not _useful_.  The reason you’d align a pointer is so you can 
store something there that needs to be aligned.  It’s not an accident 
that all of the current callers work that way.

Anders

Message #22 received at 55895 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Anders Kaseorg <andersk <at> mit.edu>
Cc: 55895 <at> debbugs.gnu.org
Subject: Re: bug#55895: [PATCH] maint: Fix ptr_align signature to silence
 -Wmaybe-uninitialized
Date: Sat, 11 Jun 2022 10:13:19 -0700

On 6/11/22 09:30, Anders Kaseorg wrote:
> A pointer to uninitialized (or zero-initialized) memory that won’t be 
> written is valid but not _useful_.

But in the example I gave, the memory *is* written to later.

A const * pointer lets a C program have a read-only window into memory 
that other parts of the program can write to, which can be a useful 
thing to have. In C and C++, "const *" doesn't mean a pointer to storage 
that does not change; it merely means a pointer that can't be used to 
write the referenced storage.

Message #25 received at 55895 <at> debbugs.gnu.org (full text, mbox):

From: Anders Kaseorg <andersk <at> mit.edu>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: "55895 <at> debbugs.gnu.org" <55895 <at> debbugs.gnu.org>
Subject: Re: bug#55895: [PATCH] maint: Fix ptr_align signature to silence
 -Wmaybe-uninitialized
Date: Sat, 11 Jun 2022 21:18:00 +0000

I know C. I’m not saying that const implies writes don’t happen elsewhere. I’m saying that _aligning_ a const pointer implies it’s not _useful_ for _unaligned_ writes to happen elsewhere. And if you align the mutable pointer, you don’t need to separately align a const pointer.

Anders

Message #28 received at 55895 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Anders Kaseorg <andersk <at> mit.edu>
Cc: 55895 <at> debbugs.gnu.org
Subject: Re: bug#55895: [PATCH] maint: Fix ptr_align signature to silence
 -Wmaybe-uninitialized
Date: Sat, 11 Jun 2022 17:14:47 -0700

On 6/11/22 14:18, Anders Kaseorg wrote:

> if you align the mutable pointer, you don’t need to separately align a const pointer.

Of course one can alter code by aligning a mutable pointer first and 
then converting it to a const pointer, instead of first converting it to 
const and then aligning the result. But that might not be convenient. 
The part of the code that needs alignment might have access to only the 
const pointer, and should be able to align the const pointer on its own 
without bothering the part of the code that doesn't need alignment.

Anyway, we're getting a long way from the original bug report. I'll let 
you have the last word on this tangential topic, if you like.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.