diff mbox

[committed] Disable single thread optimization for open_memstream

Message ID 5968DFCD.3000007@arm.com
State New
Headers show

Commit Message

Szabolcs Nagy July 14, 2017, 3:14 p.m. UTC 
On 14/07/17 15:06, Carlos O'Donell wrote:
> 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.
> 

committed.
diff mbox

Patch

diff --git a/ChangeLog b/ChangeLog
index e2605cb9c3..96c76f72ec 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,10 @@ 
+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.
+
 2017-07-12  Jiong Wang  <jiong.wang@arm.com>
 
 	* sysdeps/arm/dl-machine.h (elf_machine_load_address):  Also strip bit 0
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..c88f708a50
--- /dev/null
+++ b/nptl/tst-memstream.c
@@ -0,0 +1,101 @@ 
+/* 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/>.  */
+
+/* This test checks if concurrent writes to a FILE created with
+   open_memstream are correctly interleaved without loss or corruption
+   of data.  Large number of concurrent fputc operations are used
+   and in the end the bytes written to the memstream buffer are
+   counted to see if they all got recorded.
+
+   This is a regression test to see if the single threaded stdio
+   optimization broke multi-threaded open_memstream usage.  */
+
+#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);
+
+  /* Verify that each thread written its unique character byte_count times.  */
+  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>