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

[Dodji Seketeli <dodji@redhat.com>]

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 :-)


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

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,

-- 
		Dodji