[v2,0/4] more replacement of zero length array

Replace some missed zero length arrays not captured in the
original series.
https://patchwork.dpdk.org/project/dpdk/list/?series=30410&state=*

Zero length arrays are a GNU extension that has been
superseded by flex arrays.

https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html

v2:
    * added additional patches for fib & pipeline libs.
      series-acks have been placed only against original
      hash and rcu patches.

Tyler Retzlaff (4):
  hash: replace zero length array with flex array
  rcu: replace zero length array with flex array
  fib: replace zero length array with flex array
  pipeline: replace zero length array with flex array

 lib/fib/dir24_8.h           | 2 +-
 lib/fib/trie.h              | 2 +-
 lib/hash/rte_thash.c        | 4 ++--
 lib/pipeline/rte_pipeline.h | 2 +-
 lib/rcu/rcu_qsbr_pvt.h      | 2 +-
 5 files changed, 6 insertions(+), 6 deletions(-)
  

On Mon, 12 Feb 2024 14:36:02 -0800
Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:

> Replace some missed zero length arrays not captured in the
> original series.
> https://patchwork.dpdk.org/project/dpdk/list/?series=30410&state=*
> 
> Zero length arrays are a GNU extension that has been
> superseded by flex arrays.
> 
> https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
> 
> v2:
>     * added additional patches for fib & pipeline libs.
>       series-acks have been placed only against original
>       hash and rcu patches.
> 
> Tyler Retzlaff (4):
>   hash: replace zero length array with flex array
>   rcu: replace zero length array with flex array
>   fib: replace zero length array with flex array
>   pipeline: replace zero length array with flex array
> 
>  lib/fib/dir24_8.h           | 2 +-
>  lib/fib/trie.h              | 2 +-
>  lib/hash/rte_thash.c        | 4 ++--
>  lib/pipeline/rte_pipeline.h | 2 +-
>  lib/rcu/rcu_qsbr_pvt.h      | 2 +-
>  5 files changed, 6 insertions(+), 6 deletions(-)
> 

Series-acked-by: Stephen Hemminger <stephen@networkplumber.org>
  

> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Monday, 12 February 2024 23.57
> 
> On Mon, 12 Feb 2024 14:36:02 -0800
> Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:
> 
> > Replace some missed zero length arrays not captured in the
> > original series.
> > https://patchwork.dpdk.org/project/dpdk/list/?series=30410&state=*
> >
> > Zero length arrays are a GNU extension that has been
> > superseded by flex arrays.
> >
> > https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
> >
> > v2:
> >     * added additional patches for fib & pipeline libs.
> >       series-acks have been placed only against original
> >       hash and rcu patches.
> >
> > Tyler Retzlaff (4):
> >   hash: replace zero length array with flex array
> >   rcu: replace zero length array with flex array
> >   fib: replace zero length array with flex array
> >   pipeline: replace zero length array with flex array
> >
> >  lib/fib/dir24_8.h           | 2 +-
> >  lib/fib/trie.h              | 2 +-
> >  lib/hash/rte_thash.c        | 4 ++--
> >  lib/pipeline/rte_pipeline.h | 2 +-
> >  lib/rcu/rcu_qsbr_pvt.h      | 2 +-
> >  5 files changed, 6 insertions(+), 6 deletions(-)
> >
> 
> Series-acked-by: Stephen Hemminger <stephen@networkplumber.org>

Series-reviewed-by: Morten Brørup <mb@smartsharesystems.com>
  

On Mon, Feb 12, 2024 at 11:36 PM Tyler Retzlaff
<roretzla@linux.microsoft.com> wrote:
>
> Replace some missed zero length arrays not captured in the
> original series.
> https://patchwork.dpdk.org/project/dpdk/list/?series=30410&state=*
>
> Zero length arrays are a GNU extension that has been
> superseded by flex arrays.
>
> https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
>
> v2:
>     * added additional patches for fib & pipeline libs.
>       series-acks have been placed only against original
>       hash and rcu patches.

There seems to be an issue with the ABI check on those changes.
After a quick chat with Dodji, I opened a bug for libabigail.

https://sourceware.org/bugzilla/show_bug.cgi?id=31377
  

On Tue, Feb 13, 2024 at 02:14:28PM +0100, David Marchand wrote:
> On Mon, Feb 12, 2024 at 11:36 PM Tyler Retzlaff
> <roretzla@linux.microsoft.com> wrote:
> >
> > Replace some missed zero length arrays not captured in the
> > original series.
> > https://patchwork.dpdk.org/project/dpdk/list/?series=30410&state=*
> >
> > Zero length arrays are a GNU extension that has been
> > superseded by flex arrays.
> >
> > https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
> >
> > v2:
> >     * added additional patches for fib & pipeline libs.
> >       series-acks have been placed only against original
> >       hash and rcu patches.
> 
> There seems to be an issue with the ABI check on those changes.
> After a quick chat with Dodji, I opened a bug for libabigail.
> 
> https://sourceware.org/bugzilla/show_bug.cgi?id=31377

I double checked again and I don't see the struct in question being
embedded as a field of another struct/union.  So I don't think there should
be an ABI change here.

I'm okay with the change being merged but if there is concern I can drop
this patch from the series.

> 
> 
> -- 
> David Marchand
  

