From: Akhil Goyal <gakhil@marvell.com>
To: Dodji Seketeli <dodji@redhat.com>, Ferruh Yigit <ferruh.yigit@amd.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
"thomas@monjalon.net" <thomas@monjalon.net>,
"david.marchand@redhat.com" <david.marchand@redhat.com>,
"hemant.agrawal@nxp.com" <hemant.agrawal@nxp.com>,
Anoob Joseph <anoobj@marvell.com>,
"pablo.de.lara.guarch@intel.com" <pablo.de.lara.guarch@intel.com>,
"fiona.trahe@intel.com" <fiona.trahe@intel.com>,
"declan.doherty@intel.com" <declan.doherty@intel.com>,
"matan@nvidia.com" <matan@nvidia.com>,
"g.singh@nxp.com" <g.singh@nxp.com>,
"fanzhang.oss@gmail.com" <fanzhang.oss@gmail.com>,
"jianjay.zhou@huawei.com" <jianjay.zhou@huawei.com>,
"asomalap@amd.com" <asomalap@amd.com>,
"ruifeng.wang@arm.com" <ruifeng.wang@arm.com>,
"konstantin.v.ananyev@yandex.ru" <konstantin.v.ananyev@yandex.ru>,
"radu.nicolau@intel.com" <radu.nicolau@intel.com>,
"ajit.khaparde@broadcom.com" <ajit.khaparde@broadcom.com>,
Nagadheeraj Rottela <rnagadheeraj@marvell.com>,
"mdr@ashroe.eu" <mdr@ashroe.eu>
Subject: RE: [EXTERNAL] Re: [PATCH] [RFC] cryptodev: replace LIST_END enumerators with APIs
Date: Fri, 4 Oct 2024 17:45:23 +0000 [thread overview]
Message-ID: <CO6PR18MB448457699F3B656AFB80A081D8722@CO6PR18MB4484.namprd18.prod.outlook.com> (raw)
In-Reply-To: <87msjkdx7x.fsf@redhat.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?
next prev parent reply other threads:[~2024-10-04 17:46 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-09-05 10:14 Akhil Goyal
2024-09-05 15:09 ` Morten Brørup
2024-09-05 15:26 ` Kusztal, ArkadiuszX
2024-09-06 6:32 ` fengchengwen
2024-09-06 7:45 ` [EXTERNAL] " Akhil Goyal
2024-10-04 3:56 ` Ferruh Yigit
2024-09-06 7:54 ` Morten Brørup
2024-09-23 20:41 ` Akhil Goyal
2024-10-03 7:00 ` Akhil Goyal
2024-10-06 11:10 ` Morten Brørup
2024-10-09 11:21 ` Akhil Goyal
2024-10-04 4:00 ` Ferruh Yigit
2024-10-04 17:26 ` [EXTERNAL] " Akhil Goyal
2024-10-04 3:54 ` Ferruh Yigit
2024-10-04 7:04 ` David Marchand
2024-10-04 17:27 ` [EXTERNAL] " Akhil Goyal
2024-10-10 0:49 ` Ferruh Yigit
2024-10-10 6:18 ` [EXTERNAL] " Akhil Goyal
2024-10-28 11:15 ` Dodji Seketeli
2024-10-04 9:38 ` Dodji Seketeli
2024-10-04 17:45 ` Akhil Goyal [this message]
2024-10-28 10:55 ` [EXTERNAL] " Dodji Seketeli
2024-10-10 0:35 ` Ferruh Yigit
2024-10-28 10:12 ` Dodji Seketeli
2024-10-09 11:24 ` [PATCH] cryptodev: remove unnecessary list end Akhil Goyal
2024-10-09 12:52 ` Morten Brørup
2024-10-09 20:38 ` Akhil Goyal
2024-10-09 14:06 ` Hemant Agrawal
2024-10-10 0:51 ` Ferruh Yigit
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CO6PR18MB448457699F3B656AFB80A081D8722@CO6PR18MB4484.namprd18.prod.outlook.com \
--to=gakhil@marvell.com \
--cc=ajit.khaparde@broadcom.com \
--cc=anoobj@marvell.com \
--cc=asomalap@amd.com \
--cc=david.marchand@redhat.com \
--cc=declan.doherty@intel.com \
--cc=dev@dpdk.org \
--cc=dodji@redhat.com \
--cc=fanzhang.oss@gmail.com \
--cc=ferruh.yigit@amd.com \
--cc=fiona.trahe@intel.com \
--cc=g.singh@nxp.com \
--cc=hemant.agrawal@nxp.com \
--cc=jianjay.zhou@huawei.com \
--cc=konstantin.v.ananyev@yandex.ru \
--cc=matan@nvidia.com \
--cc=mdr@ashroe.eu \
--cc=pablo.de.lara.guarch@intel.com \
--cc=radu.nicolau@intel.com \
--cc=rnagadheeraj@marvell.com \
--cc=ruifeng.wang@arm.com \
--cc=thomas@monjalon.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).