[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:       Philippe Waroquiers <bugzilla_noreply () kde ! org>
Date:       2016-11-01 22:29:37
Message-ID: bug-371916-17878-pRuh0jiUlR () http ! bugs ! kde ! org/
[Download RAW message or body]

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

--- Comment #5 from Philippe Waroquiers <philippe.waroquiers@skynet.be> ---
Thanks for the comments, here is some feedback.
I will attach a new patch version, with an updated pub_tool_xtree.h.

(In reply to Ivo Raisr from comment #3)
> Thank you for this patch, Philippe.
> I was concentrating on the interface pub_tool_xtree.h in the first run and
> here are my questions and comments:
> 
> 1. void*(*alloc_fn)(const HChar*, SizeT) in VG_(newXT)().
> What is 'const HChar *' used for? I can imagine 'SizeT' is for the size.
> Perhaps if you can name the arguments, it would be clearer? Or you can give
> a hint that VG_(malloc)() and VG_(free)() are typically used for alloc_fn
> and free_fn?
This uses the classical alloc_fn and free_fn convention, used also
in other modules. See e.g. pub_tool_xarray.h or pub_tool_rangemap.h.
I agree that it would be more readable to have parameter names or
possibly/preferably we should define function types e.g. in
pub_tool_basics.h.
I have added this to my TODO list, to rework all occurences of alloc_fn/free_fn

> 
> 2. What is 'ec' and 'IP' as referred to in VG_(newXT)()'? It is not
> explained in the file header.
IP is Instruction Pointer, ec is exe context.
I have added some more explanation in the PURPOSE comment at the top. 

> 
> 3. What is argument 'cc' used for in VG_(newXT)()?
Classical 'cost centre' for modules that are allocating memory.
I have added a comment line to clarify cc is allocation cost centre.

> 
> 4. Functions in this header file use a mixture of camelCase and
> all_lower_case which is inconsistent and  disturbs reader's eyes. Can you do
> something with it?
Humph, that is a very valuable comment, but I am not sure what style
to use. Most of the current code is itself a mixture of upper
case/lower case, VG_(...), ...
For example, in pub_tool_hashtable.h, we find a.o.
 VgHashNode** VG_(HT_to_array)
 void VG_(HT_ResetIter) ( VgHashTable *table );
So, it seems that the convention is that there is no convention, and this
new module follows the 'no convention' convention.
But if we agree on a convention, I can rework this.
(my preference would be to Words_Separated_By_Underscore, this is longer
but IMO more readable and can be supported automatically by re-formatter
tools).

> 
> 5. I wonder why VG_(init_XtAllocs)(), VG_(add_XtAllocs)(),
> VG_(sub_XtAllocs)() and VG_(img_XtAllocs)() do not use structure XtAllocs
> instead of 'void *'? Am I missing something?
These are functions to be used as init_data_fn/add_data_fn/sub_data_fn,
so they match the argument profiles as found in VG_(newXT)

> 
> 6. XtAllocsEvents does not need to be exported.
Fixed.

> 
> 7. For VG_(XtMemoryFull_free)(), does providing ec_alloc brings an
> unnecessary burden on the tool? Perhaps m_xtree.c could cache it?
In fact, the idea is that the tool is itself maintaining the alloc ec,
and so the cpu/memory cost is then neglectible. If m_xtree.c would itself
either maintain or (re-)compute the ec_alloc, then that would bring cpu
and/or memory overhead, and the xtree interface would need to receive
the allocated or block address.

> 
> 8. VG_(XtMemory_report)() talks about xtree-memory=full,
> --xtree-memory=allocs and --xtree-memory=none. How one does set these? 
These are the 3 values for the new command line option --xtree-memory.
I have clarified the comment.

> 
> 9. "typedef void MsFile;" Surely we can do better with implementation
> hiding. Several lines above "typedef struct _XTree  XTree;" was used, so
> what about:
>     typedef struct _MsFile MsFile;
In reality, MsFile is a VgFile, but I do
not want to expose this, so using typedef void MsFile; was hiding it
without introducing an artificial struct _MsFile.
As the user cannot do much with a void, this looks to hide what it is.
Not sure about what to do here.

> 
> 10. I also wonder why callgrind and massif specific functionality is
> contained in generic module xtree? Perhaps it could be located in the tools
> themselves?
These are implementing core functionality to output an xtree in a callgrind
format or a massif format. memcheck/helgrind and massif can now each output
an xtree in massif or callgrind format.
So, this code cannot go into the respective tools, it must be in core.

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