On Tue, Feb 13, 2024 at 8:20 PM Tyler Retzlaff
<roretzla@linux.microsoft.com> wrote:
>
> On Tue, Feb 13, 2024 at 02:14:28PM +0100, David Marchand wrote:
> > On Mon, Feb 12, 2024 at 11:36 PM Tyler Retzlaff
> > <roretzla@linux.microsoft.com> wrote:
> > >
> > > Replace some missed zero length arrays not captured in the
> > > original series.
> > > https://patchwork.dpdk.org/project/dpdk/list/?series=30410&state=*
> > >
> > > Zero length arrays are a GNU extension that has been
> > > superseded by flex arrays.
> > >
> > > https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
> > >
> > > v2:
> > >     * added additional patches for fib & pipeline libs.
> > >       series-acks have been placed only against original
> > >       hash and rcu patches.
> >
> > There seems to be an issue with the ABI check on those changes.
> > After a quick chat with Dodji, I opened a bug for libabigail.
> >
> > https://sourceware.org/bugzilla/show_bug.cgi?id=31377
>
> I double checked again and I don't see the struct in question being
> embedded as a field of another struct/union.  So I don't think there should
> be an ABI change here.

That was and is still my understanding too.

The message we get when testing this series is:

                      type size hasn't changed
                      1 data member change:
                        'uint8_t action_data[]' has *some* difference
- please report as a bug

which is why I reached out to Dodji (libabigail maintainer).

Dodji explained me that zero length / flex arrays conversion is
something he has been working on, and there are still some rough
edges.
This message is there so that libabigail community gets more input on
real life cases to handle.


>
> I'm okay with the change being merged but if there is concern I can drop
> this patch from the series.

At least, we can't merge it in the current form.

If libabigail gets a fix quickly, DPDK CI will still need a released version.
So for this patch to be merged now, we need a libabigail suppression rule.
I don't see a way to precisely waive this issue, so my suggestion is
to silence any change on the concerned structure here (which should be
ok, as the pipeline library data struct have been super stable for a
couple of years).
Something like:

$ git diff
diff --git a/devtools/libabigail.abignore b/devtools/libabigail.abignore
index 21b8cd6113..d667157909 100644
--- a/devtools/libabigail.abignore
+++ b/devtools/libabigail.abignore
@@ -33,3 +33,5 @@
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 ; Temporary exceptions till next major ABI version ;
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
+[suppress_type]
+       name = rte_pipeline_table_entry
  

On Wed, Feb 14, 2024 at 8:36 AM David Marchand
<david.marchand@redhat.com> wrote:
> > I'm okay with the change being merged but if there is concern I can drop
> > this patch from the series.
>
> At least, we can't merge it in the current form.
>
> If libabigail gets a fix quickly, DPDK CI will still need a released version.
> So for this patch to be merged now, we need a libabigail suppression rule.
> I don't see a way to precisely waive this issue, so my suggestion is
> to silence any change on the concerned structure here (which should be
> ok, as the pipeline library data struct have been super stable for a
> couple of years).
> Something like:
>
> $ git diff
> diff --git a/devtools/libabigail.abignore b/devtools/libabigail.abignore
> index 21b8cd6113..d667157909 100644
> --- a/devtools/libabigail.abignore
> +++ b/devtools/libabigail.abignore
> @@ -33,3 +33,5 @@
>  ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
>  ; Temporary exceptions till next major ABI version ;
>  ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
> +[suppress_type]
> +       name = rte_pipeline_table_entry

Dodji confirmed the issue in libabigail and prepared a fix.

DPDK still needs a suppression rule (like the one proposed above) if
we want to merge this change before the libabigail fix makes it to all
distribs.
Please resubmit this series with my proposal and a comment pointing at
libabigail bz squashed in patch 4.
  

On Fri, Feb 16, 2024 at 11:14:27AM +0100, David Marchand wrote:
> On Wed, Feb 14, 2024 at 8:36 AM David Marchand
> <david.marchand@redhat.com> wrote:
> > > I'm okay with the change being merged but if there is concern I can drop
> > > this patch from the series.
> >
> > At least, we can't merge it in the current form.
> >
> > If libabigail gets a fix quickly, DPDK CI will still need a released version.
> > So for this patch to be merged now, we need a libabigail suppression rule.
> > I don't see a way to precisely waive this issue, so my suggestion is
> > to silence any change on the concerned structure here (which should be
> > ok, as the pipeline library data struct have been super stable for a
> > couple of years).
> > Something like:
> >
> > $ git diff
> > diff --git a/devtools/libabigail.abignore b/devtools/libabigail.abignore
> > index 21b8cd6113..d667157909 100644
> > --- a/devtools/libabigail.abignore
> > +++ b/devtools/libabigail.abignore
> > @@ -33,3 +33,5 @@
> >  ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
> >  ; Temporary exceptions till next major ABI version ;
> >  ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
> > +[suppress_type]
> > +       name = rte_pipeline_table_entry
> 
> Dodji confirmed the issue in libabigail and prepared a fix.
> 
> DPDK still needs a suppression rule (like the one proposed above) if
> we want to merge this change before the libabigail fix makes it to all
> distribs.
> Please resubmit this series with my proposal and a comment pointing at
> libabigail bz squashed in patch 4.

this works out conveniently, i noticed there are a few more instances
that i'll try to add to this series so i'll come back with a new rev.

i've marked the series changes requested in patchwork for now.

> 
> 
> -- 
> David Marchand
  

Hello,

David Marchand <david.marchand@redhat.com> writes:

> Dodji confirmed the issue in libabigail and prepared a fix.

Yes, that is correct. Sorry for the inconvenience.

The patch fixing this issue in libabigail has been applied to the
mainline and is
https://sourceware.org/git/?p=libabigail.git;a=commit;h=3cc1c3423c89c2cfd9d451ab99b71f3a74b35127.

> DPDK still needs a suppression rule (like the one proposed above) if
> we want to merge this change before the libabigail fix makes it to all
> distribs.

We can discuss further if you need a "quick" 2.4.1 release as a vehicle
to consume the fix or if you can wait for a 2.5 one that might come later.

I agree however that in the mean time, a temporary suppression
specification might be warranted.

[...]

Cheers,