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 <gustavoars@kernel.org>]

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