[peterz-queue:perf/core 26/39] arch/x86/include/asm/perf_event.h:290:19: error: flexible array member in a struct with no named members

[peterz-queue:perf/core 26/39] arch/x86/include/asm/perf_event.h:290:19: error: flexible array member in a struct with no named members

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 10230 bytes --]

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git perf/core
head:   beb0aaf811f551900f60359d0ca5288257f03664
commit: b7e384bec88604ec36df7a4667a09a4749b943d3 [26/39] perf/x86/intel/lbr: Unify the stored format of LBR information
config: i386-tinyconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-14) 9.3.0
reproduce (this is a W=1 build):
        git checkout b7e384bec88604ec36df7a4667a09a4749b943d3
        # save the attached .config to linux build tree
        make W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from include/linux/perf_event.h:25,
                    from include/linux/trace_events.h:10,
                    from include/trace/syscall.h:7,
                    from include/linux/syscalls.h:85,
                    from kernel/fork.c:54:
>> arch/x86/include/asm/perf_event.h:290:19: error: flexible array member in a struct with no named members
     290 |  struct lbr_entry lbr[]; /* Variable length */
         |                   ^~~
--
   In file included from include/linux/perf_event.h:25,
                    from include/linux/trace_events.h:10,
                    from include/trace/syscall.h:7,
                    from include/linux/syscalls.h:85,
                    from kernel/exit.c:42:
>> arch/x86/include/asm/perf_event.h:290:19: error: flexible array member in a struct with no named members
     290 |  struct lbr_entry lbr[]; /* Variable length */
         |                   ^~~
   kernel/exit.c:1714:13: warning: no previous prototype for 'abort' [-Wmissing-prototypes]
    1714 | __weak void abort(void)
         |             ^~~~~
--
   In file included from include/linux/perf_event.h:25,
                    from include/linux/trace_events.h:10,
                    from include/trace/syscall.h:7,
                    from include/linux/syscalls.h:85,
                    from kernel/time/hrtimer.c:30:
>> arch/x86/include/asm/perf_event.h:290:19: error: flexible array member in a struct with no named members
     290 |  struct lbr_entry lbr[]; /* Variable length */
         |                   ^~~
   kernel/time/hrtimer.c:120:21: warning: initialized field overwritten [-Woverride-init]
     120 |  [CLOCK_REALTIME] = HRTIMER_BASE_REALTIME,
         |                     ^~~~~~~~~~~~~~~~~~~~~
   kernel/time/hrtimer.c:120:21: note: (near initialization for 'hrtimer_clock_to_base_table[0]')
   kernel/time/hrtimer.c:121:22: warning: initialized field overwritten [-Woverride-init]
     121 |  [CLOCK_MONOTONIC] = HRTIMER_BASE_MONOTONIC,
         |                      ^~~~~~~~~~~~~~~~~~~~~~
   kernel/time/hrtimer.c:121:22: note: (near initialization for 'hrtimer_clock_to_base_table[1]')
   kernel/time/hrtimer.c:122:21: warning: initialized field overwritten [-Woverride-init]
     122 |  [CLOCK_BOOTTIME] = HRTIMER_BASE_BOOTTIME,
         |                     ^~~~~~~~~~~~~~~~~~~~~
   kernel/time/hrtimer.c:122:21: note: (near initialization for 'hrtimer_clock_to_base_table[7]')
   kernel/time/hrtimer.c:123:17: warning: initialized field overwritten [-Woverride-init]
     123 |  [CLOCK_TAI]  = HRTIMER_BASE_TAI,
         |                 ^~~~~~~~~~~~~~~~
   kernel/time/hrtimer.c:123:17: note: (near initialization for 'hrtimer_clock_to_base_table[11]')
   kernel/time/hrtimer.c: In function '__run_hrtimer':
   kernel/time/hrtimer.c:1483:7: warning: variable 'expires_in_hardirq' set but not used [-Wunused-but-set-variable]
    1483 |  bool expires_in_hardirq;
         |       ^~~~~~~~~~~~~~~~~~
--
   In file included from include/linux/perf_event.h:25,
                    from include/linux/trace_events.h:10,
                    from include/trace/syscall.h:7,
                    from include/linux/syscalls.h:85,
                    from kernel/time/posix-stubs.c:13:
>> arch/x86/include/asm/perf_event.h:290:19: error: flexible array member in a struct with no named members
     290 |  struct lbr_entry lbr[]; /* Variable length */
         |                   ^~~
   kernel/time/posix-stubs.c:25:17: warning: no previous prototype for 'sys_ni_posix_timers' [-Wmissing-prototypes]
      25 | asmlinkage long sys_ni_posix_timers(void)
         |                 ^~~~~~~~~~~~~~~~~~~
   kernel/time/posix-stubs.c:73:5: warning: no previous prototype for 'do_clock_gettime' [-Wmissing-prototypes]
      73 | int do_clock_gettime(clockid_t which_clock, struct timespec64 *tp)
         |     ^~~~~~~~~~~~~~~~
--
   In file included from include/linux/perf_event.h:25,
                    from include/linux/trace_events.h:10,
                    from include/trace/syscall.h:7,
                    from include/linux/syscalls.h:85,
                    from kernel/events/core.c:34:
>> arch/x86/include/asm/perf_event.h:290:19: error: flexible array member in a struct with no named members
     290 |  struct lbr_entry lbr[]; /* Variable length */
         |                   ^~~
   kernel/events/core.c:6493:6: warning: no previous prototype for 'perf_pmu_snapshot_aux' [-Wmissing-prototypes]
    6493 | long perf_pmu_snapshot_aux(struct perf_buffer *rb,
         |      ^~~~~~~~~~~~~~~~~~~~~
--
   In file included from include/linux/perf_event.h:25,
                    from include/linux/trace_events.h:10,
                    from include/trace/syscall.h:7,
                    from include/linux/syscalls.h:85,
                    from kernel/printk/printk.c:36:
>> arch/x86/include/asm/perf_event.h:290:19: error: flexible array member in a struct with no named members
     290 |  struct lbr_entry lbr[]; /* Variable length */
         |                   ^~~
   In file included from kernel/printk/printk.c:60:
   kernel/printk/internal.h:59:20: warning: no previous prototype for 'vprintk_func' [-Wmissing-prototypes]
      59 | __printf(1, 0) int vprintk_func(const char *fmt, va_list args) { return 0; }
         |                    ^~~~~~~~~~~~
   kernel/printk/printk.c:174:5: warning: no previous prototype for 'devkmsg_sysctl_set_loglvl' [-Wmissing-prototypes]
     174 | int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write,
         |     ^~~~~~~~~~~~~~~~~~~~~~~~~
--
   In file included from include/linux/perf_event.h:25,
                    from include/linux/trace_events.h:10,
                    from include/trace/syscall.h:7,
                    from include/linux/syscalls.h:85,
                    from kernel/sched/sched.h:65,
                    from kernel/sched/core.c:9:
>> arch/x86/include/asm/perf_event.h:290:19: error: flexible array member in a struct with no named members
     290 |  struct lbr_entry lbr[]; /* Variable length */
         |                   ^~~
   kernel/sched/core.c: In function 'ttwu_stat':
   kernel/sched/core.c:2160:13: warning: variable 'rq' set but not used [-Wunused-but-set-variable]
    2160 |  struct rq *rq;
         |             ^~
--
   In file included from include/linux/perf_event.h:25,
                    from include/linux/trace_events.h:10,
                    from include/trace/syscall.h:7,
                    from include/linux/syscalls.h:85,
                    from kernel/sched/sched.h:65,
                    from kernel/sched/fair.c:23:
>> arch/x86/include/asm/perf_event.h:290:19: error: flexible array member in a struct with no named members
     290 |  struct lbr_entry lbr[]; /* Variable length */
         |                   ^~~
   kernel/sched/fair.c:5352:6: warning: no previous prototype for 'init_cfs_bandwidth' [-Wmissing-prototypes]
    5352 | void init_cfs_bandwidth(struct cfs_bandwidth *cfs_b) {}
         |      ^~~~~~~~~~~~~~~~~~
   kernel/sched/fair.c:11078:6: warning: no previous prototype for 'free_fair_sched_group' [-Wmissing-prototypes]
   11078 | void free_fair_sched_group(struct task_group *tg) { }
         |      ^~~~~~~~~~~~~~~~~~~~~
   kernel/sched/fair.c:11080:5: warning: no previous prototype for 'alloc_fair_sched_group' [-Wmissing-prototypes]
   11080 | int alloc_fair_sched_group(struct task_group *tg, struct task_group *parent)
         |     ^~~~~~~~~~~~~~~~~~~~~~
   kernel/sched/fair.c:11085:6: warning: no previous prototype for 'online_fair_sched_group' [-Wmissing-prototypes]
   11085 | void online_fair_sched_group(struct task_group *tg) { }
         |      ^~~~~~~~~~~~~~~~~~~~~~~
   kernel/sched/fair.c:11087:6: warning: no previous prototype for 'unregister_fair_sched_group' [-Wmissing-prototypes]
   11087 | void unregister_fair_sched_group(struct task_group *tg) { }
         |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~
--
   In file included from include/linux/perf_event.h:25,
                    from include/linux/trace_events.h:10,
                    from include/trace/syscall.h:7,
                    from include/linux/syscalls.h:85,
                    from kernel/sched/sched.h:65,
                    from kernel/sched/rt.c:6:
>> arch/x86/include/asm/perf_event.h:290:19: error: flexible array member in a struct with no named members
     290 |  struct lbr_entry lbr[]; /* Variable length */
         |                   ^~~
   kernel/sched/rt.c:253:6: warning: no previous prototype for 'free_rt_sched_group' [-Wmissing-prototypes]
     253 | void free_rt_sched_group(struct task_group *tg) { }
         |      ^~~~~~~~~~~~~~~~~~~
   kernel/sched/rt.c:255:5: warning: no previous prototype for 'alloc_rt_sched_group' [-Wmissing-prototypes]
     255 | int alloc_rt_sched_group(struct task_group *tg, struct task_group *parent)
         |     ^~~~~~~~~~~~~~~~~~~~
   kernel/sched/rt.c:668:6: warning: no previous prototype for 'sched_rt_bandwidth_account' [-Wmissing-prototypes]
     668 | bool sched_rt_bandwidth_account(struct rt_rq *rt_rq)
         |      ^~~~~~~~~~~~~~~~~~~~~~~~~~

vim +290 arch/x86/include/asm/perf_event.h

   288	
   289	struct pebs_lbr {
 > 290		struct lbr_entry lbr[]; /* Variable length */
   291	};
   292	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 7300 bytes --]

