[v10,1/4] devtools: add exception for reserved fields

  From: Akhil Goyal <gakhil@marvell.com>

Certain structures are added with reserved fields
to address any future enhancements to retain ABI
compatibility.
However, ABI script will still report error as it
is not aware of reserved fields. Hence, adding a
generic exception for reserved fields.

Signed-off-by: Akhil Goyal <gakhil@marvell.com>
---
 devtools/libabigail.abignore | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
  

On Wed, Apr 14, 2021 at 8:04 PM <gakhil@marvell.com> wrote:
>
> From: Akhil Goyal <gakhil@marvell.com>
>
> Certain structures are added with reserved fields
> to address any future enhancements to retain ABI
> compatibility.
> However, ABI script will still report error as it
> is not aware of reserved fields. Hence, adding a
> generic exception for reserved fields.
>
> Signed-off-by: Akhil Goyal <gakhil@marvell.com>
> ---
>  devtools/libabigail.abignore | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/devtools/libabigail.abignore b/devtools/libabigail.abignore
> index 6c0b38984..654755314 100644
> --- a/devtools/libabigail.abignore
> +++ b/devtools/libabigail.abignore
> @@ -19,4 +19,8 @@
>  ; Ignore fields inserted in cacheline boundary of rte_cryptodev
>  [suppress_type]
>          name = rte_cryptodev
> -        has_data_member_inserted_between = {offset_after(attached), end}
> \ No newline at end of file
> +        has_data_member_inserted_between = {offset_after(attached), end}
> +
> +; Ignore changes in reserved fields
> +[suppress_variable]
> +       name_regexp = reserved
> --
> 2.25.1
>

Mm, this rule is a bit scary, as it matches anything with "reserved" in it.

You need an exception anyway to insert the new fields (like in patch 2).
Can you test your series dropping this patch 1 ?
  

Hi David,
> > Certain structures are added with reserved fields
> > to address any future enhancements to retain ABI
> > compatibility.
> > However, ABI script will still report error as it
> > is not aware of reserved fields. Hence, adding a
> > generic exception for reserved fields.
> >
> > Signed-off-by: Akhil Goyal <gakhil@marvell.com>
> > ---
> >  devtools/libabigail.abignore | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/devtools/libabigail.abignore b/devtools/libabigail.abignore
> > index 6c0b38984..654755314 100644
> > --- a/devtools/libabigail.abignore
> > +++ b/devtools/libabigail.abignore
> > @@ -19,4 +19,8 @@
> >  ; Ignore fields inserted in cacheline boundary of rte_cryptodev
> >  [suppress_type]
> >          name = rte_cryptodev
> > -        has_data_member_inserted_between = {offset_after(attached), end}
> > \ No newline at end of file
> > +        has_data_member_inserted_between = {offset_after(attached), end}
> > +
> > +; Ignore changes in reserved fields
> > +[suppress_variable]
> > +       name_regexp = reserved
> Mm, this rule is a bit scary, as it matches anything with "reserved" in it.

Why do you feel it is scary? Reserved is something which may change at any time
Just like experimental. Hence creating a generic exception rule for it make sense
And it is done intentionally in this patch.

> 
> You need an exception anyway to insert the new fields (like in patch 2).
> Can you test your series dropping this patch 1 ?
It will not work, as there are 2 changes,
1. addition of ca_enqueue after attached. This is taken care by the exception set in patch 2
2. change in the reserved_ptr[4] -> reserved_ptr[3]. For this change we need exception for reserved.

Regards,
Akhil
  

On Thu, Apr 15, 2021 at 7:33 AM Akhil Goyal <gakhil@marvell.com> wrote:
>
> Hi David,
> > > Certain structures are added with reserved fields
> > > to address any future enhancements to retain ABI
> > > compatibility.
> > > However, ABI script will still report error as it
> > > is not aware of reserved fields. Hence, adding a
> > > generic exception for reserved fields.
> > >
> > > Signed-off-by: Akhil Goyal <gakhil@marvell.com>
> > > ---
> > >  devtools/libabigail.abignore | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/devtools/libabigail.abignore b/devtools/libabigail.abignore
> > > index 6c0b38984..654755314 100644
> > > --- a/devtools/libabigail.abignore
> > > +++ b/devtools/libabigail.abignore
> > > @@ -19,4 +19,8 @@
> > >  ; Ignore fields inserted in cacheline boundary of rte_cryptodev
> > >  [suppress_type]
> > >          name = rte_cryptodev
> > > -        has_data_member_inserted_between = {offset_after(attached), end}
> > > \ No newline at end of file
> > > +        has_data_member_inserted_between = {offset_after(attached), end}
> > > +
> > > +; Ignore changes in reserved fields
> > > +[suppress_variable]
> > > +       name_regexp = reserved
> > Mm, this rule is a bit scary, as it matches anything with "reserved" in it.
>
> Why do you feel it is scary? Reserved is something which may change at any time
> Just like experimental. Hence creating a generic exception rule for it make sense
> And it is done intentionally in this patch.

