Re: [Xen-devel] [PATCH] xen/coverage: wrap coverage related things under 'CONFIG_COVERAGE'

From: "Jan Beulich" <JBeulich@suse.com>
To: "Baodong Chen" <chenbaodong@mxnavi.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>, WeiLiu <wl@xen.org>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Ian Jackson <Ian.Jackson@eu.citrix.com>, Tim Deegan <tim@xen.org>,
	Julien Grall <julien.grall@arm.com>,
	xen-devel <xen-devel@lists.xenproject.org>,
	Roger Pau Monne <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH] xen/coverage: wrap coverage related things under 'CONFIG_COVERAGE'
Date: Wed, 12 Jun 2019 01:58:39 -0600	[thread overview]
Message-ID: <5D00B0AF020000780023757D@prv1-mh.provo.novell.com> (raw)
In-Reply-To: <4deea016-0fa9-0bf1-f570-9c0dd6679761@mxnavi.com>

>>> On 12.06.19 at 09:36, <chenbaodong@mxnavi.com> wrote:

> On 6/12/19 14:34, Jan Beulich wrote:
>>>>> On 12.06.19 at 02:23, <chenbaodong@mxnavi.com> wrote:
>>> On 6/11/19 22:03, Jan Beulich wrote:
>>>>>>> On 11.06.19 at 08:02, <chenbaodong@mxnavi.com> wrote:
>>>>> --- a/xen/arch/x86/xen.lds.S
>>>>> +++ b/xen/arch/x86/xen.lds.S
>>>>> @@ -240,12 +240,14 @@ SECTIONS
>>>>>            *(.altinstructions)
>>>>>            __alt_instructions_end = .;
>>>>>    
>>>>> +#if defined(CONFIG_COVERAGE)
>>>>>           . = ALIGN(8);
>>>>>           __ctors_start = .;
>>>>>           *(.ctors)
>>>>>           *(.init_array)
>>>>>           *(SORT(.init_array.*))
>>>>>           __ctors_end = .;
>>>>> +#endif
>>>> How is this (only) coverage related? And how is making this conditional
>>>> going to help in any way?
>>> Hello Jan,
>>>
>>> When i read the code 'init_constructors()', i want to understand when
>>> it's used.
>>>
>>> I can not find any helper macros like '__init' in init.h, put things in
>>> this section.
>>>
>>> Also run under arm foundation platform, the section is empty.
>>>
>>> So i check commit history and found it's commit logs: it is coverage
>>> related.
>>>
>>> And compiled with CONFIG_COVERAGE enabled, this section is not empty
>>> anymore.
>>>
>>> So the patch mainly want to clarify the code is coverage related,
>>>
>>> which want to help newcomer easily understand this code.
>>>
>>> Am i misunderstanding here?
>> The code may have been _introduced_ for coverage, but are you
>> willing to guarantee it's coverage-only? Plus - what does removing
>> an empty section buy you?
> 
> Currently seems true, but not sure about the future, can not guarantee.
> 
> Personally guess this should not be used by xen, but use __init_call(fn) 
> like in init.h.
> 
> My purpose is to clarify the code is coverage related(at least currently 
> is).
> 
> If you are unhappy with it this way, how about just add a comment, 
> something like:
> 
> +/* currently only used by code coverage */
>    void __init init_constructors(void)

I'd prefer if the entire patch was dropped, unless there really was
a clear (and clearly spelled out) gain. Adding a comment like you
suggest only calls for it going stale at some point.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel