This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] nptl: Add compiler barrier in nptl/tst-pthread-getattr
On 7/22/19 9:34 AM, Florian Weimer wrote:
* Andreas Schwab:
On Jul 22 2019, Florian Weimer <fweimer@redhat.com> wrote:
Recent GCC versions warn about the attempt to return the address of a
local variable:
tst-pthread-getattr.c: In function ‘allocate_and_test’:
tst-pthread-getattr.c:54:10: error: function returns address of local variable [-Werror=return-local-addr]
54 | return mem;
| ^~~
In file included from ../include/alloca.h:3,
from tst-pthread-getattr.c:26:
../stdlib/alloca.h:35:23: note: declared here
35 | # define alloca(size) __builtin_alloca (size)
| ^~~~~~~~~~~~~~~~~~~~~~~
tst-pthread-getattr.c:51:9: note: in expansion of macro ‘alloca’
51 | mem = alloca ((size_t) (mem - target));
| ^~~~~~
2019-07-22 Florian Weimer <fweimer@redhat.com>
* nptl/tst-pthread-getattr.c (compiler_barrier): New function.
(allocate_and_test): Use it.
Does it work to change the return type to uintptr_t?
Thanks for the suggestion. I think this is the better fix. Patch
below.
This is still a workaround to something that the logic of the test should
not be doing.
nptl: Use uintptr_t for address diagnostic in nptl/tst-pthread-getattr
Recent GCC versions warn about the attempt to return the address of a
local variable:
Right.
tst-pthread-getattr.c: In function ‘allocate_and_test’:
tst-pthread-getattr.c:54:10: error: function returns address of local variable [-Werror=return-local-addr]
54 | return mem;
| ^~~
In file included from ../include/alloca.h:3,
from tst-pthread-getattr.c:26:
../stdlib/alloca.h:35:23: note: declared here
35 | # define alloca(size) __builtin_alloca (size)
| ^~~~~~~~~~~~~~~~~~~~~~~
tst-pthread-getattr.c:51:9: note: in expansion of macro ‘alloca’
51 | mem = alloca ((size_t) (mem - target));
| ^~~~~~
The address itself is used in a check in the caller, so using
uintptr_t instead is reasonable.
Any reason why we wouldn't move the test into the function?
I appreciate the desire to make a minimal change here, but I think we can
just cleanup the test and not worry about this with any future change and
the test gets cleaner.
2019-07-22 Florian Weimer <fweimer@redhat.com>
* nptl/tst-pthread-getattr.c (allocate_and_test): Change return
type to uintptr_t.
(check_stack_top): Adjust.
e.g. Something like this completely untested patch.
diff --git a/nptl/tst-pthread-getattr.c b/nptl/tst-pthread-getattr.c
index a954778767..9ece3eec6a 100644
--- a/nptl/tst-pthread-getattr.c
+++ b/nptl/tst-pthread-getattr.c
@@ -41,17 +41,31 @@
static size_t pagesize;
-/* Check if the page in which TARGET lies is accessible. This will segfault
- if it fails. */
+/* Two test. First check if the page in which TARGET lies is accessible.
+ This will segfault if the probing write fails. Then test if we actually
+ probed the page we were expecting based on stackaddr and the current
+ pagemask. */
static volatile char *
-allocate_and_test (char *target)
+allocate_and_test (char *target, char *stackaddr, uintptr_t pagemask)
{
volatile char *mem = (char *) &mem;
/* FIXME: mem >= target for _STACK_GROWSUP. */
mem = alloca ((size_t) (mem - target));
+ /* Probe. */
*mem = 42;
- return mem;
+
+ /* Before we celebrate, make sure we actually did test the same page. */
+ if (((uintptr_t) stackaddr & pagemask) != ((uintptr_t) mem & pagemask))
+ {
+ printf ("We successfully wrote into the wrong page.\n"
+ "Expected %#" PRIxPTR ", but got %#" PRIxPTR "\n",
+ (uintptr_t) stackaddr & pagemask, (uintptr_t) mem & pagemask);
+
+ return 1;
+ }
+
+ return 0;
}
static int
@@ -130,21 +144,8 @@ check_stack_top (void)
stack and test access there. It is however sufficient to simply check if
the top page is accessible, so we target our access halfway up the top
page. Thanks Chris Metcalf for this idea. */
- mem = allocate_and_test (stackaddr + pagesize / 2);
-
- /* Before we celebrate, make sure we actually did test the same page. */
- if (((uintptr_t) stackaddr & pagemask) != ((uintptr_t) mem & pagemask))
- {
- printf ("We successfully wrote into the wrong page.\n"
- "Expected %#" PRIxPTR ", but got %#" PRIxPTR "\n",
- (uintptr_t) stackaddr & pagemask, (uintptr_t) mem & pagemask);
-
- return 1;
- }
-
- puts ("Stack top tests done");
-
- return 0;
+ return allocate_and_test (stackaddr + pagesize / 2,
+ stackaddr, pagemask);
}
/* TODO: Similar check for thread stacks once the thread stack sizes are
---
--
Cheers,
Carlos.