The reserved regexp on the name of a variable / struct field is too lax.
Anything could be named with reserved in it.
If we have clear patterns, they must be preferred, like (untested)
name_regexp = ^reserved_(64|ptr)s$


Experimental is different.
This is a symbol version tag, which has a clear meaning and can't be
used for anything else.


>
> >
> > You need an exception anyway to insert the new fields (like in patch 2).
> > Can you test your series dropping this patch 1 ?
> It will not work, as there are 2 changes,
> 1. addition of ca_enqueue after attached. This is taken care by the exception set in patch 2
> 2. change in the reserved_ptr[4] -> reserved_ptr[3]. For this change we need exception for reserved.

In the eventdev struct, reserved fields are all in the range between
the attached field and the end of the struct.
I pushed your series without patch 1 to a branch of mine, and it
passes the check fine:
https://github.com/david-marchand/dpdk/commits/crypto_fwd_mode_v10
https://github.com/david-marchand/dpdk/runs/2350324578?check_suite_focus=true#step:15:8549
  

On Thu, Apr 15, 2021 at 09:26:38AM +0200, David Marchand wrote:
> On Thu, Apr 15, 2021 at 7:33 AM Akhil Goyal <gakhil@marvell.com> wrote:
> >
> > Hi David,
> > > > Certain structures are added with reserved fields
> > > > to address any future enhancements to retain ABI
> > > > compatibility.
> > > > However, ABI script will still report error as it
> > > > is not aware of reserved fields. Hence, adding a
> > > > generic exception for reserved fields.
> > > >
> > > > Signed-off-by: Akhil Goyal <gakhil@marvell.com>
> > > > ---
> > > >  devtools/libabigail.abignore | 6 +++++-
> > > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/devtools/libabigail.abignore b/devtools/libabigail.abignore
> > > > index 6c0b38984..654755314 100644
> > > > --- a/devtools/libabigail.abignore
> > > > +++ b/devtools/libabigail.abignore
> > > > @@ -19,4 +19,8 @@
> > > >  ; Ignore fields inserted in cacheline boundary of rte_cryptodev
> > > >  [suppress_type]
> > > >          name = rte_cryptodev
> > > > -        has_data_member_inserted_between = {offset_after(attached), end}
> > > > \ No newline at end of file
> > > > +        has_data_member_inserted_between = {offset_after(attached), end}
> > > > +
> > > > +; Ignore changes in reserved fields
> > > > +[suppress_variable]
> > > > +       name_regexp = reserved
> > > Mm, this rule is a bit scary, as it matches anything with "reserved" in it.
> >
> > Why do you feel it is scary? Reserved is something which may change at any time
> > Just like experimental. Hence creating a generic exception rule for it make sense
> > And it is done intentionally in this patch.
> 
> The reserved regexp on the name of a variable / struct field is too lax.
> Anything could be named with reserved in it.
> If we have clear patterns, they must be preferred, like (untested)
> name_regexp = ^reserved_(64|ptr)s$
> 
+1 to have a clear name. I would suggest using a "__reserved" prefix, since
no real field name should ever start with that prefix.
  

15/04/2021 10:25, Bruce Richardson:
> On Thu, Apr 15, 2021 at 09:26:38AM +0200, David Marchand wrote:
> > On Thu, Apr 15, 2021 at 7:33 AM Akhil Goyal <gakhil@marvell.com> wrote:
> > > > > +; Ignore changes in reserved fields
> > > > > +[suppress_variable]
> > > > > +       name_regexp = reserved
> > > > 
> > > > Mm, this rule is a bit scary, as it matches anything with "reserved" in it.
> > >
> > > Why do you feel it is scary? Reserved is something which may change at any time
> > > Just like experimental. Hence creating a generic exception rule for it make sense
> > > And it is done intentionally in this patch.
> > 
> > The reserved regexp on the name of a variable / struct field is too lax.
> > Anything could be named with reserved in it.
> > If we have clear patterns, they must be preferred, like (untested)
> > name_regexp = ^reserved_(64|ptr)s$
> > 
> +1 to have a clear name. I would suggest using a "__reserved" prefix, since
> no real field name should ever start with that prefix.

