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

[Dodji Seketeli <dodji@redhat.com>]

Hello,

Ferruh Yigit <ferruh.yigit@amd.com> writes:

[...]

>> 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.
>> 
>
> As you said, FOOD_END value change can be sometimes problematic, but
> sometimes it is not.
> This what I referred as limitation that tool is not reporting only
> problematic case, but require human review.

Oh, I see. Thank you for clarifying.

> (btw, this is a very useful tool, I don't want to sound like negative
> about it, only want to address this recurring subject in dpdk.)

No problem, I never assume you mean anything negative :-)

[...]


>> 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)?
>> 
>
> We are already using suppress feature in dpdk.
>
> But difficulty is to decide if END (MAX) enum value change warning is an
> actual ABI break or not.
>
> When tool gives warning, tendency is to just make warning go away,
> mostly by removing END (MAX) enum without really analyzing if it is a
> real ABI break.

I see.

[...]

>>> [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.
>> 
>
> I see the problem.
>
> Is this the main reason that changing FOO_END value reported as warning?

Yes, it is because of this class of issues.

Actually if ANY enumerator value is changed, that is actually an ABI
change.  And that ABI change is either compatible or not.

> If we forbid this kind of usage of the FOO_END, can we ignore this
> warning safely?

I would think so.


But then, you'd have to also forbid the use of all enumerators,
basically.  I am not sure that would be practical.

Rather I would tend to lean toward reviewing the use of enumerators, on
a case by case basis, using tools like 'grep' and whatnot.

What I would advise to forbid is the use of complicated macros or
constructs that makes the review of the use of enumerators
non-practical.  You should be able to grep "FOO_END" and see where it's
used in the source code.  Reviewing that shouldn't take more than a few
minutes whenever a tool warns about the change of its value.

>
>> 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.
>> 
>> [...]
>> 
>> Cheers,
>> 
>

-- 
		Dodji