Re: GCC 4.4.1 bug? need help isolating problem

[Corey Hickey <bugfood-ml@xxxxxxxxxx>]

Andrew Haley wrote:
>> gcc-4.4 -g  -I. -c -o libmpdemux/demux_mkv.o libmpdemux/demux_mkv.c -O1
>> -fgcse -finline-small-functions -fschedule-insns2 -fstrict-aliasing
>> -finline-functions && make
> 
> If a program compiles correctly with -fno-strict-aliasing and fails with
> -fno-strict-aliasing it almost certainly means that your program has an
> aliasing bug somewhere.
> 
> gcc now has -Wstrict-aliasing, which will catch some but not all aliasing
> errors.  Try it and see if you get any warnings.

Excellent, I think you gave me enough of a hint to find the problem.
-Wstrict-aliasing doesn't complain, but I read the manual and tried
-Wstrict-aliasing=2:

libmpdemux/demux_mkv.c:241: warning: dereferencing type-punned pointer
might break strict-aliasing rules

The line it complains about is:

grow_array((void **)&mkv_d->cluster_positions, mkv_d->num_cluster_pos,
    sizeof(uint64_t));

Would that indeed be a bug of the sort to which you refer? I tried
getting rid of the (void **) cast and that made the segfault go away;
see the attached patch. I'd just like to be sure I found the bug and
didn't just hide it.

Thanks,
Corey

Index: libmpdemux/demux_mkv.c
===================================================================
--- libmpdemux/demux_mkv.c	(revision 29731)
+++ libmpdemux/demux_mkv.c	(working copy)
@@ -203,16 +203,25 @@
 extern int dvdsub_id;
 
 /**
- * \brief ensures there is space for at least one additional element
+ * \brief ensures there is space for at least one additional uint64_t
  * \param array array to grow
  * \param nelem current number of elements in array
- * \param elsize size of one array element
  */
-static void grow_array(void **array, int nelem, size_t elsize) {
+static void grow_array_uint64(uint64_t **array, int nelem) {
   if (!(nelem & 31))
-    *array = realloc(*array, (nelem + 32) * elsize);
+    *array = realloc(*array, (nelem + 32) * sizeof(uint64_t));
 }
 
+/**
+ * \brief ensures there is space for at least one additional mkv_index_t
+ * \param array array to grow
+ * \param nelem current number of elements in array
+ */
+static void grow_array_mkv_index(mkv_index_t **array, int nelem) {
+  if (!(nelem & 31))
+    *array = realloc(*array, (nelem + 32) * sizeof(mkv_index_t));
+}
+
 static mkv_track_t *
 demux_mkv_find_track_by_num (mkv_demuxer_t *d, int n, int type)
 {
@@ -235,8 +244,7 @@
     if (mkv_d->cluster_positions[i] == position)
       return;
 
-  grow_array((void **)&mkv_d->cluster_positions, mkv_d->num_cluster_pos,
-             sizeof(uint64_t));
+  grow_array_uint64(&mkv_d->cluster_positions, mkv_d->num_cluster_pos);
   mkv_d->cluster_positions[mkv_d->num_cluster_pos++] = position;
 }
 
@@ -1079,7 +1087,7 @@
       if (time != EBML_UINT_INVALID && track != EBML_UINT_INVALID
           && pos != EBML_UINT_INVALID)
         {
-          grow_array((void **)&mkv_d->indexes, mkv_d->num_indexes, sizeof(mkv_index_t));
+          grow_array_mkv_index(&mkv_d->indexes, mkv_d->num_indexes);
           mkv_d->indexes[mkv_d->num_indexes].tnum = track;
           mkv_d->indexes[mkv_d->num_indexes].timecode = time;
           mkv_d->indexes[mkv_d->num_indexes].filepos =mkv_d->segment_start+pos;