Re: [peterz-queue:perf/core 26/39] arch/x86/include/asm/perf_event.h:290:19: error: flexible array member in a struct with no named members

[Liang, Kan]

[-- Attachment #1: Type: text/plain, Size: 1595 bytes --]


>>> arch/x86/include/asm/perf_event.h:290:19: error: flexible array member in a struct with no named members
>       290 |  struct lbr_entry lbr[]; /* Variable length */
>           |                   ^~~
>     kernel/sched/rt.c:253:6: warning: no previous prototype for 'free_rt_sched_group' [-Wmissing-prototypes]
>       253 | void free_rt_sched_group(struct task_group *tg) { }
>           |      ^~~~~~~~~~~~~~~~~~~
>     kernel/sched/rt.c:255:5: warning: no previous prototype for 'alloc_rt_sched_group' [-Wmissing-prototypes]
>       255 | int alloc_rt_sched_group(struct task_group *tg, struct task_group *parent)
>           |     ^~~~~~~~~~~~~~~~~~~~
>     kernel/sched/rt.c:668:6: warning: no previous prototype for 'sched_rt_bandwidth_account' [-Wmissing-prototypes]
>       668 | bool sched_rt_bandwidth_account(struct rt_rq *rt_rq)
>           |      ^~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> vim +290 arch/x86/include/asm/perf_event.h
> 
>     288	
>     289	struct pebs_lbr {
>   > 290		struct lbr_entry lbr[]; /* Variable length */
>     291	};
>     292	
>

The struct pebs_lbr only has one member. To fix the warning, it seems we 
have to define the struct as below.


diff --git a/arch/x86/include/asm/perf_event.h 
b/arch/x86/include/asm/perf_event.h
index 2338200..a387e14 100644
--- a/arch/x86/include/asm/perf_event.h
+++ b/arch/x86/include/asm/perf_event.h
@@ -283,7 +283,7 @@ struct pebs_xmm {
  };

  struct pebs_lbr {
-	struct lbr_entry lbr[]; /* Variable length */
+	struct lbr_entry lbr[0]; /* Variable length */
  };


Thanks,
Kan

Re: [peterz-queue:perf/core 26/39] arch/x86/include/asm/perf_event.h:290:19: error: flexible array member in a struct with no named members

[Peter Zijlstra]

[-- Attachment #1: Type: text/plain, Size: 3710 bytes --]

On Mon, Jul 06, 2020 at 09:08:41PM -0400, Liang, Kan wrote:

> >     288	
> >     289	struct pebs_lbr {
> >   > 290		struct lbr_entry lbr[]; /* Variable length */
> >     291	};
> >     292	
> 
> The struct pebs_lbr only has one member. To fix the warning, it seems we
> have to define the struct as below.
> 
> 
> diff --git a/arch/x86/include/asm/perf_event.h
> b/arch/x86/include/asm/perf_event.h
> index 2338200..a387e14 100644
> --- a/arch/x86/include/asm/perf_event.h
> +++ b/arch/x86/include/asm/perf_event.h
> @@ -283,7 +283,7 @@ struct pebs_xmm {
>  };
> 
>  struct pebs_lbr {
> -	struct lbr_entry lbr[]; /* Variable length */
> +	struct lbr_entry lbr[0]; /* Variable length */
>  };
> 

Right, but then we get trouble like the below again. There's basically
no good solution here :-(

Gustavo, any clues?

---
commit 04f5c362ec6d3ff0e14f1c05230b550da7f528a4
Author: Gustavo A. R. Silva <gustavoars@kernel.org>
Date:   Thu May 7 14:21:41 2020 -0500

    sched/fair: Replace zero-length array with flexible-array
    
    The current codebase makes use of the zero-length array language
    extension to the C90 standard, but the preferred mechanism to declare
    variable-length types such as these ones is a flexible array member[1][2],
    introduced in C99:
    
    struct foo {
            int stuff;
            struct boo array[];
    };
    
    By making use of the mechanism above, we will get a compiler warning
    in case the flexible array does not occur last in the structure, which
    will help us prevent some kind of undefined behavior bugs from being
    inadvertently introduced[3] to the codebase from now on.
    
    Also, notice that, dynamic memory allocations won't be affected by
    this change:
    
    "Flexible array members have incomplete type, and so the sizeof operator
    may not be applied. As a quirk of the original implementation of
    zero-length arrays, sizeof evaluates to zero."[1]
    
    sizeof(flexible-array-member) triggers a warning because flexible array
    members have incomplete type[1]. There are some instances of code in
    which the sizeof operator is being incorrectly/erroneously applied to
    zero-length arrays and the result is zero. Such instances may be hiding
    some bugs. So, this work (flexible-array member conversions) will also
    help to get completely rid of those sorts of issues.
    
    This issue was found with the help of Coccinelle.
    
    [1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
    [2] https://github.com/KSPP/linux/issues/21
    [3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
    
    Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
    Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
    Link: https://lkml.kernel.org/r/20200507192141.GA16183(a)embeddedor

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 44b0c8edc260..01f94cf52783 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -1094,7 +1094,7 @@ struct numa_group {
 	 * more by CPU use than by memory faults.
 	 */
 	unsigned long *faults_cpu;
-	unsigned long faults[0];
+	unsigned long faults[];
 };
 
 /*
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 21416b30c520..2bd2a222318a 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -1462,7 +1462,7 @@ struct sched_group {
 	 * by attaching extra space to the end of the structure,
 	 * depending on how many CPUs the kernel has booted up with)
 	 */
-	unsigned long		cpumask[0];
+	unsigned long		cpumask[];
 };
 
 static inline struct cpumask *sched_group_span(struct sched_group *sg)

Re: [peterz-queue:perf/core 26/39] arch/x86/include/asm/perf_event.h:290:19: error: flexible array member in a struct with no named members

[Gustavo A. R. Silva]

[-- Attachment #1: Type: text/plain, Size: 4960 bytes --]

On Tue, Jul 07, 2020 at 12:24:43PM +0200, Peter Zijlstra wrote:
> On Mon, Jul 06, 2020 at 09:08:41PM -0400, Liang, Kan wrote:
> 
> > >     288	
> > >     289	struct pebs_lbr {
> > >   > 290		struct lbr_entry lbr[]; /* Variable length */
> > >     291	};
> > >     292	
> > 
> > The struct pebs_lbr only has one member. To fix the warning, it seems we
> > have to define the struct as below.
> > 
> > 
> > diff --git a/arch/x86/include/asm/perf_event.h
> > b/arch/x86/include/asm/perf_event.h
> > index 2338200..a387e14 100644
> > --- a/arch/x86/include/asm/perf_event.h
> > +++ b/arch/x86/include/asm/perf_event.h
> > @@ -283,7 +283,7 @@ struct pebs_xmm {
> >  };
> > 
> >  struct pebs_lbr {
> > -	struct lbr_entry lbr[]; /* Variable length */
> > +	struct lbr_entry lbr[0]; /* Variable length */
> >  };
> > 
> 
> Right, but then we get trouble like the below again. There's basically

Yep, zero-length and one-element arrays are being deprecated[1].

> no good solution here :-(
> 
> Gustavo, any clues?
> 

Yep; the issue is that "Flexible array members may only appear as the
last member of a struct that is otherwise non-empty"[2].

The solution is to declare something like this:

struct pebs_lbr {
	size_t num_lbr; /* count for number of lbr entries */
	struct lbr_entry lbr[];
};

and allocate memory as follows:

struct pebs_lbr *lbr_entries;

...

lbr_entries = kmalloc(struct_size(lbr_entries, lbr, count), GFP_KERNEL);
lbr_entries->num_lbr = count;

...

and then you can access each entry through the use of an index
delimited by lbr_entries->num_lbr (which is self-contained and
not any other random variable):

for (i = 0; i < lbr_entries->num_lbr; i++)
{
	...

	whatever = lbr_entries->lbr[i];

	...
}

and so forth...

--
Gustavo

[1] https://lore.kernel.org/lkml/20200608213711.GA22271(a)embeddedor/
[2] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html

> ---
> commit 04f5c362ec6d3ff0e14f1c05230b550da7f528a4
> Author: Gustavo A. R. Silva <gustavoars@kernel.org>
> Date:   Thu May 7 14:21:41 2020 -0500
> 
>     sched/fair: Replace zero-length array with flexible-array
>     
>     The current codebase makes use of the zero-length array language
>     extension to the C90 standard, but the preferred mechanism to declare
>     variable-length types such as these ones is a flexible array member[1][2],
>     introduced in C99:
>     
>     struct foo {
>             int stuff;
>             struct boo array[];
>     };
>     
>     By making use of the mechanism above, we will get a compiler warning
>     in case the flexible array does not occur last in the structure, which
>     will help us prevent some kind of undefined behavior bugs from being
>     inadvertently introduced[3] to the codebase from now on.
>     
>     Also, notice that, dynamic memory allocations won't be affected by
>     this change:
>     
>     "Flexible array members have incomplete type, and so the sizeof operator
>     may not be applied. As a quirk of the original implementation of
>     zero-length arrays, sizeof evaluates to zero."[1]
>     
>     sizeof(flexible-array-member) triggers a warning because flexible array
>     members have incomplete type[1]. There are some instances of code in
>     which the sizeof operator is being incorrectly/erroneously applied to
>     zero-length arrays and the result is zero. Such instances may be hiding
>     some bugs. So, this work (flexible-array member conversions) will also
>     help to get completely rid of those sorts of issues.
>     
>     This issue was found with the help of Coccinelle.
>     
>     [1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
>     [2] https://github.com/KSPP/linux/issues/21
>     [3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
>     
>     Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
>     Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
>     Link: https://lkml.kernel.org/r/20200507192141.GA16183(a)embeddedor
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 44b0c8edc260..01f94cf52783 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -1094,7 +1094,7 @@ struct numa_group {
>  	 * more by CPU use than by memory faults.
>  	 */
>  	unsigned long *faults_cpu;
> -	unsigned long faults[0];
> +	unsigned long faults[];
>  };
>  
>  /*
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index 21416b30c520..2bd2a222318a 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -1462,7 +1462,7 @@ struct sched_group {
>  	 * by attaching extra space to the end of the structure,
>  	 * depending on how many CPUs the kernel has booted up with)
>  	 */
> -	unsigned long		cpumask[0];
> +	unsigned long		cpumask[];
>  };
>  
>  static inline struct cpumask *sched_group_span(struct sched_group *sg)

Re: [peterz-queue:perf/core 26/39] arch/x86/include/asm/perf_event.h:290:19: error: flexible array member in a struct with no named members

[Peter Zijlstra]

[-- Attachment #1: Type: text/plain, Size: 1829 bytes --]

On Tue, Jul 07, 2020 at 08:51:10AM -0500, Gustavo A. R. Silva wrote:
> On Tue, Jul 07, 2020 at 12:24:43PM +0200, Peter Zijlstra wrote:
> > On Mon, Jul 06, 2020 at 09:08:41PM -0400, Liang, Kan wrote:
> > 
> > > >     288	
> > > >     289	struct pebs_lbr {
> > > >   > 290		struct lbr_entry lbr[]; /* Variable length */
> > > >     291	};
> > > >     292	
> > > 
> > > The struct pebs_lbr only has one member. To fix the warning, it seems we
> > > have to define the struct as below.
> > > 
> > > 
> > > diff --git a/arch/x86/include/asm/perf_event.h
> > > b/arch/x86/include/asm/perf_event.h
> > > index 2338200..a387e14 100644
> > > --- a/arch/x86/include/asm/perf_event.h
> > > +++ b/arch/x86/include/asm/perf_event.h
> > > @@ -283,7 +283,7 @@ struct pebs_xmm {
> > >  };
> > > 
> > >  struct pebs_lbr {
> > > -	struct lbr_entry lbr[]; /* Variable length */
> > > +	struct lbr_entry lbr[0]; /* Variable length */
> > >  };
> > > 
> > 
> > Right, but then we get trouble like the below again. There's basically
> 
> Yep, zero-length and one-element arrays are being deprecated[1].
> 
> > no good solution here :-(
> > 
> > Gustavo, any clues?
> > 
> 
> Yep; the issue is that "Flexible array members may only appear as the
> last member of a struct that is otherwise non-empty"[2].
> 
> The solution is to declare something like this:
> 
> struct pebs_lbr {
> 	size_t num_lbr; /* count for number of lbr entries */
> 	struct lbr_entry lbr[];
> };
> 

Can't.. this is hardware provided layout. The thing is that the array
size is variable, so [0] or [] would be correct.

That said, Kan, I suppose we can simply remove struct pebs_lbr and use
struct lbr_entry * directly. There is very little actual point in having
struct pebs_lbr.

I'll frob the patches and push out again.

Re: [peterz-queue:perf/core 26/39] arch/x86/include/asm/perf_event.h:290:19: error: flexible array member in a struct with no named members

[Liang, Kan]

[-- Attachment #1: Type: text/plain, Size: 1977 bytes --]



On 7/7/2020 10:44 AM, Peter Zijlstra wrote:
> On Tue, Jul 07, 2020 at 08:51:10AM -0500, Gustavo A. R. Silva wrote:
>> On Tue, Jul 07, 2020 at 12:24:43PM +0200, Peter Zijlstra wrote:
>>> On Mon, Jul 06, 2020 at 09:08:41PM -0400, Liang, Kan wrote:
>>>
>>>>>      288	
>>>>>      289	struct pebs_lbr {
>>>>>    > 290		struct lbr_entry lbr[]; /* Variable length */
>>>>>      291	};
>>>>>      292	
>>>>
>>>> The struct pebs_lbr only has one member. To fix the warning, it seems we
>>>> have to define the struct as below.
>>>>
>>>>
>>>> diff --git a/arch/x86/include/asm/perf_event.h
>>>> b/arch/x86/include/asm/perf_event.h
>>>> index 2338200..a387e14 100644
>>>> --- a/arch/x86/include/asm/perf_event.h
>>>> +++ b/arch/x86/include/asm/perf_event.h
>>>> @@ -283,7 +283,7 @@ struct pebs_xmm {
>>>>   };
>>>>
>>>>   struct pebs_lbr {
>>>> -	struct lbr_entry lbr[]; /* Variable length */
>>>> +	struct lbr_entry lbr[0]; /* Variable length */
>>>>   };
>>>>
>>>
>>> Right, but then we get trouble like the below again. There's basically
>>
>> Yep, zero-length and one-element arrays are being deprecated[1].
>>
>>> no good solution here :-(
>>>
>>> Gustavo, any clues?
>>>
>>
>> Yep; the issue is that "Flexible array members may only appear as the
>> last member of a struct that is otherwise non-empty"[2].
>>
>> The solution is to declare something like this:
>>
>> struct pebs_lbr {
>> 	size_t num_lbr; /* count for number of lbr entries */
>> 	struct lbr_entry lbr[];
>> };
>>
> 
> Can't.. this is hardware provided layout. The thing is that the array
> size is variable, so [0] or [] would be correct.
> 
> That said, Kan, I suppose we can simply remove struct pebs_lbr and use
> struct lbr_entry * directly. There is very little actual point in having
> struct pebs_lbr.
> 

Yes, I will do more tests with the patch I just sent in another thread.

> I'll frob the patches and push out again.
> 

Thanks.

Kan

Re: [peterz-queue:perf/core 26/39] arch/x86/include/asm/perf_event.h:290:19: error: flexible array member in a struct with no named members

[Liang, Kan]

[-- Attachment #1: Type: text/plain, Size: 4358 bytes --]



On 7/7/2020 9:51 AM, Gustavo A. R. Silva wrote:
> On Tue, Jul 07, 2020 at 12:24:43PM +0200, Peter Zijlstra wrote:
>> On Mon, Jul 06, 2020 at 09:08:41PM -0400, Liang, Kan wrote:
>>
>>>>      288	
>>>>      289	struct pebs_lbr {
>>>>    > 290		struct lbr_entry lbr[]; /* Variable length */
>>>>      291	};
>>>>      292	
>>>
>>> The struct pebs_lbr only has one member. To fix the warning, it seems we
>>> have to define the struct as below.
>>>
>>>
>>> diff --git a/arch/x86/include/asm/perf_event.h
>>> b/arch/x86/include/asm/perf_event.h
>>> index 2338200..a387e14 100644
>>> --- a/arch/x86/include/asm/perf_event.h
>>> +++ b/arch/x86/include/asm/perf_event.h
>>> @@ -283,7 +283,7 @@ struct pebs_xmm {
>>>   };
>>>
>>>   struct pebs_lbr {
>>> -	struct lbr_entry lbr[]; /* Variable length */
>>> +	struct lbr_entry lbr[0]; /* Variable length */
>>>   };
>>>
>>
>> Right, but then we get trouble like the below again. There's basically
> 
> Yep, zero-length and one-element arrays are being deprecated[1].
>  >> no good solution here :-(
>>
>> Gustavo, any clues?
>>
> 
> Yep; the issue is that "Flexible array members may only appear as the
> last member of a struct that is otherwise non-empty"[2].
> 
> The solution is to declare something like this:
> 
> struct pebs_lbr {
> 	size_t num_lbr; /* count for number of lbr entries */
> 	struct lbr_entry lbr[];
> };
> 
> and allocate memory as follows:

It's kind different for PEBS LBR. We don't allocate a buffer for struct 
pebs_lbr. We just point it to a big memory (PEBS records) at run time 
which includes not only LBR data but also other data.

With the above structure, there is an error when I assign the pointer 
via lbr.lbr = (struct lbr_entry *) next_record;.
     "error: invalid use of flexible array member"

I think we may want to drop the struct pebs_lbr and directly pass the 
pointer to intel_pmu_store_lbr().
(The patch as below hasn't been tested yet.)

diff --git a/arch/x86/events/intel/ds.c b/arch/x86/events/intel/ds.c
index 0d33f85..13f18f6 100644
--- a/arch/x86/events/intel/ds.c
+++ b/arch/x86/events/intel/ds.c
@@ -1595,13 +1595,12 @@ static void 
setup_pebs_adaptive_sample_data(struct perf_event *event,
  	}

  	if (format_size & PEBS_DATACFG_LBRS) {
-		struct pebs_lbr *lbr = next_record;
  		int num_lbr = ((format_size >> PEBS_DATACFG_LBR_SHIFT)
  					& 0xff) + 1;
  		next_record = next_record + num_lbr * sizeof(struct lbr_entry);

  		if (has_branch_stack(event)) {
-			intel_pmu_store_pebs_lbrs(lbr);
+			intel_pmu_store_pebs_lbrs((struct lbr_entry *)next_record);
  			data->br_stack = &cpuc->lbr_stack;
  		}
  	}
diff --git a/arch/x86/events/intel/lbr.c b/arch/x86/events/intel/lbr.c
index db4cdce..0f04169 100644
--- a/arch/x86/events/intel/lbr.c
+++ b/arch/x86/events/intel/lbr.c
@@ -1461,7 +1461,7 @@ intel_pmu_lbr_filter(struct cpu_hw_events *cpuc)
  	}
  }

-void intel_pmu_store_pebs_lbrs(struct pebs_lbr *lbr)
+void intel_pmu_store_pebs_lbrs(struct lbr_entry *lbr)
  {
  	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);

@@ -1472,7 +1472,7 @@ void intel_pmu_store_pebs_lbrs(struct pebs_lbr *lbr)
  	else
  		cpuc->lbr_stack.hw_idx = intel_pmu_lbr_tos();

-	intel_pmu_store_lbr(cpuc, lbr->lbr, lbr_need_info(cpuc));
+	intel_pmu_store_lbr(cpuc, lbr, lbr_need_info(cpuc));

  	intel_pmu_lbr_filter(cpuc);
  }
diff --git a/arch/x86/events/perf_event.h b/arch/x86/events/perf_event.h
index 466e1f9..7b68ab5 100644
--- a/arch/x86/events/perf_event.h
+++ b/arch/x86/events/perf_event.h
@@ -1122,7 +1122,7 @@ void intel_pmu_pebs_sched_task(struct 
perf_event_context *ctx, bool sched_in);

  void intel_pmu_auto_reload_read(struct perf_event *event);

-void intel_pmu_store_pebs_lbrs(struct pebs_lbr *lbr);
+void intel_pmu_store_pebs_lbrs(struct lbr_entry *lbr);

  void intel_ds_init(void);

diff --git a/arch/x86/include/asm/perf_event.h 
b/arch/x86/include/asm/perf_event.h
index a387e14..0c1b137 100644
--- a/arch/x86/include/asm/perf_event.h
+++ b/arch/x86/include/asm/perf_event.h
@@ -282,10 +282,6 @@ struct pebs_xmm {
  	u64 xmm[16*2];	/* two entries for each register */
  };

-struct pebs_lbr {
-	struct lbr_entry lbr[0]; /* Variable length */
-};
-
  /*
   * IBS cpuid feature detection
   */

Thanks,
Kan

Re: [peterz-queue:perf/core 26/39] arch/x86/include/asm/perf_event.h:290:19: error: flexible array member in a struct with no named members

[Gustavo A. R. Silva]

[-- Attachment #1: Type: text/plain, Size: 4855 bytes --]

On Tue, Jul 07, 2020 at 10:50:47AM -0400, Liang, Kan wrote:
> 
> 
> On 7/7/2020 9:51 AM, Gustavo A. R. Silva wrote:
> > On Tue, Jul 07, 2020 at 12:24:43PM +0200, Peter Zijlstra wrote:
> > > On Mon, Jul 06, 2020 at 09:08:41PM -0400, Liang, Kan wrote:
> > > 
> > > > >      288	
> > > > >      289	struct pebs_lbr {
> > > > >    > 290		struct lbr_entry lbr[]; /* Variable length */
> > > > >      291	};
> > > > >      292	
> > > > 
> > > > The struct pebs_lbr only has one member. To fix the warning, it seems we
> > > > have to define the struct as below.
> > > > 
> > > > 
> > > > diff --git a/arch/x86/include/asm/perf_event.h
> > > > b/arch/x86/include/asm/perf_event.h
> > > > index 2338200..a387e14 100644
> > > > --- a/arch/x86/include/asm/perf_event.h
> > > > +++ b/arch/x86/include/asm/perf_event.h
> > > > @@ -283,7 +283,7 @@ struct pebs_xmm {
> > > >   };
> > > > 
> > > >   struct pebs_lbr {
> > > > -	struct lbr_entry lbr[]; /* Variable length */
> > > > +	struct lbr_entry lbr[0]; /* Variable length */
> > > >   };
> > > > 
> > > 
> > > Right, but then we get trouble like the below again. There's basically
> > 
> > Yep, zero-length and one-element arrays are being deprecated[1].
> >  >> no good solution here :-(
> > > 
> > > Gustavo, any clues?
> > > 
> > 
> > Yep; the issue is that "Flexible array members may only appear as the
> > last member of a struct that is otherwise non-empty"[2].
> > 
> > The solution is to declare something like this:
> > 
> > struct pebs_lbr {
> > 	size_t num_lbr; /* count for number of lbr entries */
> > 	struct lbr_entry lbr[];
> > };
> > 
> > and allocate memory as follows:
> 
> It's kind different for PEBS LBR. We don't allocate a buffer for struct
> pebs_lbr. We just point it to a big memory (PEBS records) at run time which
> includes not only LBR data but also other data.
> 
> With the above structure, there is an error when I assign the pointer via
> lbr.lbr = (struct lbr_entry *) next_record;.
>     "error: invalid use of flexible array member"
> 
> I think we may want to drop the struct pebs_lbr and directly pass the
> pointer to intel_pmu_store_lbr().
> (The patch as below hasn't been tested yet.)
> 
> diff --git a/arch/x86/events/intel/ds.c b/arch/x86/events/intel/ds.c
> index 0d33f85..13f18f6 100644
> --- a/arch/x86/events/intel/ds.c
> +++ b/arch/x86/events/intel/ds.c
> @@ -1595,13 +1595,12 @@ static void setup_pebs_adaptive_sample_data(struct
> perf_event *event,
>  	}
> 
>  	if (format_size & PEBS_DATACFG_LBRS) {
> -		struct pebs_lbr *lbr = next_record;

			^^^^^^
Why don't you just directly use lbr_entry here instead of pebs_lbr and avoid
the cast below...

>  		int num_lbr = ((format_size >> PEBS_DATACFG_LBR_SHIFT)
>  					& 0xff) + 1;
>  		next_record = next_record + num_lbr * sizeof(struct lbr_entry);
> 
>  		if (has_branch_stack(event)) {
> -			intel_pmu_store_pebs_lbrs(lbr);
> +			intel_pmu_store_pebs_lbrs((struct lbr_entry *)next_record);
>  			data->br_stack = &cpuc->lbr_stack;
>  		}
>  	}
> diff --git a/arch/x86/events/intel/lbr.c b/arch/x86/events/intel/lbr.c
> index db4cdce..0f04169 100644
> --- a/arch/x86/events/intel/lbr.c
> +++ b/arch/x86/events/intel/lbr.c
> @@ -1461,7 +1461,7 @@ intel_pmu_lbr_filter(struct cpu_hw_events *cpuc)
>  	}
>  }
> 
> -void intel_pmu_store_pebs_lbrs(struct pebs_lbr *lbr)
> +void intel_pmu_store_pebs_lbrs(struct lbr_entry *lbr)
>  {
>  	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
> 
> @@ -1472,7 +1472,7 @@ void intel_pmu_store_pebs_lbrs(struct pebs_lbr *lbr)
>  	else
>  		cpuc->lbr_stack.hw_idx = intel_pmu_lbr_tos();
> 
> -	intel_pmu_store_lbr(cpuc, lbr->lbr, lbr_need_info(cpuc));
> +	intel_pmu_store_lbr(cpuc, lbr, lbr_need_info(cpuc));
> 
>  	intel_pmu_lbr_filter(cpuc);
>  }
> diff --git a/arch/x86/events/perf_event.h b/arch/x86/events/perf_event.h
> index 466e1f9..7b68ab5 100644
> --- a/arch/x86/events/perf_event.h
> +++ b/arch/x86/events/perf_event.h
> @@ -1122,7 +1122,7 @@ void intel_pmu_pebs_sched_task(struct
> perf_event_context *ctx, bool sched_in);
> 
>  void intel_pmu_auto_reload_read(struct perf_event *event);
> 
> -void intel_pmu_store_pebs_lbrs(struct pebs_lbr *lbr);
> +void intel_pmu_store_pebs_lbrs(struct lbr_entry *lbr);
> 
>  void intel_ds_init(void);
> 
> diff --git a/arch/x86/include/asm/perf_event.h
> b/arch/x86/include/asm/perf_event.h
> index a387e14..0c1b137 100644
> --- a/arch/x86/include/asm/perf_event.h
> +++ b/arch/x86/include/asm/perf_event.h
> @@ -282,10 +282,6 @@ struct pebs_xmm {
>  	u64 xmm[16*2];	/* two entries for each register */
>  };
> 
> -struct pebs_lbr {
> -	struct lbr_entry lbr[0]; /* Variable length */
> -};
> -
>  /*
>   * IBS cpuid feature detection
>   */
> 
> Thanks,
> Kan

--
Gustavo

Re: [peterz-queue:perf/core 26/39] arch/x86/include/asm/perf_event.h:290:19: error: flexible array member in a struct with no named members

[Liang, Kan]

[-- Attachment #1: Type: text/plain, Size: 2990 bytes --]



On 7/7/2020 11:05 AM, Gustavo A. R. Silva wrote:
> On Tue, Jul 07, 2020 at 10:50:47AM -0400, Liang, Kan wrote:
>>
>>
>> On 7/7/2020 9:51 AM, Gustavo A. R. Silva wrote:
>>> On Tue, Jul 07, 2020 at 12:24:43PM +0200, Peter Zijlstra wrote:
>>>> On Mon, Jul 06, 2020 at 09:08:41PM -0400, Liang, Kan wrote:
>>>>
>>>>>>       288	
>>>>>>       289	struct pebs_lbr {
>>>>>>     > 290		struct lbr_entry lbr[]; /* Variable length */
>>>>>>       291	};
>>>>>>       292	
>>>>>
>>>>> The struct pebs_lbr only has one member. To fix the warning, it seems we
>>>>> have to define the struct as below.
>>>>>
>>>>>
>>>>> diff --git a/arch/x86/include/asm/perf_event.h
>>>>> b/arch/x86/include/asm/perf_event.h
>>>>> index 2338200..a387e14 100644
>>>>> --- a/arch/x86/include/asm/perf_event.h
>>>>> +++ b/arch/x86/include/asm/perf_event.h
>>>>> @@ -283,7 +283,7 @@ struct pebs_xmm {
>>>>>    };
>>>>>
>>>>>    struct pebs_lbr {
>>>>> -	struct lbr_entry lbr[]; /* Variable length */
>>>>> +	struct lbr_entry lbr[0]; /* Variable length */
>>>>>    };
>>>>>
>>>>
>>>> Right, but then we get trouble like the below again. There's basically
>>>
>>> Yep, zero-length and one-element arrays are being deprecated[1].
>>>   >> no good solution here :-(
>>>>
>>>> Gustavo, any clues?
>>>>
>>>
>>> Yep; the issue is that "Flexible array members may only appear as the
>>> last member of a struct that is otherwise non-empty"[2].
>>>
>>> The solution is to declare something like this:
>>>
>>> struct pebs_lbr {
>>> 	size_t num_lbr; /* count for number of lbr entries */
>>> 	struct lbr_entry lbr[];
>>> };
>>>
>>> and allocate memory as follows:
>>
>> It's kind different for PEBS LBR. We don't allocate a buffer for struct
>> pebs_lbr. We just point it to a big memory (PEBS records) at run time which
>> includes not only LBR data but also other data.
>>
>> With the above structure, there is an error when I assign the pointer via
>> lbr.lbr = (struct lbr_entry *) next_record;.
>>      "error: invalid use of flexible array member"
>>
>> I think we may want to drop the struct pebs_lbr and directly pass the
>> pointer to intel_pmu_store_lbr().
>> (The patch as below hasn't been tested yet.)
>>
>> diff --git a/arch/x86/events/intel/ds.c b/arch/x86/events/intel/ds.c
>> index 0d33f85..13f18f6 100644
>> --- a/arch/x86/events/intel/ds.c
>> +++ b/arch/x86/events/intel/ds.c
>> @@ -1595,13 +1595,12 @@ static void setup_pebs_adaptive_sample_data(struct
>> perf_event *event,
>>   	}
>>
>>   	if (format_size & PEBS_DATACFG_LBRS) {
>> -		struct pebs_lbr *lbr = next_record;
> 
> 			^^^^^^
> Why don't you just directly use lbr_entry here instead of pebs_lbr and avoid
> the cast below...


Yes, we can directly use the lbr_entry here.

I think Peter just update the branch which also directly use the 
lbr_entry here. I will pull the latest code and do more test for the 
change of dropping the struct pebs_lbr.


Thanks
Kan

Re: [peterz-queue:perf/core 26/39] arch/x86/include/asm/perf_event.h:290:19: error: flexible array member in a struct with no named members

[Peter Zijlstra]

[-- Attachment #1: Type: text/plain, Size: 376 bytes --]

On Tue, Jul 07, 2020 at 11:24:59AM -0400, Liang, Kan wrote:
> I think Peter just update the branch which also directly use the lbr_entry
> here. I will pull the latest code and do more test for the change of
> dropping the struct pebs_lbr.

Yes, if you look at:

  git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git perf/core

pebs_lbr should be gone now.