public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
To: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Girish Pathak <Girish.Pathak@arm.com>,
	Evan Lloyd <Evan.Lloyd@arm.com>,
	 Matteo Carlini <Matteo.Carlini@arm.com>, nd <nd@arm.com>,
	 "edk2-devel@lists.01.org" <edk2-devel@lists.01.org>,
	Thomas Abraham <thomas.abraham@arm.com>,
	 Arvind Chauhan <Arvind.Chauhan@arm.com>
Subject: Re: [PATCH edk2-platforms v2 06/18] ARM/VExpressPkg: Add and update debug ASSERTS
Date: Wed, 14 Mar 2018 12:35:03 +0000	[thread overview]
Message-ID: <CAKv+Gu_fQNug8KPTbr8nRC0EjMoTxro0pwV6cC8nAzcZ1a6DrA@mail.gmail.com> (raw)
In-Reply-To: <20180314122415.ohlblcrhzfvn22bl@bivouac.eciton.net>

On 14 March 2018 at 12:24, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Thu, Jan 04, 2018 at 07:24:20PM +0000, Ard Biesheuvel wrote:
>> On 4 January 2018 at 18:55, Girish Pathak <Girish.Pathak@arm.com> wrote:
>> > Hi Ard,
>> >
>> >> -----Original Message-----
>> >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>> >> Ard Biesheuvel
>> >> Sent: 23 December 2017 14:12
>> >> To: Evan Lloyd <Evan.Lloyd@arm.com>
>> >> Cc: "Matteo.Carlini@arm.com"@arm.com;
>> >> "leif.lindholm@linaro.org"@arm.com; "nd@arm.com"@arm.com; edk2-
>> >> devel@lists.01.org; Thomas Abraham <thomas.abraham@arm.com>; Arvind
>> >> Chauhan <Arvind.Chauhan@arm.com>;
>> >> "ard.biesheuvel@linaro.org"@arm.com
>> >> Subject: Re: [edk2] [PATCH edk2-platforms v2 06/18] ARM/VExpressPkg: Add
>> >> and update debug ASSERTS
>> >>
>> >> On 22 December 2017 at 19:08,  <evan.lloyd@arm.com> wrote:
>> >> > From: Girish Pathak <girish.pathak at arm.com>
>> >> >
>> >> > This change adds some debug assertions e.g to catch NULL pointer
>> >> > errors missing in PL11Lcd and HdLcd platform libraries.
>> >> >
>> >> > Contributed-under: TianoCore Contribution Agreement 1.1
>> >> > Signed-off-by: Girish Pathak <girish.pathak@arm.com>
>> >> > Signed-off-by: Evan Lloyd <evan.lloyd@arm.com>
>> >> > ---
>> >> >
>> >> Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpr
>> >> ess.c       | 22 +++++++++++++++++-
>> >> >
>> >> >
>> >> Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArm
>> >> VEx
>> >> > press.c | 24 +++++++++++++++++++-
>> >> >  2 files changed, 44 insertions(+), 2 deletions(-)
>> >> >
>> >> > diff --git
>> >> >
>> >> a/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVEx
>> >> pres
>> >> > s.c
>> >> >
>> >> b/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVEx
>> >> pres
>> >> > s.c index
>> >> >
>> >> 6afd764897f49c64490ce891682f99bb0f5d993b..a8fe8696da0653017ce9fa6e4a
>> >> 86
>> >> > caf283bc04c9 100644
>> >> > ---
>> >> >
>> >> a/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVEx
>> >> pres
>> >> > s.c
>> >> > +++
>> >> b/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVEx
>> >> > +++ press.c
>> >> > @@ -153,6 +153,9 @@ LcdPlatformGetVram (
>> >> >    EFI_STATUS              Status;
>> >> >    EFI_ALLOCATE_TYPE       AllocationType;
>> >> >
>> >> > +  ASSERT (VramBaseAddress != NULL);
>> >> > +  ASSERT (VramSize != NULL);
>> >> > +
>> >> >    // Set the vram size
>> >> >    *VramSize = LCD_VRAM_SIZE;
>> >> >
>> >> > @@ -171,6 +174,7 @@ LcdPlatformGetVram (
>> >> >                    VramBaseAddress
>> >> >                    );
>> >> >    if (EFI_ERROR (Status)) {
>> >> > +    ASSERT (FALSE);
>> >> >      return Status;
>> >> >    }
>> >> >
>> >> > @@ -181,8 +185,8 @@ LcdPlatformGetVram (
>> >> >                    *VramSize,
>> >> >                    EFI_MEMORY_WC
>> >> >                    );
>> >> > -  ASSERT_EFI_ERROR (Status);
>> >> >    if (EFI_ERROR (Status)) {
>> >> > +    ASSERT (FALSE);
>> >>
>> >> As in the sibling patch against EDK2, this patch makes it more difficult to
>> >> figure out what went wrong when you hit the ASSERT.
>> >> ASSERT_EFI_ERROR prints the value of Status, ASSERT(FALSE) only prints
>> >> '0 != 1'
>> >>
>> >
>> > This change(and other similar changes) is in response to review comments on patch v1
>> > https://lists.01.org/pipermail/edk2-devel/2017-October/015995.html
>> >
>> > with above reference, Can you please confirm if we should revert to the patch v1 version ?
>> >
>>
>> I guess Leif and I are in disagreement here. In particular, I think
>> his comment
>>
>> """
>> ASSERT (FALSE)?  (You already know Status is an EFI_ERROR, and a
>> console message saying ASSERT (Status) is not getting you out of
>> looking at the source code to find out what happened.)
>> """
>>
>> is misguided, given that ASSERT_EFI_ERROR (Status) will actually print
>> the value of Status to the debug console.
>
> I don't have a strong enough opinion on this that it should be
> listened to if you both agree.
>
> It's just the "if error, then assert if error" that breaks up the
> parsing flow for me. Could it make sense to use ASSERT_RETURN_ERROR
> instead?
>
> So
>       if (EFI_ERROR (Status)) {
>         ASSERT_RETURN_ERROR (Status);
>       }
>

Do you mean using ASSERT_RETURN_ERROR() rather than ASSERT_EFI_ERROR()?

That doesn't make sense: the former is for RETURN_STATUS type
variables and the latter for EFI_STATUS

>> However, the objections against putting function calls in ASSERT()s
>> are justified: ASSERT() should not have side effects if its condition
>> is met, and function calls may have side effects.
>>
>> I suppose we should wait for Leif to return on the 22nd before
>> proceeding with the review.
>> Apologies for the confusion, and for the delay.
>
> Apologies for the even more ridicilous delay.
> Err, I need to stop taking holidays or something.
>
> Don't worry, I won't have another one for a couple of weeks :]
> First Linaro Connect, then Plugfest.
>
> /
>     Leif


  reply	other threads:[~2018-03-14 12:28 UTC|newest]

Thread overview: 57+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-12-22 19:08 [PATCH edk2-platforms v2 00/18] ARM: Update GOP evan.lloyd
2017-12-22 19:08 ` [PATCH edk2-platforms v2 01/18] ARM/VExpressPkg: Fix MODULE_TYPE of HDLCD/PL111 platform libraries evan.lloyd
2017-12-22 19:08 ` [PATCH edk2-platforms v2 02/18] ARM/VExpressPkg: Tidy HDLCD and PL11LCD platform Lib: Coding standard evan.lloyd
2017-12-23 14:07   ` Ard Biesheuvel
2017-12-22 19:08 ` [PATCH edk2-platforms v2 03/18] ARM/VExpressPkg: Tidy HdLcd/PL111Lcd code: Updated comments evan.lloyd
2017-12-23 14:08   ` Ard Biesheuvel
2017-12-22 19:08 ` [PATCH edk2-platforms v2 04/18] ARM/VExpressPkg: Remove unused PcdPL111LcdMaxMode from HDLCD inf evan.lloyd
2017-12-23 14:08   ` Ard Biesheuvel
2017-12-22 19:08 ` [PATCH edk2-platforms v2 05/18] ARM/VExpressPkg: PL111 and HDLCD: add const qualifier evan.lloyd
2017-12-23 14:09   ` Ard Biesheuvel
2017-12-22 19:08 ` [PATCH edk2-platforms v2 06/18] ARM/VExpressPkg: Add and update debug ASSERTS evan.lloyd
2017-12-23 14:12   ` Ard Biesheuvel
2018-01-04 18:55     ` Girish Pathak
2018-01-04 19:24       ` Ard Biesheuvel
2018-01-04 19:51         ` Evan Lloyd
2018-01-04 19:54           ` Ard Biesheuvel
2018-02-28 20:27             ` Evan Lloyd
2018-03-02 19:07               ` Ard Biesheuvel
2018-03-05 15:08                 ` Evan Lloyd
2018-03-06 11:16                   ` Ard Biesheuvel
2018-03-14 12:24         ` Leif Lindholm
2018-03-14 12:35           ` Ard Biesheuvel [this message]
2018-03-14 12:39             ` Leif Lindholm
2017-12-22 19:08 ` [PATCH edk2-platforms v2 07/18] ARM/VExpressPkg: PL111LcdArmVExpressLib: Minor code cleanup evan.lloyd
2017-12-23 14:13   ` Ard Biesheuvel
2017-12-22 19:08 ` [PATCH edk2-platforms v2 08/18] ARM/VExpressPkg: PL111 and HDLCD: Use FixedPcdGet32 evan.lloyd
2017-12-23 14:14   ` Ard Biesheuvel
2017-12-22 19:08 ` [PATCH edk2-platforms v2 09/18] ARM/VExpressPkg: PL11LcdArmVExpressLib: Improvement conditional evan.lloyd
2017-12-23 14:16   ` Ard Biesheuvel
2017-12-22 19:08 ` [PATCH edk2-platforms v2 10/18] ARM/VExpressPkg: HdLcdArmVExpressLib: Remove status check EFI_TIMEOUT evan.lloyd
2017-12-23 14:16   ` Ard Biesheuvel
2017-12-22 19:08 ` [PATCH edk2-platforms v2 11/18] ARM/VExpressPkg: HdLcdArmVExpressLib: Remove redundant Bpp evan.lloyd
2017-12-23 14:17   ` Ard Biesheuvel
2017-12-22 19:08 ` [PATCH edk2-platforms v2 12/18] ARM/VExpressPkg: Redefine LcdPlatformGetTimings function evan.lloyd
2017-12-23 14:18   ` Ard Biesheuvel
2017-12-22 19:08 ` [PATCH edk2-platforms v2 13/18] ARM/VExpressPkg: PL111 and HDLCD: Add PCD to select pixel format evan.lloyd
2017-12-23 16:00   ` Ard Biesheuvel
2017-12-22 19:08 ` [PATCH edk2-platforms v2 14/18] ARM/VExpressPkg: Reserving framebuffer at build evan.lloyd
2017-12-23 16:02   ` Ard Biesheuvel
2018-01-03 11:04     ` Evan Lloyd
2017-12-22 19:08 ` [PATCH edk2-platforms v2 15/18] ARM/VExpressPkg: New DP500/DP550/DP650 platform library evan.lloyd
2017-12-23 16:07   ` Ard Biesheuvel
2018-01-08 18:51     ` Evan Lloyd
2018-01-24 11:27       ` Alexei Fedorov
2018-01-24 11:34         ` Ard Biesheuvel
2017-12-22 19:08 ` [PATCH edk2-platforms v2 16/18] ARM/JunoPkg: Mapping Non-Trused SRAM as device memory evan.lloyd
2017-12-23 16:08   ` Ard Biesheuvel
2017-12-22 19:08 ` [PATCH edk2-platforms v2 17/18] ARM/JunoPkg: Adding SCMI MTL library evan.lloyd
2017-12-23 16:12   ` Ard Biesheuvel
2017-12-22 19:08 ` [PATCH edk2-platforms v2 18/18] ARM/JunoPkg: Add HDLCD platform library evan.lloyd
2017-12-23 16:22   ` Ard Biesheuvel
2018-01-09 18:21     ` Evan Lloyd
2018-01-09 18:26       ` Ard Biesheuvel
2018-01-10 11:45         ` Alexei Fedorov
2018-01-10 12:02           ` Ard Biesheuvel
2017-12-22 19:29 ` [PATCH edk2-platforms v2 00/18] ARM: Update GOP Ard Biesheuvel
2018-01-02 10:28   ` Evan Lloyd

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-list 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=CAKv+Gu_fQNug8KPTbr8nRC0EjMoTxro0pwV6cC8nAzcZ1a6DrA@mail.gmail.com \
    --to=devel@edk2.groups.io \
    /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