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

[chenbaodong <chenbaodong@xxxxxxxxxx>]

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)

> > > > --- 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel