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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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