| Message ID | 1560232949-10673-1-git-send-email-chenbaodong@mxnavi.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/coverage: wrap coverage related things under 'CONFIG_COVERAGE' | expand |
>>> 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
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 > > > . >
>>> 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
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 > > > . >
>>> 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
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 > > > . >
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)); }
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(+)