[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-bugs-dist
Subject:    [valgrind] [Bug 371916] execution tree xtree concept
From:       Julian Seward <bugzilla_noreply () kde ! org>
Date:       2016-11-10 18:22:45
Message-ID: bug-371916-17878-81xmjaivOm () http ! bugs ! kde ! org/
[Download RAW message or body]

https://bugs.kde.org/show_bug.cgi?id=371916

--- Comment #18 from Julian Seward <jseward@acm.org> ---
Some comments on the v3 patch:

Mostly it looks fine.  Below [1] some small comments.

Larger comments:

* I assume there are no regressions with correctness or performance
  with this, yes?  Could you do a self-host Memcheck run at some point?

* The large size of the patch concerns me a bit.  I would be happier
  if it could be split into two parts:

  (1) that creates m_xtree.c and refactors Massif to use it, but
      does not change the user-visible functionality at all.  So
      it is an implementation-only change, and

  (2) a patch that builds on (1), that supplies the new functionality.

  Is that possible, if it is not a lot of work?


[1] Small comments:

----------------

+typedef
+   struct { Addr a; const HChar* sym_name; PtrdiffT offset; }
+   Sym_Name_CacheEnt;

I prefer to have something more descriptive than just "a" for the
first field.  Since it is debug-info related, it would also be good to
clarify whether this is an SVMA or an AVMA or something else, per
comment at the top of m_debuginfo/debuginfo.c.  I suspect it's a SVMA,
in which case a good name would by "sym_svma".

----------------

+"    --xtree-memory=none|allocs|full   profile heap memory in an xtree
[none]\n"
+"                              and produces a report at the end of the
execution\n"
+"                              none: no profiling, allocs: current
allocated\n"
+"                              size/blocks, full: profile current and
cumulative\n"
+"                              allocated size/blocks and freed size/blocks.\n"
+"    --xtree-memory-file=<file>   xtree memory report file
[xtmemory.kcg.%%p]\n"

This flag only has effect for tools that replace malloc, correct?  Is
it listed in the section "user options for Valgrind tools that replace
malloc:" ?

----------------

+   This file is part of Valgrind, a dynamic binary instrumentation
+   framework.
+
+   Copyright (C) 2016-2016 Philippe Waroquiers

For m_xtree.c, if there is a lot of code in there which has been moved
from massif and/or callgrind, and is not much changed, I think it
would be diplomatic to add a line or two explaining that the original
authors were Nick and/or Josef.  See the top of coregrind/m_wordfm.c for
an example.

----------------

 // growing such a block, but for consistency (it also simplifies things) we
 // ignore such reallocs as well.
+// XTREE??? why can't we just consider that a realloc of an ignored
+// alloc is just a new alloc (i.e. do not remove the old sz from the stats).

and again later.  The "XTREE???" is confusing -- I don't know what it
signifies.  Can you maybe write instead something like "PW Nov 2016,
xtree work:"?  I do that in comments from time to time.

----------------

Per previous comments in the bug re CamelCase vs snake_case, I really
have no problem mixing them.  I like to use camelcase, with a capital
first letter for type names, and snake case when function names get
long.  But no fixed rules.

-- 
You are receiving this mail because:
You are watching all bug changes.=
[prev in list] [next in list] [prev in thread] [next in thread]