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

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

[Baodong Chen]

Constructors between '__ctors_start' and '__ctors_end' only used
for code-coverage, not by xen itself, so use 'CONFIG_COVERAGE' wrap them.

Signed-off-by: Baodong Chen <chenbaodong@mxnavi.com>
---
 xen/arch/arm/xen.lds.S | 2 ++
 xen/arch/x86/xen.lds.S | 2 ++
 xen/common/lib.c       | 5 +++++
 3 files changed, 9 insertions(+)

diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index 1e72906..320ff68 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -172,12 +172,14 @@ SECTIONS
        *(.init.data.rel)
        *(.init.data.rel.*)
 
+#if defined(CONFIG_COVERAGE)
        . = ALIGN(8);
        __ctors_start = .;
        *(.ctors)
        *(.init_array)
        *(SORT(.init_array.*))
        __ctors_end = .;
+#endif
 
 #if defined(CONFIG_HAS_VPCI) && !defined(CONFIG_LATE_HWDOM)
        . = ALIGN(POINTER_ALIGN);
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index cb42dc8..604353f 100644
--- 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
 
 #if defined(CONFIG_HAS_VPCI) && !defined(CONFIG_LATE_HWDOM)
        . = ALIGN(POINTER_ALIGN);
diff --git a/xen/common/lib.c b/xen/common/lib.c
index 8ebec81..b8fc165 100644
--- 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
 
+/* see 'docs/hypervisor-guide/code-coverage.rst' */
 void __init init_constructors(void)
 {
+#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. */
     BUILD_BUG_ON(sizeof(size_t) != sizeof(ssize_t));
 }
-- 
2.7.4


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

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

[Jan Beulich]

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

And if we were to take this, then please use the shorter #ifdef.

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

> +/* see 'docs/hypervisor-guide/code-coverage.rst' */
>  void __init init_constructors(void)

There's no mention of this function in the referenced docs file.

>  {
> +#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.

Jan



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

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

[chenbaodong]

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?

>
> And if we were to take this, then please use the shorter #ifdef.
Yes, can be fixed.
>
>> --- 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.
>
>> +/* 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.

>
>>   {
>> +#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.

>
> Jan
>
>
> .
>

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

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

[Jan Beulich]

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

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

[chenbaodong]

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)

>>>> --- 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.
Sorry for my expression, what you said here is exactly what i want.
>
> Jan
>
>
> .
>

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

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

[Jan Beulich]

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

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

[chenbaodong]

On 6/12/19 15:58, Jan Beulich wrote:
>>>> 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.

Copy that.

Thanks for all your comments.

>
> Jan
>
>
> .
>

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