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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.06.19 at 09:36, <chenbaodong@xxxxxxxxxx> wrote:

> On 6/12/19 14:34, Jan Beulich wrote:
>>>>> On 12.06.19 at 02:23, <chenbaodong@xxxxxxxxxx> wrote:
>>> On 6/11/19 22:03, Jan Beulich wrote:
>>>>>>> On 11.06.19 at 08:02, <chenbaodong@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel