Re: regression in multi-threaded git-pack-index

Jeff King [off-list ref] writes:

On Tue, Mar 19, 2013 at 06:08:00AM -0400, Jeff King wrote:

quoted

@@ -538,6 +539,8 @@ static void resolve_delta(struct object_entry *delta_obj,
 
 	delta_obj->real_type = base->obj->real_type;
 	delta_obj->delta_depth = base->obj->delta_depth + 1;
+	if (deepest_delta < delta_obj->delta_depth)
+		deepest_delta = delta_obj->delta_depth;
 	delta_obj->base_object_no = base->obj - objects;
 	delta_data = get_data_from_pack(delta_obj);
 	base_data = get_base_data(base);

and valgrind reports an uninitialized value in the conditional. But we
can see that deepest_delta is static, and therefore always has some
value. And delta_obj->delta_depth is set in the line above. So both
should have some known value, unless they are computed from unknown
values. In that case, shouldn't valgrind have previously noticed when we
accessed those unknown values?

Ah, indeed. Putting:

  fprintf(stderr, "%lu\n", base->obj->delta_depth);

before the conditional reveals that base->obj->delta_depth is
uninitialized, which is the real problem. I'm sure there is some
perfectly logical explanation for why valgrind can't detect its use
during the assignment, but I'm not sure what it is.

That's simply because you would get far too much noise.  It only reports
an uninitialized value when it actually gets used in a conditional or
for output (syscalls), which is when they matter.

You can use --track-origins=yes to see where the undefined value came
from, but it's veeeery slow.

It looks like the delta_depth value was
introduced in 38a4556 (index-pack: start learning to emulate
"verify-pack -v", 2011-06-03), and it used only for showing the chain
depths with --verify-stat. So it is almost certainly not related to
Stefan's original problem, but it does mean we've probably been
computing bogus chain lengths.

Nice catch!

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

On Tue, Mar 19, 2013 at 11:29:36AM +0100, Thomas Rast wrote:

quoted

Ah, indeed. Putting:

  fprintf(stderr, "%lu\n", base->obj->delta_depth);

before the conditional reveals that base->obj->delta_depth is
uninitialized, which is the real problem. I'm sure there is some
perfectly logical explanation for why valgrind can't detect its use
during the assignment, but I'm not sure what it is.

That's simply because you would get far too much noise.  It only reports
an uninitialized value when it actually gets used in a conditional or
for output (syscalls), which is when they matter.

Would it? I would think any computation you start with an undefined
value would be suspect (and you would want to know about it as soon as
possible, before the tainted value gets output). I was assuming it was a
performance issue or something.

You can use --track-origins=yes to see where the undefined value came
from, but it's veeeery slow.

It's pretty slow either way. :) I do have --track-origins on, but the
origin is this:

  objects = xrealloc(objects,
                     (nr_objects + nr_unresolved + 1)
                     * sizeof(*objects));

which is not very helpful. It's _somewhere_ in the list of objects...:)

-Peff