diff mbox

Fix valgrind checking bootstrap with PCH (PR middle-end/56461)

Message ID 20130301201616.GG12913@tucnak.redhat.com
State New
Headers show

Commit Message

Jakub Jelinek March 1, 2013, 8:16 p.m. UTC 
Hi!

When I've tried --enable-checking=valgrind bootstrap recently, it failed
as soon as compiling first PCH header.  The problem is that in
ggc_internal_alloc_stat we often increase the requested size through
ggc_round_alloc_size_1 (size, &order, &object_size);
and there is no way to query the actual size, so for PCH saving all we
know is the larger object_size.  From size to object_size the memory
is marked as unaccessible for valgrind though:
  /* Make the bytes after the end of the object unaccessible.  Discard the
     handle to avoid handle leak.  */
  VALGRIND_DISCARD (VALGRIND_MAKE_MEM_NOACCESS ((char *) result + size,
                                                object_size - size));
thus when we're trying to save it into the PCH file (sure, we'll store
there usually the 0xaf bytes), we hit tons of valgrind errors.

Fixed by making the memory accessible again (and fully defined) for the
actual writing to PCH, and then restoring the previous state.

Also, I've plugged two memory leaks in PCH writing.

Bootstrapped/regtested on x86_64-linux and i686-linux, tested also with
valgrind checking on various testcases (full --enable-checking=yes,valgrind
bootstrap queued for the weekend).

Ok for trunk?

2013-03-01  Jakub Jelinek  <jakub@redhat.com>

	PR middle-end/56461
	* ggc-common.c (gt_pch_save): For ENABLE_VALGRIND_CHECKING,
	if VALGRIND_GET_VBITS is defined, temporarily make object
	memory all defined, and restore previous valgrind addressability
	and definability afterwards.  Free this_object at the end.

	* c-pch.c (pch_init): Free target_validity at the end.


	Jakub

Comments

Jakub Jelinek March 4, 2013, 12:30 p.m. UTC | #1 
On Fri, Mar 01, 2013 at 09:16:16PM +0100, Jakub Jelinek wrote:
> Bootstrapped/regtested on x86_64-linux and i686-linux, tested also with
> valgrind checking on various testcases (full --enable-checking=yes,valgrind
> bootstrap queued for the weekend).

FYI, with this patch (first time I remember) the valgrind checking
bootstrap actually succeeded on x86_64-linux.
../configure --enable-languages=all,obj-c++,lto --enable-checking=yes,valgrind
make -j16 STAGE1_CFLAGS='-g -O2' STAGE1_CXXFLAGS='-g -O2' > LOG 2>&1 && make -j16 -k check > LOGC 2>&1; ../contrib/test_summary > LOGT 2>&1

Bootstrap took a few minutes under 24 hours, regtest a few minutes under 40
hours.

