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

[Dodji Seketeli <dodji@redhat.com>]

Hello,

Akhil Goyal <gakhil@marvell.com> writes:

[...]


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

I think this (inline function) construct is essentially the same as just
adding a FOO_END enumerator after FOO_SECOND.  Using either
foo_enum_list_end() or FOO_END result in having the value '2' in the
application using the library to get FOO_END or foo_enum_list_end().

Newer versions of the library being linked to the application won't
change that value '2', regardless of the newer values of FOO_END or
foo_enum_list_end().

So, adding a FOO_THIRD right after FOO_END, induces and ABI change.

This change being "breaking" (incompatible) or not, really depends on
what the application expects, I would say.  Sorry if that looks "vague",
but this whole concept is quite blurry.

For instance, if you add FOO_THIRD after FOO_SECOND in the newer version
of the library and the application still gets the value '2' rather than
getting the value '3', and that value is actually multiplied by "two
trillions" in the application to get the value of the dividend to be
payed to investors, then, then that's a very big error induced by that
change.  That might be considered by the application as a "breaking" ABI
change and you might get a call or two from the CEO of an S&P500 company
that uses the library.

Other applications might consider that "off-by-one" error not being
problematic at all and thus might consider it not "breaking".

Note that REMOVING an enumerator however is always considered an
incompatible (breaking) ABI change.

Adding an enumerator however has this annoying "grey topic" (not black or
white) property that I am not sure how to address at this point.

Cheers,

-- 
		Dodji