From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c0b::243; helo=mail-it0-x243.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x243.google.com (mail-it0-x243.google.com [IPv6:2607:f8b0:4001:c0b::243]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 9FD2B20954CC1 for ; Wed, 14 Mar 2018 05:28:42 -0700 (PDT) Received: by mail-it0-x243.google.com with SMTP id n136-v6so4360206itg.5 for ; Wed, 14 Mar 2018 05:35:05 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=EMQQljOileBq0+I2fM5vLwkwrPjUxoaO1BRJ3wp6kMY=; b=Vdn4T62xFFOkRaK5SZHTgAdU3pIyX3xC+m9Bgsr612KbHITUJfPJ8FWhRHwDSJSDjC fMLDDvAnNXYrmbi53ZtrJQrxYTL2q+mVxZLBTXlsDfiVQs61hCsv0dSnaXII8ijzS4A/ CpzYKqyEVwx3x0RmDWn4o5vrRDXgBZ2HMZ1Bk= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=EMQQljOileBq0+I2fM5vLwkwrPjUxoaO1BRJ3wp6kMY=; b=nc6Psex9ZJ/wMgt/AojNzZ6Ip0gi18XSbS2G7cj5D90bp50QdhxULnavX0Lsf3oJCb 4LOUOL+XupRNHMjKeCRUG5o2tOLh0aFEwlEyKUKN94t3xtZLoDMJfZVI909+z4fhJuAC hSMQNt/GqUlGTQyhmr1n8oslEQAJOIhTZMaXx559+V5qlip8rmEn42n4kdrLnW686DeB baLB6hoHBXXWIem8V5ETejbKHy9Pjy/xIWf7be05taf8L3Go99iMZUGV+DyfMlhHPzxM 9iyHUZPYnA7+cOlh+1mPTCvN+ybSodNrp2DDD1aKwzR4iJQRovWZruQZYCBSQRSVruGz +95A== X-Gm-Message-State: AElRT7GAs2yCAUJTXqnZ5aBfPr82K/bXmFZ/+o4HaCm/+7aS+QzV7YYF 0CRPSujJLCGkkVe4tih8bIULwO2s536zXHaopS2pIQ== X-Google-Smtp-Source: AG47ELuf/BGeiNipN77Nd5vAONawIP9lqyicsJdeoQIlvj1dUbq6DS5/JEumNCHy01n0Yh10G2+2MC4k8lIHosgLdaA= X-Received: by 10.36.60.216 with SMTP id m207mr1745748ita.68.1521030904195; Wed, 14 Mar 2018 05:35:04 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.138.209 with HTTP; Wed, 14 Mar 2018 05:35:03 -0700 (PDT) In-Reply-To: <20180314122415.ohlblcrhzfvn22bl@bivouac.eciton.net> References: <20171222190821.12440-1-evan.lloyd@arm.com> <20171222190821.12440-7-evan.lloyd@arm.com> <20180314122415.ohlblcrhzfvn22bl@bivouac.eciton.net> From: Ard Biesheuvel Date: Wed, 14 Mar 2018 12:35:03 +0000 Message-ID: To: Leif Lindholm Cc: Girish Pathak , Evan Lloyd , Matteo Carlini , nd , "edk2-devel@lists.01.org" , Thomas Abraham , Arvind Chauhan Subject: Re: [PATCH edk2-platforms v2 06/18] ARM/VExpressPkg: Add and update debug ASSERTS X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 14 Mar 2018 12:28:43 -0000 Content-Type: text/plain; charset="UTF-8" On 14 March 2018 at 12:24, Leif Lindholm wrote: > On Thu, Jan 04, 2018 at 07:24:20PM +0000, Ard Biesheuvel wrote: >> On 4 January 2018 at 18:55, Girish Pathak 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 >> >> 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 ; Arvind >> >> Chauhan ; >> >> "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, wrote: >> >> > From: Girish Pathak >> >> > >> >> > 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 >> >> > Signed-off-by: Evan Lloyd >> >> > --- >> >> > >> >> 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