[dpdk-dev] /validate-abi.sh complains [PATCH v1 3/8] ring: introduce RTS ring mode

[Ananyev, Konstantin]

Hi David,

> > Have a question regarding validate-abi.sh.
> > It complains on the following changes with that patch:
> >
> > @@ -111,11 +129,21 @@ struct rte_ring {
> >         char pad0 __rte_cache_aligned; /**< empty cache line */
> >
> >         /** Ring producer status. */
> > -       struct rte_ring_headtail prod __rte_cache_aligned;
> > +       RTE_STD_C11
> > +       union {
> > +               struct rte_ring_headtail prod;
> > +               struct rte_ring_rts_headtail rts_prod;
> > +       }  __rte_cache_aligned;
> > +
> >         char pad1 __rte_cache_aligned; /**< empty cache line */
> >
> >         /** Ring consumer status. */
> > -       struct rte_ring_headtail cons __rte_cache_aligned;
> > +       RTE_STD_C11
> > +       union {
> > +               struct rte_ring_headtail cons;
> > +               struct rte_ring_rts_headtail rts_cons;
> > +       }  __rte_cache_aligned;
> > +
> >
> > Complaints:
> > rte_ring.h
> > [−] struct rte_ring  2
> >
> > 1
> > Change: Field cons has been removed from this type.
> > Effect:    Applications will access incorrect memory when attempting to access this field.
> > 2
> > Change: Field prod has been removed from this type.
> > Effect:    Applications will access incorrect memory when attempting to access this field.
> >
> > From my perspective it looks false-positive:
> > *prod* and *cons* fields are still there,
> > their format, size and offset within rte_ring remain the same.
> > Is that some limitation with the tool, or am I missing something here?
> 
> - Side note, we have build failures with clang and ARM jobs:
> https://travis-ci.com/github/ovsrobot/dpdk/builds/157277423

ack, will fix.

> 
> 
> - We switched to libabigail called from devtools/check-abi.sh which
> you can run locally
> (https://doc.dpdk.org/guides/contributing/patches.html?highlight=abi#checking-abi-compatibility).
> Or you can count on Aaron's robot to do this check in Travis.
> 
> It reported a warning on those fields:
> https://travis-ci.com/github/ovsrobot/dpdk/jobs/310689008#L2380
> I understand this as a false positive too.
> 
> It seems similar to the bz I opened about fields moved to anonymous
> constructs: https://sourceware.org/bugzilla/show_bug.cgi?id=25661

Yes, looks the same.


> Cc: Dodji.
> 
> 
> For the time being, you can waive this by adding a rule in
> devtools/libabigail.abignore.

Ok, so what is the procedure here?
Should I submit changes to devtools/libabigail.abignore together with the
patch that introduced the problem?
BTW, I used the following changes in libabigail.abignore to supress errors:
$ git diff devtools/libabigail.abignore
diff --git a/devtools/libabigail.abignore b/devtools/libabigail.abignore
index a59df8f13..032479b9f 100644
--- a/devtools/libabigail.abignore
+++ b/devtools/libabigail.abignore
@@ -11,3 +11,9 @@
         type_kind = enum
         name = rte_crypto_asym_xform_type
         changed_enumerators = RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
+; Ignore updates of ring prod/cons
+[suppress_type]
+        type_kind = struct
+        name = rte_ring
+        has_data_members_inserted_at = offsetof(prod)
+        has_data_members_inserted_at = offsetof(cons)

Do you know, is there a better (more fine-grained) approach?
Thanks
Konstantin