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

["Jan Beulich" <JBeulich@suse.com>]

>>> 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?

>>> --- a/xen/common/lib.c
>>> +++ b/xen/common/lib.c
>>> @@ -491,15 +491,20 @@ unsigned long long parse_size_and_unit(const char *s, const char **ps)
>>>       return ret;
>>>   }
>>>   
>>> +#if defined(CONFIG_COVERAGE)
>>>   typedef void (*ctor_func_t)(void);
>>>   extern const ctor_func_t __ctors_start[], __ctors_end[];
>>> +#endif
>> Again - how does this help?
> Want to clarify this is coverage related code.

If only it really was (provably).

>>> +/* see 'docs/hypervisor-guide/code-coverage.rst' */
>>>   void __init init_constructors(void)
>> There's no mention of this function in the referenced docs file.
> 
> Same as above.

No. The reference makes no sense here without that doc somehow
mentioning the function you attach the comment to.

>>>   {
>>> +#if defined(CONFIG_COVERAGE)
>>>       const ctor_func_t *f;
>>>       for ( f = __ctors_start; f < __ctors_end; ++f )
>>>           (*f)();
>>>   
>>> +#endif
>>>       /* Putting this here seems as good (or bad) as any other place. */
>> Again, besides lacking suitable reasoning you also should look
>> more closely, in this case where exactly it makes sense to place
>> the #endif.
> 
> The blank line here? If yes, can be removed. i missed this.

Removed? No. If anything there's one missing. You've inserted
the #ifdef after the blank line rather than before it.

Jan



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