diff mbox series

nptl: Add compiler barrier in nptl/tst-pthread-getattr

Message ID 87blxms3r1.fsf@oldenburg2.str.redhat.com
State New
Headers show
Series nptl: Add compiler barrier in nptl/tst-pthread-getattr | expand

Commit Message

Florian Weimer July 22, 2019, 11:33 a.m. UTC
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.

Comments

Andreas Schwab July 22, 2019, 11:58 a.m. UTC | #1
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?

Andreas.
Florian Weimer July 22, 2019, 1:34 p.m. UTC | #2
* 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.

Florian

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:

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.

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.

diff --git a/nptl/tst-pthread-getattr.c b/nptl/tst-pthread-getattr.c
index a954778767..e3634ea0a7 100644
--- a/nptl/tst-pthread-getattr.c
+++ b/nptl/tst-pthread-getattr.c
@@ -43,7 +43,7 @@ static size_t pagesize;
 
 /* Check if the page in which TARGET lies is accessible.  This will segfault
    if it fails.  */
-static volatile char *
+static volatile uintptr_t
 allocate_and_test (char *target)
 {
   volatile char *mem = (char *) &mem;
@@ -51,7 +51,7 @@ allocate_and_test (char *target)
   mem = alloca ((size_t) (mem - target));
 
   *mem = 42;
-  return mem;
+  return (uintptr_t) mem;
 }
 
 static int
@@ -84,7 +84,6 @@ check_stack_top (void)
 {
   struct rlimit stack_limit;
   void *stackaddr;
-  volatile void *mem;
   size_t stacksize = 0;
   int ret;
   uintptr_t pagemask = ~(pagesize - 1);
@@ -130,14 +129,14 @@ 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);
+  uintptr_t 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))
+  if (((uintptr_t) stackaddr & pagemask) != (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);
+	      (uintptr_t) stackaddr & pagemask, mem & pagemask);
 
       return 1;
     }
Andreas Schwab July 22, 2019, 2:03 p.m. UTC | #3
On Jul 22 2019, Florian Weimer <fweimer@redhat.com> wrote:

> 	* nptl/tst-pthread-getattr.c (allocate_and_test): Change return
> 	type to uintptr_t.
> 	(check_stack_top): Adjust.

Ok.

Andreas.
Carlos O'Donell July 29, 2019, 7:27 p.m. UTC | #4
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
---
Florian Weimer July 29, 2019, 8:41 p.m. UTC | #5
* Carlos O'Donell:

>> 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 think printf might fault because of its own stack usage (which can
easily exceed half a page in some cases).

Thanks,
Florian
Carlos O'Donell July 30, 2019, 1:49 a.m. UTC | #6
On 7/29/19 4:41 PM, Florian Weimer wrote:
> * Carlos O'Donell:
> 
>>> 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 think printf might fault because of its own stack usage (which can
> easily exceed half a page in some cases).

Good point.

I approve the original commit if you extend the comment to this:

diff --git a/nptl/tst-pthread-getattr.c b/nptl/tst-pthread-getattr.c
index a954778767..50a324cc68 100644
--- a/nptl/tst-pthread-getattr.c
+++ b/nptl/tst-pthread-getattr.c
@@ -41,8 +41,10 @@
  
  static size_t pagesize;
  
-/* Check if the page in which TARGET lies is accessible.  This will segfault
-   if it fails.  */
+/* Test that the page in which TARGET lies is accessible.  This will
+   segfault if the write fails.  This function has only half a page
+   of thread stack left and so should not do anything and immediately
+   return the address to which the stack reached.  */
  static volatile char *
  allocate_and_test (char *target)
  {
---
Andreas Schwab July 30, 2019, 7:08 a.m. UTC | #7
On Jul 29 2019, Florian Weimer <fweimer@redhat.com> wrote:

> * Carlos O'Donell:
>
>>> 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 think printf might fault because of its own stack usage (which can
> easily exceed half a page in some cases).

Then return only the value of the same-page check and print it in the
parent.

Andreas.
Florian Weimer July 30, 2019, 7:11 a.m. UTC | #8
* Andreas Schwab:

> On Jul 29 2019, Florian Weimer <fweimer@redhat.com> wrote:
>
>> * Carlos O'Donell:
>>
>>>> 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 think printf might fault because of its own stack usage (which can
>> easily exceed half a page in some cases).
>
> Then return only the value of the same-page check and print it in the
> parent.

Sorry, what do you mean?  I think the write location is printed for
diagnostics.  And my patch turns this into uintptr_t, suppressing the
compiler warning.

Thanks,
Florian
Andreas Schwab July 30, 2019, 7:30 a.m. UTC | #9
On Jul 30 2019, Florian Weimer <fweimer@redhat.com> wrote:

> * Andreas Schwab:
>
>> On Jul 29 2019, Florian Weimer <fweimer@redhat.com> wrote:
>>
>>> * Carlos O'Donell:
>>>
>>>>> 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 think printf might fault because of its own stack usage (which can
>>> easily exceed half a page in some cases).
>>
>> Then return only the value of the same-page check and print it in the
>> parent.
>
> Sorry, what do you mean?

If you don't want to print it in allocate_and_test, print it in main.

Andreas.
Florian Weimer July 30, 2019, 7:50 a.m. UTC | #10
* Andreas Schwab:

> On Jul 30 2019, Florian Weimer <fweimer@redhat.com> wrote:
>
>> * Andreas Schwab:
>>
>>> On Jul 29 2019, Florian Weimer <fweimer@redhat.com> wrote:
>>>
>>>> * Carlos O'Donell:
>>>>
>>>>>> 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 think printf might fault because of its own stack usage (which can
>>>> easily exceed half a page in some cases).
>>>
>>> Then return only the value of the same-page check and print it in the
>>> parent.
>>
>> Sorry, what do you mean?
>
> If you don't want to print it in allocate_and_test, print it in main.

Do you mean to print it in check_stack_top?  That's what the current
code does.

Thanks,
Florian
Andreas Schwab July 30, 2019, 8 a.m. UTC | #11
On Jul 30 2019, Florian Weimer <fweimer@redhat.com> wrote:

> * Andreas Schwab:
>
>> On Jul 30 2019, Florian Weimer <fweimer@redhat.com> wrote:
>>
>>> * Andreas Schwab:
>>>
>>>> On Jul 29 2019, Florian Weimer <fweimer@redhat.com> wrote:
>>>>
>>>>> * Carlos O'Donell:
>>>>>
>>>>>>> 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 think printf might fault because of its own stack usage (which can
>>>>> easily exceed half a page in some cases).
>>>>
>>>> Then return only the value of the same-page check and print it in the
>>>> parent.
>>>
>>> Sorry, what do you mean?
>>
>> If you don't want to print it in allocate_and_test, print it in main.
>
> Do you mean to print it in check_stack_top?

Yes, the caller of allocate_and_test.  But compute the check in
allocate_and_test, so you don't need to return a handle to the memory
that goes out of scope.

Andreas.
Florian Weimer July 30, 2019, 8:08 a.m. UTC | #12
* Andreas Schwab:

> On Jul 30 2019, Florian Weimer <fweimer@redhat.com> wrote:
>
>> * Andreas Schwab:
>>
>>> On Jul 30 2019, Florian Weimer <fweimer@redhat.com> wrote:
>>>
>>>> * Andreas Schwab:
>>>>
>>>>> On Jul 29 2019, Florian Weimer <fweimer@redhat.com> wrote:
>>>>>
>>>>>> * Carlos O'Donell:
>>>>>>
>>>>>>>> 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 think printf might fault because of its own stack usage (which can
>>>>>> easily exceed half a page in some cases).
>>>>>
>>>>> Then return only the value of the same-page check and print it in the
>>>>> parent.
>>>>
>>>> Sorry, what do you mean?
>>>
>>> If you don't want to print it in allocate_and_test, print it in main.
>>
>> Do you mean to print it in check_stack_top?
>
> Yes, the caller of allocate_and_test.  But compute the check in
> allocate_and_test, so you don't need to return a handle to the memory
> that goes out of scope.

But we need that uintptr_t value to print the diagnostic anyway.  I do
not see what we gain if we move the check of the value into
allocate_and_test.  If printing the value is fine according to the C
semantics, using it in computations should be defined, too.

Thanks,
Florian
Andreas Schwab July 30, 2019, 8:12 a.m. UTC | #13
On Jul 30 2019, Florian Weimer <fweimer@redhat.com> wrote:

> But we need that uintptr_t value to print the diagnostic anyway.  I do
> not see what we gain if we move the check of the value into
> allocate_and_test.  If printing the value is fine according to the C
> semantics, using it in computations should be defined, too.

Then I guess it was a dumb suggestion.  Sorry.

Andreas.
Florian Weimer July 30, 2019, 8:28 a.m. UTC | #14
* Andreas Schwab:

> On Jul 30 2019, Florian Weimer <fweimer@redhat.com> wrote:
>
>> But we need that uintptr_t value to print the diagnostic anyway.  I do
>> not see what we gain if we move the check of the value into
>> allocate_and_test.  If printing the value is fine according to the C
>> semantics, using it in computations should be defined, too.
>
> Then I guess it was a dumb suggestion.  Sorry.

No problem, I will use the second patch with Carlos' comment update.

Thanks,
Florian
Carlos O'Donell July 30, 2019, 1:34 p.m. UTC | #15
On 7/30/19 4:12 AM, Andreas Schwab wrote:
> On Jul 30 2019, Florian Weimer <fweimer@redhat.com> wrote:
> 
>> But we need that uintptr_t value to print the diagnostic anyway.  I do
>> not see what we gain if we move the check of the value into
>> allocate_and_test.  If printing the value is fine according to the C
>> semantics, using it in computations should be defined, too.
> 
> Then I guess it was a dumb suggestion.  Sorry.

I don't think it's a dumb suggestion.

It is a pragmatic suggestion, and that's good.

There is the set of things you are allowed to do, and when that set of
operations comes close, semantically speaking, to things which users are
not allowed to do, the compiler is may issue warnings and those
warnings may not be entirely accurate and that could be on purpose.

So while it is fine in theory to return the value of the address and
use it in computations, the compiler may warn again on this kind of
behaviour because it's trying to catch uses of that address as a pointer
and it may give false positives.

Both Andreas and my suggestion are intended to avoid this future
possibility.

Having said that, the only thing I care about *today* is that gcc 10
can be used to build glibc without error. So I want Florian's patch,
which is tested, to go in right now (with an additional comment).
diff mbox series

Patch

diff --git a/nptl/tst-pthread-getattr.c b/nptl/tst-pthread-getattr.c
index a954778767..a11a25437d 100644
--- a/nptl/tst-pthread-getattr.c
+++ b/nptl/tst-pthread-getattr.c
@@ -41,6 +41,15 @@ 
 
 static size_t pagesize;
 
+/* The weak attribute marks this function as interposable, so the
+   compiler cannot assume that the pointer remains unchanged.  */
+__attribute__ ((noinline, noclone,  weak))
+volatile char *
+compiler_barrier (volatile char *ptr)
+{
+  return ptr;
+}
+
 /* Check if the page in which TARGET lies is accessible.  This will segfault
    if it fails.  */
 static volatile char *
@@ -51,7 +60,10 @@  allocate_and_test (char *target)
   mem = alloca ((size_t) (mem - target));
 
   *mem = 42;
-  return mem;
+
+  /* Insert the compiler barrier to hide the fact that mem is
+     stack-allocated.  The address is only used in a diagnostic.  */
+  return compiler_barrier (mem);
 }
 
 static int