+1 for the double underscore
Changing it now does not break API as it is not supposed to be used.
  

> > > >  ; Ignore fields inserted in cacheline boundary of rte_cryptodev
> > > >  [suppress_type]
> > > >          name = rte_cryptodev
> > > > -        has_data_member_inserted_between = {offset_after(attached),
> end}
> > > > \ No newline at end of file
> > > > +        has_data_member_inserted_between = {offset_after(attached),
> end}
> > > > +
> > > > +; Ignore changes in reserved fields
> > > > +[suppress_variable]
> > > > +       name_regexp = reserved
> > > Mm, this rule is a bit scary, as it matches anything with "reserved" in it.
> >
> > Why do you feel it is scary? Reserved is something which may change at
> any time
> > Just like experimental. Hence creating a generic exception rule for it make
> sense
> > And it is done intentionally in this patch.
> 
> The reserved regexp on the name of a variable / struct field is too lax.
> Anything could be named with reserved in it.
> If we have clear patterns, they must be preferred, like (untested)
> name_regexp = ^reserved_(64|ptr)s$
> 
> 
> Experimental is different.
> This is a symbol version tag, which has a clear meaning and can't be
> used for anything else.
> 
> 
> >
> > >
> > > You need an exception anyway to insert the new fields (like in patch 2).
> > > Can you test your series dropping this patch 1 ?
> > It will not work, as there are 2 changes,
> > 1. addition of ca_enqueue after attached. This is taken care by the
> exception set in patch 2
> > 2. change in the reserved_ptr[4] -> reserved_ptr[3]. For this change we
> need exception for reserved.
> 
> In the eventdev struct, reserved fields are all in the range between
> the attached field and the end of the struct.
> I pushed your series without patch 1 to a branch of mine, and it
> passes the check fine:
> https://github.com/david-marchand/dpdk/runs/2350324578?check_suite_focus=true#step:15:8549> 
> 
Yes it will work, I actually put the new field after reserved and
it was creating issues, so I added it.
But later I decided to move it above reserved fields and missed
that it will work without reserved exception.

Hence we can drop the first patch for now.

Regards,
Akhil
  

On Thu, 15 Apr 2021 08:31:30 +0000
Akhil Goyal <gakhil@marvell.com> wrote:

> > > > >  ; Ignore fields inserted in cacheline boundary of rte_cryptodev
> > > > >  [suppress_type]
> > > > >          name = rte_cryptodev
> > > > > -        has_data_member_inserted_between = {offset_after(attached),  
> > end}  
> > > > > \ No newline at end of file
> > > > > +        has_data_member_inserted_between = {offset_after(attached),  
> > end}  
> > > > > +
> > > > > +; Ignore changes in reserved fields
> > > > > +[suppress_variable]
> > > > > +       name_regexp = reserved  
> > > > Mm, this rule is a bit scary, as it matches anything with "reserved" in it.  
> > >
> > > Why do you feel it is scary? Reserved is something which may change at  
> > any time  
> > > Just like experimental. Hence creating a generic exception rule for it make  
> > sense  
> > > And it is done intentionally in this patch.  
> > 
> > The reserved regexp on the name of a variable / struct field is too lax.
> > Anything could be named with reserved in it.
> > If we have clear patterns, they must be preferred, like (untested)
> > name_regexp = ^reserved_(64|ptr)s$
> > 
> > 
> > Experimental is different.
> > This is a symbol version tag, which has a clear meaning and can't be
> > used for anything else.
> > 
> >   
> > >  
> > > >
> > > > You need an exception anyway to insert the new fields (like in patch 2).
> > > > Can you test your series dropping this patch 1 ?  
> > > It will not work, as there are 2 changes,
> > > 1. addition of ca_enqueue after attached. This is taken care by the  
> > exception set in patch 2  
> > > 2. change in the reserved_ptr[4] -> reserved_ptr[3]. For this change we  
> > need exception for reserved.
> > 
> > In the eventdev struct, reserved fields are all in the range between
> > the attached field and the end of the struct.
> > I pushed your series without patch 1 to a branch of mine, and it
> > passes the check fine:
> > https://github.com/david-marchand/dpdk/runs/2350324578?check_suite_focus=true#step:15:8549> 
> >   
> Yes it will work, I actually put the new field after reserved and
> it was creating issues, so I added it.
> But later I decided to move it above reserved fields and missed
> that it will work without reserved exception.
> 
> Hence we can drop the first patch for now.
> 
> Regards,
> Akhil

Is there a check that size didn't change?
For example if a reserved field was removed.