Re: [Xen-devel] [PATCH-0/2] Hypervisor profiling using GCOV (64bit Hypervisor)

[Tej <bewith.tej@xxxxxxxxx>]

On Thu, Mar 26, 2009 at 4:27 PM, Gianluca Guida
<Gianluca.Guida@xxxxxxxxxxxxx> wrote:
> Hi,
>
> Sorry for the late reply.
>
> On Mar 26, 2009, at 7:59 AM, Tej wrote:
>
>> On Mon, Mar 2, 2009 at 9:36 PM, Tej <bewith.tej@xxxxxxxxx> wrote:
>>>
>>> On Sat, Feb 28, 2009 at 12:32 AM, Gianluca Guida
>>> <gianluca.guida@xxxxxxxxxxxxx> wrote:
>>>>
>>>> While I still need to test the patch (building 3.3 right now) and to
>>>> understand gcov internals, I think that a few comments can be done,
>>>> mostly
>>>> aestethicals.
>>>>
>>>> - xen coding style: Using four-spaces tabs is generally the tradition.
>>>> Also
>>>> I do prefer to have brackets that start code blocks on a new line
>>>> aligned to
>>>> the previous line, but that's not followed everywhere in the code.
>>>>
>>>> - Makefiles: while the num=$*.c is still a mystery to me, my question
>>>> is: do
>>>> you really need to make links with different names to files compiled
>>>> multiple times? If so, it would be useful to remove them on 'make
>>>> clean'.
>>>> Also, it would be useful to make this feature enabled with a
>>>> compile-time
>>>> option.
>>>
>>> Attached patches address the four-spaces tabs and make clean issues.
>>> Hope the code is more clean and readable
>>
>> Hi
>>
>> Sorry for interrupting you guys from your busy schedule, as i just
>> want to know the current status for GCOV patches i submitted early
>> this month.
>> any feedback on "testing/coding/design/feature" will be appreciated.
>
> I've been testing it for a while, and it seems an useful and interesting
> feature.

glad to hear this

>
> A few comments, though:
>
> - As I said, while the patch is minimal enough, the feature requires some
> intrusive changes in Xen, like changing the linker script (via the
> definition of CONSTRUCTORS) and in general it changes the compiler behavior,
> so I think it should be mandatory to make this feature explicitely enabled
> at compile time, e.g.  gcov=y.

> - I am talking about Xen code only, but I think a few part of the codes
> should be more clear. I find very confusing the difference between
> xen_gcov_info and gcov_info, and all the instructions that uses them. A
> better naming scheme for the variables and more comments would be
> appreciated.

basically there should not be no difference between xen_gcov_info and
gcov_info, but due to some earlier implementation difficulties we kept
 two different structure. In final implementation those difference
narrow down drastically but we kept the both structures. I will look
back into this.

and All comments will be addressed asap.

thanks
-tej

>
> Thanks!
> Gianluca
>

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel