RE: [EXTERNAL] Re: [PATCH] [RFC] cryptodev: replace LIST_END enumerators with APIs

[Akhil Goyal <gakhil@marvell.com>]

> Hello,
> 
> Ferruh Yigit <ferruh.yigit@amd.com> writes:
> 
> > On 9/5/2024 11:14 AM, Akhil Goyal wrote:
> >> Replace *_LIST_END enumerators from asymmetric crypto
> >> lib to avoid ABI breakage for every new addition in
> >> enums with inline APIs.
> >>
> >> Signed-off-by: Akhil Goyal <gakhil@marvell.com>
> >> ---
> >> This patch was discussed in ML long time back.
> >> https://patches.dpdk.org/project/dpdk/patch/20211008204516.3497060-1-gakhil@marvell.com/
> >> Now added inline APIs for getting the list end which need to be updated
> >> for each new entry to the enum. This shall help in avoiding ABI break
> >> for adding new algo.
> >>
> >
> > Hi Akhil,
> >
> > *I think* this hides the problem instead of fixing it, and this may be
> > partially because of the tooling (libabigail) limitation.
> 
> So, I am not sure to understand how this would be a libabigail
> limitation in general.  Let me explain.
> 
> The abidiff tool that compares two binaries and emits reports about
> their ABI changes can be given a suppression specification file which
> instructs the tool about what type of change to avoid emitting reports
> about.
> 
> It turns out there is a construct specifically designed to handle the
> case where an enumerator is added right before the last enumerator of a
> enum.  Such a change causes the value of the last enumerator to
> increase.
> 
> For instance, consider this enum:
> 
>     enum foo_enum
>     {
>      FOO_FIRST,
>      FOO_SECOND,
>      FOO_END
>     };
> 
> If I add a new enumerator right before the last enumerator, I would get
> this:
> 
>     enum foo_enum
>     {
>      FOO_FIRST,
>      FOO_SECOND,
>      FOO_THIRD,
>      FOO_END
>     };
> 
> This change cause the value of the the FOOD_END enumerator to increase.
> And that increase might be problematic.  At the moment, for it being
> problematic or not has to be the result of a careful review.
> 
> So, by default, abidiff will complain by saying that the value of
> FOO_END was changed.
> 
> But you, as a community of practice, can decide that this kind of change
> to the value of the last enumerator is not a problematic change, after
> careful review of your code and practice.  You thus can specify that
> the tool using a suppression specification which has the following
> syntax:
> 
>     [suppress_type]
>       type_kind = enum
>       changed_enumerators = FOO_END, ANOTHER_ENUM_END,
> AND_ANOTHER_ENUM_END
> 
> or, alternatively, you can specify the enumerators you want to suppress
> the changes for as a list of regular expressions:
> 
>     [suppress_type]
>       type_kind = enum
>       changed_enumerators_regexp = .*_END$, .*_LAST$, .*_LIST_END$
> 
> Wouldn't that be enough to address your use case here (honest question)?
> 
> > This patch prevents the libabigail warning, true, but it doesn't improve
> > anything from the application's perspective.
> > Before or after this patch, application knows a fixed value as END value.
> >
> > Not all changes in the END (MAX) enum values cause ABI break, but tool
> > warns on all, that is why I think this may be tooling limitation [1].
> > (Just to stress, I am NOT talking about regular enum values change, I am
> > talking about only END (MAX) value changes caused by appending new enum
> > items.)
> >
> > As far as I can see (please Dodji, David correct me if I am wrong) ABI
> > break only happens if application and library exchange enum values in
> > the API (directly or within a struct).
> 
> Sorry, I am not sure to understand what you mean by "exchange enum values".
> 
> I would say is that if a change to an enumerator value causes a change
> to the layout of a type which is part of the ABI, then that enumerator
> value change might change the ABI.  That ABI change might be problematic
> (i.e an ABI break) or not.
> 
> >
> > Exchanging enum values via API cause ABI issues because:
> > 1. enum size is not fixed, it uses min storage size that can hold all
> > values, adding new enums may cause its size change and of course this
> > breaks ABI.
> > 2. Library can send enum values that application doesn't know, this may
> > cause application behave undefined.
> > 3. Application sending MAX value (or more) to API expects error, but
> > that may be a valid value in the new library and applications gets
> > unexpected result.
> >
> >
> > Let's assume above 1. happens, with this patch warning is prevented, but
> > ABI break still can occur.
> >
> > One option can be not exchanging enums in APIs at all, this way changes
> > in the END (MAX) enum values are harmless. But I can see cryptodev does
> > this a lot.
> >
> > When enum is exchanged in APIs, and if we want to be strict about the
> > ABI, safest option can be not appending to enums at all, and keeping END
> > (MAX) items can be a way to enable warnings for this.
> >
> >
> > More relaxed option is protect against only 1. since that is the real
> > ABI issue, 2 & 3 are more defensive programming, so as long as enum size
> > is not changed we may ignore the END (MAX) value change warnings.
> >
> >
> >
> > [1] It would be better if tool gives END (MAX) enum value warnings only
> > if it is exchanged in an API, but not sure if this can be possible to
> > detect.
> 
> I believe that if you want to know if an enumerator value is *USED* by a
> type (which I believe is at the root of what you are alluding to), then
> you would need a static analysis tool that works at the source level.
> Or, you need a human review of the code once the binary analysis tool
> told you that that value of the enumerator changed.
> 
> Why ? please let me give you an example:
> 
>     enum foo_enum
>     {
>      FOO_FIRST,
>      FOO_SECOND,
>      FOO_END
>     };
> 
>     int array[FOO_END];
> 
> Once this is compiled into binary, what libabigail is going to see by
> analyzing the binary is that 'array' is an array of 2 integers.  The
> information about the size of the array being initially an enumerator
> value is lost.  To detect that, you need source level analysis.
> 
> But then, by reviewing the code, this is a construct that you can spot
> and allow or forbid, depending on your goals as a project.
> 
In the above example if in newer library a FOO_THIRD is added.
FOO_END value will change and will cause ABI break for change in existing value.
But if we replace it with inline function to get the list_end and use it in array like below.
So if FOO_THIRD is added, we will also update foo_enum_list_end() function to return (FOO_THIRD+1)

     enum foo_enum
     {
      FOO_FIRST,
      FOO_SECOND,
     };
     static inline int foo_enum_list_end()
     {
            return FOO_SECOND + 1;
     }
     int array[foo_enum_list_end()];

Will this cause an ABI break if we add this array in application or in library?