From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 360C478003C for ; Mon, 20 Nov 2023 03:57:41 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=W0Se7du2LeuXf7ipVkS8vlPrd5BRmN9aLT20IjKR9+U=; c=relaxed/simple; d=groups.io; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject:To:Cc:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type; s=20140610; t=1700452660; v=1; b=Ev9DwcbLWQ0nIpjPSuna8iyoK1+docECTBEVpP98a0IhLj6LrNK3rrbP1t9CdvqsyWBImEBy vcoLusrSOPl4grI+GlRdeFz498gqIWnmcjkTzjcBrk7HkeDvcxixYJaEYg8ri0tGYDfh45qXHaP IXE9roZ9AktrPWD6bwgoljz0= X-Received: by 127.0.0.2 with SMTP id dNskYY7687511xjfxFBIKcSB; Sun, 19 Nov 2023 19:57:40 -0800 X-Received: from mail-pj1-f41.google.com (mail-pj1-f41.google.com [209.85.216.41]) by mx.groups.io with SMTP id smtpd.web11.43697.1700452659960642905 for ; Sun, 19 Nov 2023 19:57:40 -0800 X-Received: by mail-pj1-f41.google.com with SMTP id 98e67ed59e1d1-2802e5ae23bso3428261a91.2 for ; Sun, 19 Nov 2023 19:57:39 -0800 (PST) X-Gm-Message-State: LfuiYoIftlCkRVIyjS8MUABgx7686176AA= X-Google-Smtp-Source: AGHT+IHQ9Oz0M+ovRwlHVNEkZM9Mi4LgrJBstjVC5DNdrt/Ar/WIAHtu5+JQu1mn/9j7paY0Q7vyIQ5egvsJmgxgfEk= X-Received: by 2002:a17:90a:ec01:b0:280:c0:9d3f with SMTP id l1-20020a17090aec0100b0028000c09d3fmr7036038pjy.34.1700452659220; Sun, 19 Nov 2023 19:57:39 -0800 (PST) MIME-Version: 1.0 References: <20231107061959.113213-1-rsingh@ventanamicro.com> <20231107061959.113213-5-rsingh@ventanamicro.com> <5a7f3c56-d18f-9e4c-0e70-9113923ee36d@redhat.com> <5443d245-0631-a33f-220f-81e39e33af4a@redhat.com> In-Reply-To: <5443d245-0631-a33f-220f-81e39e33af4a@redhat.com> From: "Ranbir Singh" Date: Mon, 20 Nov 2023 09:27:28 +0530 Message-ID: Subject: Re: [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue To: Laszlo Ersek Cc: "Kinney, Michael D" , "devel@edk2.groups.io" , "Andrew Fish (afish@apple.com)" , "quic_llindhol@quicinc.com" , Michael Kubacki , "Ni, Ray" , Veeresh Sangolli Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,rsingh@ventanamicro.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Content-Type: multipart/alternative; boundary="000000000000b5e5f9060a8d7cc8" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=Ev9DwcbL; dmarc=none; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io --000000000000b5e5f9060a8d7cc8 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Wed, Nov 15, 2023 at 3:20=E2=80=AFPM Laszlo Ersek wr= ote: > On 11/14/23 17:21, Kinney, Michael D wrote: > > Hi Ranbir, > > > > > > > > First I want to recognize your efforts to collect Coverity issues and > > propose changes to address > > them. > > > > I still disagree with adding CpuDealLoop() for any static analysis > issues. > > > > There have been previous discussions about adding a PANIC() or FATAL() > > macro that would > > perform platform specific actions if a condition is detected where the > > boot of the platform > > can not continue. A platform get to make the choice to log or reboot o= r > > hang, etc. Not the > > code that detected the condition. > > > > https://edk2.groups.io/g/devel/message/86597 > > > > This is indeed a great idea. > > I didn't know about that discussion. Perhaps a new DebugLib API would > handle this (i.e., "panic"). > > I've been certainly proposing CpuDeadLoop() as a means to panic. > > Did the thread conclude anything tangible? > > > Unfortunately, in order to fix some of these static analysis issues > > correctly, we are going > > to have to identify the use of ASSERT() that really is a fatal conditio= n > > that can not continue > > Absolutely. > > > and introduce an implementation approach that provides a platform > > handler and > > satisfies the static analysis tools. > > The "platform handler" is the difficult part. If the above-noted > discussion from 2022 didn't produce an agreement, should we really block > the static analyzer fixes on an agreed-upon panic API? I'm concerned > that would just cause these fixes to get stuck. And I don't consider > CpuDeadLoop()s added for this purpose serious technical debt. They are > easy to find and update later, assuming we also add comments. > > I agree with the approach to not gate current fixes adding CpuDeadLoop(). Later on, it can be updated with the desired panic API and I can contribute for those required changes related to patches submitted by me. I can update current patches to carry additional comment in suffix form to ease later search like CpuDeadLoop (); // TBD: replace with Panic API in future Laszlo, Mike - Let me know if that works for now. > > We also have to evaluate if a return error status with a DEBUG_ERROR > > message would be a better > > choice than an ASSERT() that can be filtered out by build options. > > I agree 100% that this would be better for the codebase, but the work > needed for this is (IMO) impossible to contain. ASSERT() has been abused > for a long time *because* it seemed to allow the programmer to ignore > any related error paths. If we now want to implement those error paths > retroactively (which would be absolutely the right thing to do from a > software engineering perspective), then immense amounts of work are > going to be needed (patch review and regression testing), and I think > it's just not practical to dump all that on someone that wants to > improve the status quo. Replacing an invalid ASSERT() with a panic is > honest about the current situation, is safer regarding RELEASE builds, > and its work demand (regression testing, patch review) is tolerable. > > I do agree that, if the error path mostly exists already, then returning > errors for data/environment-based error conditions (i.e., not for > algorithmic invariant failures) is best. > > Where we need to be extremely vigilant is new patches. We must > uncompromisingly reject *new* abuses of ASSERT(), in new patches. > > Anyway, it seems that we've been trying to steer Ranbir in opposite > directions. I'll let you take the lead on this; for one, I've not been > aware of the panic api discussion for 2022! > > (I don't feel especially pushy about fixing coverity issues, it's just > that Ranbir has been contributing such patches, which I've found very > welcome, and I wanted to help out with reviews.) > > Laszlo > > -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#111444): https://edk2.groups.io/g/devel/message/111444 Mute This Topic: https://groups.io/mt/102438320/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- --000000000000b5e5f9060a8d7cc8 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Wed, Nov 15, 2023 at 3:20=E2=80=AF= PM Laszlo Ersek <lersek@redhat.com<= /a>> wrote:
O= n 11/14/23 17:21, Kinney, Michael D wrote:
> Hi Ranbir,
>
> =C2=A0
>
> First I want to recognize your efforts to collect Coverity issues and<= br> > propose changes to address
> them.
>
> I still disagree with adding CpuDealLoop() for any static analysis iss= ues.
>
> There have been previous discussions about adding a PANIC() or FATAL()=
> macro that would
> perform platform specific actions if a condition is detected where the=
> boot of the platform
> can not continue.=C2=A0 A platform get to make the choice to log or re= boot or
> hang, etc.=C2=A0 Not the
> code that detected the condition.
>
> https://edk2.groups.io/g/devel/message/86597 > <https://edk2.groups.io/g/devel/message/86597>

This is indeed a great idea.

I didn't know about that discussion. Perhaps a new DebugLib API would handle this (i.e., "panic").

I've been certainly proposing CpuDeadLoop() as a means to panic.

Did the thread conclude anything tangible?

> Unfortunately, in order to fix some of these static analysis issues > correctly, we are going
> to have to identify the use of ASSERT() that really is a fatal conditi= on
> that can not continue

Absolutely.

> and introduce an implementation approach that provides a platform
> handler and
> satisfies the static analysis tools.

The "platform handler" is the difficult part. If the above-noted<= br> discussion from 2022 didn't produce an agreement, should we really bloc= k
the static analyzer fixes on an agreed-upon panic API? I'm concerned that would just cause these fixes to get stuck. And I don't consider CpuDeadLoop()s added for this purpose serious technical debt. They are
easy to find and update later, assuming we also add comments.




> We also have to evaluate if a return error status with a DEBUG_ERROR > message would be a better
> choice than an ASSERT() that can be filtered out by build options.

I agree 100% that this would be better for the codebase, but the work
needed for this is (IMO) impossible to contain. ASSERT() has been abused for a long time *because* it seemed to allow the programmer to ignore
any related error paths. If we now want to implement those error paths
retroactively (which would be absolutely the right thing to do from a
software engineering perspective), then immense amounts of work are
going to be needed (patch review and regression testing), and I think
it's just not practical to dump all that on someone that wants to
improve the status quo. Replacing an invalid ASSERT() with a panic is
honest about the current situation, is safer regarding RELEASE builds,
and its work demand (regression testing, patch review) is tolerable.

I do agree that, if the error path mostly exists already, then returning errors for data/environment-based error conditions (i.e., not for
algorithmic invariant failures) is best.

Where we need to be extremely vigilant is new patches. We must
uncompromisingly reject *new* abuses of ASSERT(), in new patches.

Anyway, it seems that we've been trying to steer Ranbir in opposite
directions. I'll let you take the lead on this; for one, I've not b= een
aware of the panic api discussion for 2022!

(I don't feel especially pushy about fixing coverity issues, it's j= ust
that Ranbir has been contributing such patches, which I've found very welcome, and I wanted to help out with reviews.)

Laszlo

_._,_._,_
Groups.io Links:

=20 You receive all messages sent to this group. =20 =20

View/Reply Online (#111444) | =20 | Mute= This Topic | New Topic
Your Subscriptio= n | Contact Group Owner | Unsubscribe [rebecca@openfw.io]

_._,_._,_
--000000000000b5e5f9060a8d7cc8--