* [PATCH 1/2] explicit_bzero-tests: pacify GCC @ 2021-07-18 4:56 Paul Eggert 2021-07-18 4:56 ` [PATCH 2/2] memrchr-tests: " Paul Eggert 2021-07-18 9:12 ` [PATCH 1/2] explicit_bzero-tests: " Bruno Haible 0 siblings, 2 replies; 7+ messages in thread From: Paul Eggert @ 2021-07-18 4:56 UTC (permalink / raw) To: bug-gnulib; +Cc: Paul Eggert Redo to pacify -Wmaybe-uninitialized with GCC 11.1.1 20210531 (Red Hat 11.1.1-3) x86-64. * tests/test-explicit_bzero.c (stackbuf): New static pointer. (do_secret_stuff): Use it. (test_stack): Set it to a local buffer. --- ChangeLog | 7 +++++++ tests/test-explicit_bzero.c | 8 +++++--- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/ChangeLog b/ChangeLog index 6ba44e7df..1caabff62 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,12 @@ 2021-07-17 Paul Eggert <eggert@cs.ucla.edu> + explicit_bzero-tests: pacify GCC + Redo to pacify -Wmaybe-uninitialized with + GCC 11.1.1 20210531 (Red Hat 11.1.1-3) x86-64. + * tests/test-explicit_bzero.c (stackbuf): New static pointer. + (do_secret_stuff): Use it. + (test_stack): Set it to a local buffer. + posixtm: pacify latest GCC Also, modernize while I’m at it. * lib/posixtm.c: Include c-ctype.h, idx.h, intprops.h, verify.h diff --git a/tests/test-explicit_bzero.c b/tests/test-explicit_bzero.c index cdb839245..c42aba93f 100644 --- a/tests/test-explicit_bzero.c +++ b/tests/test-explicit_bzero.c @@ -126,12 +126,12 @@ test_heap (void) /* There are two passes: 1. Put a secret in memory and invoke explicit_bzero on it. 2. Verify that the memory has been erased. - Implement them in the same function, so that they access the same memory - range on the stack. */ + Access the memory via a volatile pointer, so the compiler + does not assume the pointer's value and optimize away accesses. */ +static char *volatile stackbuf; static int _GL_ATTRIBUTE_NOINLINE do_secret_stuff (volatile int pass) { - char stackbuf[SECRET_SIZE]; if (pass == 1) { memcpy (stackbuf, SECRET, SECRET_SIZE); @@ -147,6 +147,8 @@ do_secret_stuff (volatile int pass) static void test_stack (void) { + char stack_buffer[SECRET_SIZE]; + stackbuf = stack_buffer; int count = 0; int repeat; -- 2.31.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] memrchr-tests: pacify GCC 2021-07-18 4:56 [PATCH 1/2] explicit_bzero-tests: pacify GCC Paul Eggert @ 2021-07-18 4:56 ` Paul Eggert 2021-07-18 9:12 ` [PATCH 1/2] explicit_bzero-tests: " Bruno Haible 1 sibling, 0 replies; 7+ messages in thread From: Paul Eggert @ 2021-07-18 4:56 UTC (permalink / raw) To: bug-gnulib; +Cc: Paul Eggert Pacify GCC 11.1.1 20210531 (Red Hat 11.1.1-3) x86-64. * tests/test-memrchr.c: Disable -Wmaybe-uninitialized. --- ChangeLog | 4 ++++ tests/test-memrchr.c | 5 +++++ 2 files changed, 9 insertions(+) diff --git a/ChangeLog b/ChangeLog index 1caabff62..c50808437 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,9 @@ 2021-07-17 Paul Eggert <eggert@cs.ucla.edu> + memrchr-tests: pacify GCC + Pacify GCC 11.1.1 20210531 (Red Hat 11.1.1-3) x86-64. + * tests/test-memrchr.c: Disable -Wmaybe-uninitialized. + explicit_bzero-tests: pacify GCC Redo to pacify -Wmaybe-uninitialized with GCC 11.1.1 20210531 (Red Hat 11.1.1-3) x86-64. diff --git a/tests/test-memrchr.c b/tests/test-memrchr.c index 24719ffb9..9df55c158 100644 --- a/tests/test-memrchr.c +++ b/tests/test-memrchr.c @@ -27,6 +27,11 @@ SIGNATURE_CHECK (memrchr, void *, (void const *, int, size_t)); #include "zerosize-ptr.h" #include "macros.h" +/* Work around GCC bug 101494. */ +#if 4 < __GNUC__ + (3 <= __GNUC_MINOR__) +# pragma GCC diagnostic ignored "-Wmaybe-uninitialized" +#endif + /* Calculating void * + int is not portable, so this wrapper converts to char * to make the tests easier to write. */ #define MEMRCHR (char *) memrchr -- 2.31.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] explicit_bzero-tests: pacify GCC 2021-07-18 4:56 [PATCH 1/2] explicit_bzero-tests: pacify GCC Paul Eggert 2021-07-18 4:56 ` [PATCH 2/2] memrchr-tests: " Paul Eggert @ 2021-07-18 9:12 ` Bruno Haible 2021-07-18 19:14 ` Paul Eggert 1 sibling, 1 reply; 7+ messages in thread From: Bruno Haible @ 2021-07-18 9:12 UTC (permalink / raw) To: bug-gnulib; +Cc: Paul Eggert Hi Paul, > diff --git a/tests/test-explicit_bzero.c b/tests/test-explicit_bzero.c > index cdb839245..c42aba93f 100644 > --- a/tests/test-explicit_bzero.c > +++ b/tests/test-explicit_bzero.c > @@ -126,12 +126,12 @@ test_heap (void) > /* There are two passes: > 1. Put a secret in memory and invoke explicit_bzero on it. > 2. Verify that the memory has been erased. > - Implement them in the same function, so that they access the same memory > - range on the stack. */ > + Access the memory via a volatile pointer, so the compiler > + does not assume the pointer's value and optimize away accesses. */ > +static char *volatile stackbuf; > static int _GL_ATTRIBUTE_NOINLINE > do_secret_stuff (volatile int pass) > { > - char stackbuf[SECRET_SIZE]; > if (pass == 1) > { > memcpy (stackbuf, SECRET, SECRET_SIZE); I disagree with this change, as it significantly reduces the strength of the test. The purpose of the test is to verify that the compiler does not eliminate a call to explicit_bzero, even if data flow analysis reveals that the stack area is "dead" at the end of the function. With this patch, it was turned into a weaker assertion: namely, that the compiler does not eliminate a call to explicit_bzero if it cannot make inferences about the pointer argument. I would suggest to revert this patch, and instead use a #pragma, like you did in the test-memrchr.c patch. Bruno ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] explicit_bzero-tests: pacify GCC 2021-07-18 9:12 ` [PATCH 1/2] explicit_bzero-tests: " Bruno Haible @ 2021-07-18 19:14 ` Paul Eggert 2021-07-18 22:23 ` Bruno Haible 0 siblings, 1 reply; 7+ messages in thread From: Paul Eggert @ 2021-07-18 19:14 UTC (permalink / raw) To: Bruno Haible; +Cc: bug-gnulib [-- Attachment #1: Type: text/plain, Size: 1277 bytes --] On 7/18/21 4:12 AM, Bruno Haible wrote: > The purpose of the test is to verify that the compiler does not eliminate > a call to explicit_bzero, even if data flow analysis reveals that the stack > area is "dead" at the end of the function. > > With this patch, it was turned into a weaker assertion: namely, that the > compiler does not eliminate a call to explicit_bzero if it cannot make > inferences about the pointer argument. Strictly speaking, neither assertion is weaker than the other. However, I take your point that the patch changes the meaning of the test in an undesirable way. I installed the attached to implement your suggestion. This new diagnostic points out a problem with the test, though. If GCC can determine that memcmp reads uninitialized storage, GCC can optimize away the memcmp and act as if memcmp returns 0 (or any other constant). So the test as it stands is problematic given recent advances in practical compilers. As an aside, I don't understand the discussion of asynchronous signal invocations in that test's commentary. There is no asynchronous signaling in that code. (These points are of course low-priority, as explicit_bzero is documented to be best-effort as opposed to being a guarantee that the information is erased.) [-- Attachment #2: 0001-explicit_bzero-tests-pacify-GCC-better.patch --] [-- Type: text/x-patch, Size: 2797 bytes --] From f9803478355d038aa060d71bdd9eddf2bd43325f Mon Sep 17 00:00:00 2001 From: Paul Eggert <eggert@cs.ucla.edu> Date: Sun, 18 Jul 2021 14:08:56 -0500 Subject: [PATCH] explicit_bzero-tests: pacify GCC better Problem reported by Bruno Haible in: https://lists.gnu.org/r/bug-gnulib/2021-07/msg00039.html * tests/test-explicit_bzero.c: Ignore -Wmaybe-uninitialized. (stackbuf): Remove this static pointer, reverting recent change. (do_secret_stuff, test_stack): Revert these related changes too. --- ChangeLog | 9 +++++++++ tests/test-explicit_bzero.c | 16 +++++++++++----- 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/ChangeLog b/ChangeLog index c50808437..d175c39af 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,12 @@ +2021-07-18 Paul Eggert <eggert@cs.ucla.edu> + + explicit_bzero-tests: pacify GCC better + Problem reported by Bruno Haible in: + https://lists.gnu.org/r/bug-gnulib/2021-07/msg00039.html + * tests/test-explicit_bzero.c: Ignore -Wmaybe-uninitialized. + (stackbuf): Remove this static pointer, reverting recent change. + (do_secret_stuff, test_stack): Revert these related changes too. + 2021-07-17 Paul Eggert <eggert@cs.ucla.edu> memrchr-tests: pacify GCC diff --git a/tests/test-explicit_bzero.c b/tests/test-explicit_bzero.c index c42aba93f..14f0ead2b 100644 --- a/tests/test-explicit_bzero.c +++ b/tests/test-explicit_bzero.c @@ -32,6 +32,12 @@ SIGNATURE_CHECK (explicit_bzero, void, (void *, size_t)); #include "vma-iter.h" #include "macros.h" +/* Suppress GCC warning that do_secret_stuff (2) reads uninitialized + local storage. */ +#if 4 < __GNUC__ + (3 <= __GNUC_MINOR__) +# pragma GCC diagnostic ignored "-Wmaybe-uninitialized" +#endif + #define SECRET "xyzzy1729" #define SECRET_SIZE 9 @@ -126,12 +132,14 @@ test_heap (void) /* There are two passes: 1. Put a secret in memory and invoke explicit_bzero on it. 2. Verify that the memory has been erased. - Access the memory via a volatile pointer, so the compiler - does not assume the pointer's value and optimize away accesses. */ -static char *volatile stackbuf; + Implement them in the same function, so that they access the same memory + range on the stack. That way, the test verifies that the compiler + does not eliminate a call to explicit_bzero, even if data flow analysis + reveals that the stack area is dead at the end of the function. */ static int _GL_ATTRIBUTE_NOINLINE do_secret_stuff (volatile int pass) { + char stackbuf[SECRET_SIZE]; if (pass == 1) { memcpy (stackbuf, SECRET, SECRET_SIZE); @@ -147,8 +155,6 @@ do_secret_stuff (volatile int pass) static void test_stack (void) { - char stack_buffer[SECRET_SIZE]; - stackbuf = stack_buffer; int count = 0; int repeat; -- 2.25.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] explicit_bzero-tests: pacify GCC 2021-07-18 19:14 ` Paul Eggert @ 2021-07-18 22:23 ` Bruno Haible 2021-07-18 23:17 ` Paul Eggert 0 siblings, 1 reply; 7+ messages in thread From: Bruno Haible @ 2021-07-18 22:23 UTC (permalink / raw) To: Paul Eggert; +Cc: bug-gnulib Paul Eggert wrote: > I take your point that the patch changes the meaning of the test in an > undesirable way. I installed the attached to implement your suggestion. Thank you. > This new diagnostic points out a problem with the test, though. If GCC > can determine that memcmp reads uninitialized storage, GCC can optimize > away the memcmp and act as if memcmp returns 0 (or any other constant). > So the test as it stands is problematic given recent advances in > practical compilers. Such compiler optimizations would need to be backed by the standards. Are there initiatives to "outlaw" references to uninitialized storage in recent C or C++ standards? I hope code such as int x; /* uninitialized */ if (!((x & 1) == 0 || (x & 1) == 1)) abort (); will never crash. x & 1 can only be 0 or 1. Tertium not datur. Also, I think the danger is small: GCC does not _know_ that the array is uninitialized. It's only a "maybe uninitialized". If GCC ever infers that it is "certainly uninitialized", we could defeat that through a use of 'volatile', such as int (* volatile memcmp_func) (const void *, const void *, size_t) = memcmp; > As an aside, I don't understand the discussion of asynchronous signal > invocations in that test's commentary. There is no asynchronous > signaling in that code. In some operating systems, interrupt handling uses the stack of the currently executing thread. While this is not the case in Linux [1], it was definitely the case in Atari TOS, and is still probably the case in some embedded OSes for MMU-less CPUs. Bruno [1] https://stackoverflow.com/questions/28759227/which-stack-is-used-by-interrupt-handler-linux ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] explicit_bzero-tests: pacify GCC 2021-07-18 22:23 ` Bruno Haible @ 2021-07-18 23:17 ` Paul Eggert 2021-07-19 0:37 ` Bruno Haible 0 siblings, 1 reply; 7+ messages in thread From: Paul Eggert @ 2021-07-18 23:17 UTC (permalink / raw) To: Bruno Haible; +Cc: bug-gnulib On 7/18/21 5:23 PM, Bruno Haible wrote: > > Such compiler optimizations would need to be backed by the standards. > Are there initiatives to "outlaw" references to uninitialized storage > in recent C or C++ standards? No initiatives are needed, at least for C. Using uninitialized storage is undefined behavior in the current C standard and this has been true ever since C was standardized. I imagine C++ is similar. > > I hope code such as > > int x; /* uninitialized */ > if (!((x & 1) == 0 || (x & 1) == 1)) > abort (); > > will never crash. x & 1 can only be 0 or 1. Tertium not datur. The C standard doesn't guarantee that code will never crash. For example, the standard allows an implementation that uses two's complement but where INT_MIN == -INT_MAX and where the bit pattern 0x80000000 is a trap value (i.e., your program aborts if it reads an int whose machine value is 0x80000000). > GCC does not _know_ that the array > is uninitialized. It's only a "maybe uninitialized". That's what GCC's diagnostic says, yes. But in cases like these GCC actually "knows" that variables are uninitialized and it sometimes optimizes based on this knowledge. For example, for: _Bool f (void) { char *p; return !p; } gcc -O2 (GCC 11.1.1 20210531 (Red Hat 11.1.1-3)) "knows" that P is uninitialized and generates code equivalent to that of: _Bool f (void) { return 1; } That is, GCC optimizes away the access to p's value, which GCC can do because the behavior is undefined. > If GCC ever > infers that it is "certainly uninitialized", we could defeat that > through a use of 'volatile', such as Yes, some use of volatile should do the trick for GCC (which is what my patch did). However, one would still have problems with a debugging implementation, e.g., if GCC ever supports an -fsanitize=uninitialized option that catches use of uninitialized storage. This is all low priority of course. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] explicit_bzero-tests: pacify GCC 2021-07-18 23:17 ` Paul Eggert @ 2021-07-19 0:37 ` Bruno Haible 0 siblings, 0 replies; 7+ messages in thread From: Bruno Haible @ 2021-07-19 0:37 UTC (permalink / raw) To: Paul Eggert; +Cc: bug-gnulib Paul Eggert wrote: > No initiatives are needed, at least for C. Using uninitialized storage > is undefined behavior in the current C standard and this has been true > ever since C was standardized. I imagine C++ is similar. > ... > But in cases like these GCC > actually "knows" that variables are uninitialized and it sometimes > optimizes based on this knowledge. For example, for: > > _Bool f (void) { char *p; return !p; } > > gcc -O2 (GCC 11.1.1 20210531 (Red Hat 11.1.1-3)) "knows" that P is > uninitialized and generates code equivalent to that of: > > _Bool f (void) { return 1; } > > That is, GCC optimizes away the access to p's value, which GCC can do > because the behavior is undefined. Ouch, I seriously underrated this warning. Thanks for correcting me. Indeed, GCC does this optimization already since version 4.3. > > If GCC ever > > infers that it is "certainly uninitialized", we could defeat that > > through a use of 'volatile', such as > > Yes, some use of volatile should do the trick for GCC (which is what my > patch did). However, one would still have problems with a debugging > implementation, e.g., if GCC ever supports an -fsanitize=uninitialized > option that catches use of uninitialized storage. Yes, this test probably fails under 'valgrind'. I don't know a fail-safe workaround. When the purpose is to verify whether a memory region that has gone out-of-scope has been cleared, it necessarily means to access this memory region as if it were uninitialized. Bruno ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-07-19 0:37 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-18 4:56 [PATCH 1/2] explicit_bzero-tests: pacify GCC Paul Eggert 2021-07-18 4:56 ` [PATCH 2/2] memrchr-tests: " Paul Eggert 2021-07-18 9:12 ` [PATCH 1/2] explicit_bzero-tests: " Bruno Haible 2021-07-18 19:14 ` Paul Eggert 2021-07-18 22:23 ` Bruno Haible 2021-07-18 23:17 ` Paul Eggert 2021-07-19 0:37 ` Bruno Haible
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).