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

[chenbaodong <chenbaodong@xxxxxxxxxx>]

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

Thanks for all your comments.

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