diff mbox

Disable single thread optimization for open_memstream

Message ID 5968C08E.60008@arm.com
State New, archived
Headers

Commit Message

Szabolcs Nagy July 14, 2017, 1:01 p.m. UTC 
  Single thread optimization is valid if at thread creation time the
optimization can be disabled.  This is in principle true for all
stream objects that user code can access (and thus needs locking),
using the same internal list as fflush(0) uses.  However in glibc
open_memstream is not on that list (BZ 21735) so the optimization
has to be disabled.

OK with the test?

2017-07-14  Szabolcs Nagy  <szabolcs.nagy@arm.com>
	    Florian Weimer  <fweimer@redhat.com>

	* libio/memstream.c (__open_memstream): Set _IO_FLAGS2_NEED_LOCK.
	* libio/wmemstream.c (open_wmemstream): Likewise.
	* nptl/tst-memstream.c: New.

Comments

Carlos O'Donell July 14, 2017, 2:06 p.m. UTC | #1 
On 07/14/2017 09:01 AM, Szabolcs Nagy wrote:
> Single thread optimization is valid if at thread creation time the
> optimization can be disabled.  This is in principle true for all
> stream objects that user code can access (and thus needs locking),
> using the same internal list as fflush(0) uses.  However in glibc
> open_memstream is not on that list (BZ 21735) so the optimization
> has to be disabled.
> 
> OK with the test?
> 
> 2017-07-14  Szabolcs Nagy  <szabolcs.nagy@arm.com>
> 	    Florian Weimer  <fweimer@redhat.com>
> 
> 	* libio/memstream.c (__open_memstream): Set _IO_FLAGS2_NEED_LOCK.
> 	* libio/wmemstream.c (open_wmemstream): Likewise.
> 	* nptl/tst-memstream.c: New.
> 

OK to checkin if you add a pargraph comment to tst-memstream.c
explaining the test invariants, expectations, and exactly what
is being tested and why.
diff mbox

Patch

diff --git a/libio/memstream.c b/libio/memstream.c
index f83d4a5213..e391efd48a 100644
--- a/libio/memstream.c
+++ b/libio/memstream.c
@@ -96,6 +96,9 @@  __open_memstream (char **bufloc, _IO_size_t *sizeloc)
   new_f->fp.bufloc = bufloc;
   new_f->fp.sizeloc = sizeloc;
 
+  /* Disable single thread optimization.  BZ 21735.  */
+  new_f->fp._sf._sbf._f._flags2 |= _IO_FLAGS2_NEED_LOCK;
+
   return (_IO_FILE *) &new_f->fp._sf._sbf;
 }
 libc_hidden_def (__open_memstream)
diff --git a/libio/wmemstream.c b/libio/wmemstream.c
index 5bc77f52ee..103a760bf5 100644
--- a/libio/wmemstream.c
+++ b/libio/wmemstream.c
@@ -98,6 +98,9 @@  open_wmemstream (wchar_t **bufloc, _IO_size_t *sizeloc)
   new_f->fp.bufloc = bufloc;
   new_f->fp.sizeloc = sizeloc;
 
+  /* Disable single thread optimization.  BZ 21735.  */
+  new_f->fp._sf._sbf._f._flags2 |= _IO_FLAGS2_NEED_LOCK;
+
   return (_IO_FILE *) &new_f->fp._sf._sbf;
 }
 
diff --git a/nptl/Makefile b/nptl/Makefile
index 853da72e74..dd01994d3e 100644
--- a/nptl/Makefile
+++ b/nptl/Makefile
@@ -302,7 +302,7 @@  tests = tst-attr1 tst-attr2 tst-attr3 tst-default-attr \
 			    c89 gnu89 c99 gnu99 c11 gnu11) \
 	tst-bad-schedattr \
 	tst-thread_local1 tst-mutex-errorcheck tst-robust10 \
-	tst-robust-fork tst-create-detached
+	tst-robust-fork tst-create-detached tst-memstream
 
 tests-internal := tst-typesizes tst-rwlock19 tst-sem11 tst-sem12 tst-sem13 \
 		  tst-barrier5 tst-signal7 tst-mutex8 tst-mutex8-static \
diff --git a/nptl/tst-memstream.c b/nptl/tst-memstream.c
new file mode 100644
index 0000000000..335c6ed638
--- /dev/null
+++ b/nptl/tst-memstream.c
@@ -0,0 +1,91 @@ 
+/* Test for open_memstream locking.
+   Copyright (C) 2017 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <pthread.h>
+#include <stdio.h>
+#include <support/check.h>
+#include <support/xthread.h>
+
+enum
+{
+  thread_count = 2,
+  byte_count = 1000000,
+};
+
+struct closure
+{
+  FILE *fp;
+  char b;
+};
+
+static void *
+thread_func (void *closure)
+{
+  struct closure *args = closure;
+
+  for (int i = 0; i < byte_count; ++i)
+    fputc (args->b, args->fp);
+
+  return NULL;
+}
+
+int
+do_test (void)
+{
+  char *buffer = NULL;
+  size_t buffer_length = 0;
+  FILE *fp = open_memstream (&buffer, &buffer_length);
+  if (fp == NULL)
+    FAIL_RET ("open_memstream: %m");
+
+  pthread_t threads[thread_count];
+  struct closure args[thread_count];
+
+  for (int i = 0; i < thread_count; ++i)
+    {
+      args[i].fp = fp;
+      args[i].b = 'A' + i;
+      threads[i] = xpthread_create (NULL, thread_func, args + i);
+    }
+
+  for (int i = 0; i < thread_count; ++i)
+    xpthread_join (threads[i]);
+
+  fclose (fp);
+
+  if (buffer_length != thread_count * byte_count)
+    FAIL_RET ("unexpected number of written bytes: %zu (should be %d)",
+	      buffer_length, thread_count * byte_count);
+
+  size_t counts[thread_count] = { 0, };
+  for (size_t i = 0; i < buffer_length; ++i)
+    {
+      if (buffer[i] < 'A' || buffer[i] >= 'A' + thread_count)
+	FAIL_RET ("written byte at %zu out of range: %d", i, buffer[i] & 0xFF);
+      ++counts[buffer[i] - 'A'];
+    }
+  for (int i = 0; i < thread_count; ++i)
+    if (counts[i] != byte_count)
+      FAIL_RET ("incorrect write count for thread %d: %zu (should be %d)", i,
+		counts[i], byte_count);
+
+  return 0;
+}
+
+#define TIMEOUT 100
+#include <support/test-driver.c>