Re: [dpdk-dev] [PATCH v1] devtools: update abi ignore for cryptodev

[Thomas Monjalon <thomas@monjalon.net>]

21/01/2021 16:15, Dodji Seketeli:
> Hello Thomas and others,
> 
> Thomas Monjalon <thomas@monjalon.net> writes:
> 
> > Question to an expert, Dodji,
> 
> Thanks for the kind words, but I am not an expert in anything, sadly.  I
> am just trying to keep learning about these things ;-)
> 
> > We have this structure:
> >
> > struct rte_cryptodev {
> > 	lot of fields...
> > 	uint8_t attached : 1;
> > } __rte_cache_aligned;
> >
> > Because of the cache alignment, there is enough padding in the struct
> > (no matter the size of the cache line) for adding two more pointers:
> >
> > struct rte_cryptodev {
> > 	lot of fields...
> > 	uint8_t attached : 1;
> > 	struct rte_cryptodev_cb_rcu *enq_cbs;
> > 	struct rte_cryptodev_cb_rcu *deq_cbs;
> > } __rte_cache_aligned;
> >
> > We checked manually that the ABI is still compatible.
> 
> Right.
> 
> I am curious, but normally, libabigail should raise the addition of
> structures, but then it'll tell you that there was no size or offset
> change between the two structures.  If it doesn't, then that's a bug.  I
> hope it does :-)

Yes it was raising a problem, that's why we are adding a rule.


> > Then I've added (quickly) a libabigail exception rule:
> >
> > [suppress_type]
> > 	name = rte_cryptodev
> > 	has_data_member_inserted_between = {0, 1023}
> >
> > Now we want to improve this rule to restrict the offsets
> > to the padding at the end of the struct only,
> > so we keep forbidding changes in existing fields,
> > and forbidding additions further the current struct size.
> > Is this new rule good?
> >
> > 	has_data_member_inserted_between = {offset_after(attached), end}
> 
> 
> Yes, this rule should do what you think it says.
> 
> > Do you confirm that the keyword "end" means the old reference size?
> 
> Yes I do.
> 
> 
> > What else do we need to check for adding a new field in a padding?
> 
> Actually, that rule will work independantly of it there is enough
> padding or not.  It'll shut down the change report, even if the added
> data exceeds the padding.

I don't understand why.
If "end" means the old reference size, then addition after the old size
should be reported, isn't it?


> You just made me think of an idea of a new feature there.
> 
> Maybe we'd need a new property for the [suppress_type] directive that
> would suppress changes only if said changes don't modify the size of the
> type or any offset of any member of the type?
> 
> Maybe something like:
> 
>     [suppress_type]
>        ; lots of properties can go here.
> 
>        ; ...
> 
>        ; If the type has any size or offset change
>        ; then this suppression directive will fail
>        ; and the change report will be emitted
>        has_no_size_or_offset_change
> 
> Would that be useful to you in this case,
> 
> Cheers,