During regtesting some failures were seen (smaller amount of them
timeouts because I haven't increased the timeouts, bigger amounts because of
various valgrind errors, I'll go through them soon).
Compared to non-valgrind checking bootstrap the difference was:

                === gcc Summary ===
 
-# of expected passes           95837
-# of unexpected failures       32
+# of expected passes           94093
+# of unexpected failures       287
 # of unexpected successes      44
 # of expected failures         266
+# of unresolved testcases      26
 # of unsupported tests         1398

                === gfortran Summary ===
 
-# of expected passes           43273
-# of unexpected failures       6
+# of expected passes           43227
+# of unexpected failures       48
 # of unexpected successes      2
 # of expected failures         48
-# of unresolved testcases      6
+# of unresolved testcases      10
 # of unsupported tests         61

                === g++ Summary ===
 
-# of expected passes           53896
+# of expected passes           52584
+# of unexpected failures       84
 # of expected failures         290
+# of unresolved testcases      1
 # of unsupported tests         832

                === objc Summary ===
 
-# of expected passes           2988
+# of expected passes           2985
+# of unexpected failures       3
 # of expected failures         6
 # of unsupported tests         74

                === libstdc++ Summary ===
 
-# of expected passes           9246
+# of expected passes           9237
+# of unexpected failures       7
 # of expected failures         45
+# of unresolved testcases      2
 # of unsupported tests         212

> 2013-03-01  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR middle-end/56461
> 	* ggc-common.c (gt_pch_save): For ENABLE_VALGRIND_CHECKING,
> 	if VALGRIND_GET_VBITS is defined, temporarily make object
> 	memory all defined, and restore previous valgrind addressability
> 	and definability afterwards.  Free this_object at the end.
> 
> 	* c-pch.c (pch_init): Free target_validity at the end.

	Jakub
diff mbox

Patch

--- gcc/ggc-common.c.jj	2013-01-24 17:04:30.000000000 +0100
+++ gcc/ggc-common.c	2013-03-01 16:32:24.547261238 +0100
@@ -561,6 +561,10 @@  gt_pch_save (FILE *f)
 
   ggc_pch_prepare_write (state.d, state.f);
 
+#if defined ENABLE_VALGRIND_CHECKING && defined VALGRIND_GET_VBITS
+  vec<char> vbits = vNULL;
+#endif
+
   /* Actually write out the objects.  */
   for (i = 0; i < state.count; i++)
     {
@@ -569,6 +573,50 @@  gt_pch_save (FILE *f)
 	  this_object_size = state.ptrs[i]->size;
 	  this_object = XRESIZEVAR (char, this_object, this_object_size);
 	}
+#if defined ENABLE_VALGRIND_CHECKING && defined VALGRIND_GET_VBITS
+      /* obj might contain uninitialized bytes, e.g. in the trailing
+	 padding of the object.  Avoid warnings by making the memory
+	 temporarily defined and then restoring previous state.  */
+      int get_vbits = 0;
+      size_t valid_size = state.ptrs[i]->size;
+      if (__builtin_expect (RUNNING_ON_VALGRIND, 0))
+	{
+	  if (vbits.length () < valid_size)
+	    vbits.safe_grow (valid_size);
+	  get_vbits = VALGRIND_GET_VBITS (state.ptrs[i]->obj,
+					  vbits.address (), valid_size);
+	  if (get_vbits == 3)
+	    {
+	      /* We assume that first part of obj is addressable, and
+		 the rest is unaddressable.  Find out where the boundary is
+		 using binary search.  */
+	      size_t lo = 0, hi = valid_size;
+	      while (hi > lo)
+		{
+		  size_t mid = (lo + hi) / 2;
+		  get_vbits = VALGRIND_GET_VBITS ((char *) state.ptrs[i]->obj
+						  + mid, vbits.address (),
+						  1);
+		  if (get_vbits == 3)
+		    hi = mid;
+		  else if (get_vbits == 1)
+		    lo = mid + 1;
+		  else
+		    break;
+		}
+	      if (get_vbits == 1 || get_vbits == 3)
+		{
+		  valid_size = lo;
+		  get_vbits = VALGRIND_GET_VBITS (state.ptrs[i]->obj,
+						  vbits.address (),
+						  valid_size);
+		}
+	    }
+	  if (get_vbits == 1)
+	    VALGRIND_DISCARD (VALGRIND_MAKE_MEM_DEFINED (state.ptrs[i]->obj,
+							 state.ptrs[i]->size));
+	}
+#endif
       memcpy (this_object, state.ptrs[i]->obj, state.ptrs[i]->size);
       if (state.ptrs[i]->reorder_fn != NULL)
 	state.ptrs[i]->reorder_fn (state.ptrs[i]->obj,
@@ -582,11 +630,29 @@  gt_pch_save (FILE *f)
 			    state.ptrs[i]->note_ptr_fn == gt_pch_p_S);
       if (state.ptrs[i]->note_ptr_fn != gt_pch_p_S)
 	memcpy (state.ptrs[i]->obj, this_object, state.ptrs[i]->size);
+#if defined ENABLE_VALGRIND_CHECKING && defined VALGRIND_GET_VBITS
+      if (__builtin_expect (get_vbits == 1, 0))
+	{
+	  (void) VALGRIND_SET_VBITS (state.ptrs[i]->obj, vbits.address (),
+				     valid_size);
+	  if (valid_size != state.ptrs[i]->size)
+	    VALGRIND_DISCARD (VALGRIND_MAKE_MEM_NOACCESS ((char *)
+							  state.ptrs[i]->obj
+							  + valid_size,
+							  state.ptrs[i]->size
+							  - valid_size));
+	}
+#endif
     }
+#if defined ENABLE_VALGRIND_CHECKING && defined VALGRIND_GET_VBITS
+  vbits.release ();
+#endif
+
   ggc_pch_finish (state.d, state.f);
   gt_pch_fixup_stringpool ();
 
-  free (state.ptrs);
+  XDELETE (state.ptrs);
+  XDELETE (this_object);
   htab_delete (saving_htab);
 }
 
--- gcc/c-family/c-pch.c.jj	2013-02-13 23:48:14.000000000 +0100
+++ gcc/c-family/c-pch.c	2013-03-01 16:29:54.001422781 +0100
@@ -141,6 +141,8 @@  pch_init (void)
 
   if (pch_ready_to_save_cpp_state)
     pch_cpp_save_state ();
+
+  XDELETE (target_validity);
 }
 
 /* Whether preprocessor state has been saved in a PCH file.  */