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:c06::244; helo=mail-io0-x244.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-io0-x244.google.com (mail-io0-x244.google.com [IPv6:2607:f8b0:4001:c06::244]) (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 B2F032235228E for ; Tue, 6 Mar 2018 03:10:17 -0800 (PST) Received: by mail-io0-x244.google.com with SMTP id 30so21651035iog.2 for ; Tue, 06 Mar 2018 03:16:31 -0800 (PST) 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=xBZJeOoCPGgUmnLwhEUJ3csxVOr91ZxW+K6ch3H0i5A=; b=TpO3uoimteKNJMOywgyUhFMKFRtQD69TlSIkITtrXkZo7Vrj9WWXNXK1qwuq5C+glp rbOjOaeEaJcu+X6c4bBDDv8HJuLUgRkbeAq0D6nhEHkghVC0BJL2yP2+qgmwRhg45K3d Hq2iW7jJwfcrLWPiFpWWOo+BC0Y5RX0uA6EOg= 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=xBZJeOoCPGgUmnLwhEUJ3csxVOr91ZxW+K6ch3H0i5A=; b=CdLdg83YLo01dmyoAo13hjOpu7tuL/vKDUl840YbPJDsDiNCvKENzpGX8XsXazeWEa HSqjVbEpQtYTXn1hJv5a0bAu43o0NeRhLSnmDmAC5A9ztzjVLKMaedDZVgbnrXTvcrXn zrWvkyQyb8uHk26weGFgozkMMpJRd7GZ31Hc1pwBqkPB0JVVFKTvOY2CJq8+Bw6pREiG nWZZ+izHxNQwjsx4t5ijuCnnV+qSVrViXKEKflXMFTXbFW3Zh9aiQIKoNcUodf3QJWBe A03nvPFzf13cfUAUxUTRH30YTo++NiMbsmNxHP9Q+l55f4RwXyhZHSGsHnEdn8WIs5dO NcFw== X-Gm-Message-State: APf1xPBU5ZvWmU4QGCDdak4Q+Jh6dbcyJX+HYHmoZou/Ibsmaey5m98h 9rmz8L/BUcqcSVVGzJm+d6g7s24ni1gHp9c8HrbgBg== X-Google-Smtp-Source: AG47ELum4JiPekeWScZq8iT9sTuQdjDmIKXE8AeTNjch+x5Bmtl+aMdm6NA5bUljiGTV6baFMOifEIwGwpMMdGvDKEA= X-Received: by 10.107.5.199 with SMTP id 190mr22022645iof.107.1520334990629; Tue, 06 Mar 2018 03:16:30 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.138.209 with HTTP; Tue, 6 Mar 2018 03:16:30 -0800 (PST) In-Reply-To: References: <20171222190821.12440-1-evan.lloyd@arm.com> <20171222190821.12440-7-evan.lloyd@arm.com> From: Ard Biesheuvel Date: Tue, 6 Mar 2018 11:16:30 +0000 Message-ID: To: Evan Lloyd Cc: "leif.lindholm@linaro.org" , Girish Pathak , Matteo Carlini , nd , "edk2-devel@lists.01.org" 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: Tue, 06 Mar 2018 11:10:18 -0000 Content-Type: text/plain; charset="UTF-8" On 5 March 2018 at 15:08, Evan Lloyd wrote: > In that case, would you be happy to take Girish's patches with the ASSERTs done your (our) way? > Leif can fulminate about it when he gets back, if he really feels that strongly. Why? Because it is almost the end of the quarter? Can't it wait? > I suspect, though, that Leif is capable of being reasonable if pressed (and offered beer at Plugfest). > I am not going to push something I know Leif disapproves of in its current form in his absence, sorry. > >> -----Original Message----- >> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] >> Sent: 02 March 2018 19:07 >> To: Evan Lloyd >> Cc: leif.lindholm@linaro.org; Girish Pathak ; >> Matteo Carlini ; nd ; edk2- >> devel@lists.01.org >> Subject: Re: [edk2] [PATCH edk2-platforms v2 06/18] ARM/VExpressPkg: >> Add and update debug ASSERTS >> >> On 28 February 2018 at 20:27, Evan Lloyd wrote: >> > Hi Leif, Ard. >> > Can I get you two argue out the pros and cons of the "ASSERT(FALSE)" >> debate, please. >> >> I can argue the cons if you like. For the pros, you'll have to wait for Leif to >> return from holiday (in a week or two AFAIK) >> >> > (see >> > https://lists.01.org/pipermail/edk2-devel/2018-January/019788.html) >> > For what it is worth, our (surprisingly unanimous) opinion is that, since >> the ASSERT is only there to help spot a problem, then the more information >> reported the better. The only benefits of ASSERT(FALSE) would be a smaller >> debug image and minor efficiency improvement on the path to the crash. >> > >> > Regards, >> > Evan >> > >> >> -----Original Message----- >> >> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] >> >> Sent: 04 January 2018 19:55 >> >> To: Evan Lloyd >> >> Cc: Girish Pathak ; Matteo Carlini >> >> ; nd ; edk2- >> devel@lists.01.org; >> >> Thomas Abraham ; Arvind Chauhan >> >> ; leif.lindholm@linaro.org >> >> Subject: Re: [edk2] [PATCH edk2-platforms v2 06/18] ARM/VExpressPkg: >> >> Add and update debug ASSERTS >> >> >> >> On 4 January 2018 at 19:51, Evan Lloyd wrote: >> >> > >> >> > >> >> >> -----Original Message----- >> >> >> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] >> >> >> Sent: 04 January 2018 19:24 >> >> >> To: Girish Pathak >> >> >> Cc: Evan Lloyd ; Matteo Carlini >> >> >> ; nd ; edk2- >> >> devel@lists.01.org; >> >> >> Thomas Abraham ; Arvind Chauhan >> >> >> ; leif.lindholm@linaro.org >> >> >> Subject: Re: [edk2] [PATCH edk2-platforms v2 06/18] >> ARM/VExpressPkg: >> >> >> Add and update debug ASSERTS >> >> >> >> >> >> 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/HdLcdArmVExp >> >> >> r >> >> >> >> ess.c | 22 +++++++++++++++++- >> >> >> >> > >> >> >> >> > >> >> >> >> >> >> >> >> >> >> Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdAr >> >> >> m >> >> >> >> VEx >> >> >> >> > press.c | 24 +++++++++++++++++++- >> >> >> >> > 2 files changed, 44 insertions(+), 2 deletions(-) >> >> >> >> > >> >> >> >> > diff --git >> >> >> >> > >> >> >> >> >> >> >> >> >> >> a/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVE >> >> >> x >> >> >> >> pres >> >> >> >> > s.c >> >> >> >> > >> >> >> >> >> >> >> >> >> >> b/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVE >> >> >> x >> >> >> >> pres >> >> >> >> > s.c index >> >> >> >> > >> >> >> >> >> >> >> >> >> >> 6afd764897f49c64490ce891682f99bb0f5d993b..a8fe8696da0653017ce9fa >> >> >> 6e4a >> >> >> >> 86 >> >> >> >> > caf283bc04c9 100644 >> >> >> >> > --- >> >> >> >> > >> >> >> >> >> >> >> >> >> >> a/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVE >> >> >> x >> >> >> >> pres >> >> >> >> > s.c >> >> >> >> > +++ >> >> >> >> >> >> >> >> >> >> b/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVE >> >> >> x >> >> >> >> > +++ 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.ht >> >> >> > ml >> >> >> > >> >> >> > 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. >> >> >> >> >> >> 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. >> >> > >> >> > [[Evan Lloyd]] An alternative might be for Girish to take the >> >> > other route >> >> Leif suggested, and cache the condition in a variable. >> >> > That might be a slight overhead, and the (presumably BOOLEAN) >> >> > variable >> >> may need careful naming, but... >> >> > >> >> >> >> If we are going to use a boolean to record the result of the >> >> comparison, and >> >> ASSERT() on it in the if () block if the comparison is false, I don't >> >> see what the difference is with doing ASSERT (FALSE) directly. >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> > gBS->FreePages (*VramBaseAddress, EFI_SIZE_TO_PAGES >> >> >> (*VramSize)); >> >> >> >> > return Status; >> >> >> >> > } >> >> >> >> > @@ -221,6 +225,7 @@ LcdPlatformSetMode ( >> >> >> >> > EFI_STATUS Status; >> >> >> >> > >> >> >> >> > if (ModeNumber >= LcdPlatformGetMaxMode ()) { >> >> >> >> > + ASSERT (FALSE); >> >> >> >> >> >> >> >> These are fine: the code itself explains adequately which >> >> >> >> condition triggered the ASSERT to fire. >> >> >> >> >> >> >> >> > return EFI_INVALID_PARAMETER; >> >> >> >> > } >> >> >> >> > >> >> >> >> > @@ -279,7 +284,10 @@ LcdPlatformQueryMode ( >> >> >> >> > OUT EFI_GRAPHICS_OUTPUT_MODE_INFORMATION * CONST >> >> Info >> >> >> >> > ) >> >> >> >> > { >> >> >> >> > + ASSERT (Info != NULL); >> >> >> >> > + >> >> >> >> > if (ModeNumber >= LcdPlatformGetMaxMode ()) { >> >> >> >> > + ASSERT (FALSE); >> >> >> >> > return EFI_INVALID_PARAMETER; >> >> >> >> > } >> >> >> >> > >> >> >> >> > @@ -343,7 +351,18 @@ LcdPlatformGetTimings ( >> >> >> >> > OUT UINT32 * CONST VFrontPorch >> >> >> >> > ) >> >> >> >> > { >> >> >> >> > + // One of the pointers is NULL ASSERT (HRes != NULL); >> >> >> >> > + ASSERT (HSync != NULL); ASSERT (HBackPorch != NULL); >> >> >> >> > + ASSERT (HFrontPorch != NULL); ASSERT (VRes != NULL); >> >> >> >> > + ASSERT (VSync != NULL); ASSERT (VBackPorch != NULL); >> >> >> >> > + ASSERT (VFrontPorch != NULL); >> >> >> >> > + >> >> >> >> > if (ModeNumber >= LcdPlatformGetMaxMode ()) { >> >> >> >> > + ASSERT (FALSE); >> >> >> >> > return EFI_INVALID_PARAMETER; >> >> >> >> > } >> >> >> >> > >> >> >> >> > @@ -376,6 +395,7 @@ LcdPlatformGetBpp ( >> >> >> >> > ) >> >> >> >> > { >> >> >> >> > if (ModeNumber >= LcdPlatformGetMaxMode ()) { >> >> >> >> > + ASSERT (FALSE); >> >> >> >> > return EFI_INVALID_PARAMETER; >> >> >> >> > } >> >> >> >> > >> >> >> >> > diff --git >> >> >> >> > >> >> >> >> >> >> >> >> >> >> a/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111Lcd >> >> >> Ar >> >> >> >> mV >> >> >> >> > Express.c >> >> >> >> > >> >> >> >> >> >> >> >> >> >> b/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111Lcd >> >> >> Ar >> >> >> >> mV >> >> >> >> > Express.c index >> >> >> >> > >> >> >> >> >> >> >> >> >> >> 799fb3fc781ce04bb64cb1fa0b87f262a670ed78..fd4eea8f8e2397bc7d4ddf >> >> >> 4cfe >> >> >> >> 3d >> >> >> >> > cc97a5109edb 100644 >> >> >> >> > --- >> >> >> >> > >> >> >> >> >> >> >> >> >> >> a/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111Lcd >> >> >> Ar >> >> >> >> mV >> >> >> >> > Express.c >> >> >> >> > +++ >> >> >> >> >> >> >> >> >> >> b/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111Lcd >> >> >> >> > +++ ArmVExpress.c >> >> >> >> > @@ -205,6 +205,9 @@ LcdPlatformGetVram ( >> >> >> >> > >> >> >> >> > Status = EFI_SUCCESS; >> >> >> >> > >> >> >> >> > + ASSERT (VramBaseAddress != NULL); ASSERT (VramSize != >> >> >> >> > + NULL); >> >> >> >> > + >> >> >> >> > // Is it on the motherboard or on the daughterboard? >> >> >> >> > switch (PL111_CLCD_SITE) { >> >> >> >> > >> >> >> >> > @@ -225,6 +228,7 @@ LcdPlatformGetVram ( >> >> >> >> > VramBaseAddress >> >> >> >> > ); >> >> >> >> > if (EFI_ERROR (Status)) { >> >> >> >> > + ASSERT (FALSE); >> >> >> >> > return Status; >> >> >> >> > } >> >> >> >> > >> >> >> >> > @@ -235,8 +239,8 @@ LcdPlatformGetVram ( >> >> >> >> > *VramSize, >> >> >> >> > EFI_MEMORY_WC >> >> >> >> > ); >> >> >> >> > - ASSERT_EFI_ERROR (Status); >> >> >> >> > if (EFI_ERROR (Status)) { >> >> >> >> > + ASSERT (FALSE); >> >> >> >> > gBS->FreePages (*VramBaseAddress, EFI_SIZE_TO_PAGES >> >> >> >> (*VramSize)); >> >> >> >> > return Status; >> >> >> >> > } >> >> >> >> > @@ -294,6 +298,7 @@ LcdPlatformSetMode ( >> >> >> >> > UINT32 SysId; >> >> >> >> > >> >> >> >> > if (ModeNumber >= LcdPlatformGetMaxMode ()) { >> >> >> >> > + ASSERT (FALSE); >> >> >> >> > return EFI_INVALID_PARAMETER; >> >> >> >> > } >> >> >> >> > >> >> >> >> > @@ -369,7 +374,10 @@ LcdPlatformQueryMode ( >> >> >> >> > OUT EFI_GRAPHICS_OUTPUT_MODE_INFORMATION * CONST >> Info >> >> >> >> > ) >> >> >> >> > { >> >> >> >> > + ASSERT (Info != NULL); >> >> >> >> > + >> >> >> >> > if (ModeNumber >= LcdPlatformGetMaxMode ()) { >> >> >> >> > + ASSERT (FALSE); >> >> >> >> > return EFI_INVALID_PARAMETER; >> >> >> >> > } >> >> >> >> > >> >> >> >> > @@ -433,7 +441,18 @@ LcdPlatformGetTimings ( >> >> >> >> > OUT UINT32 * CONST VFrontPorch >> >> >> >> > ) >> >> >> >> > { >> >> >> >> > + // One of the pointers is NULL ASSERT (HRes != NULL); >> >> >> >> > + ASSERT (HSync != NULL); ASSERT (HBackPorch != NULL); >> >> >> >> > + ASSERT (HFrontPorch != NULL); ASSERT (VRes != NULL); >> >> >> >> > + ASSERT (VSync != NULL); ASSERT (VBackPorch != NULL); >> >> >> >> > + ASSERT (VFrontPorch != NULL); >> >> >> >> > + >> >> >> >> > if (ModeNumber >= LcdPlatformGetMaxMode ()) { >> >> >> >> > + ASSERT (FALSE); >> >> >> >> > return EFI_INVALID_PARAMETER; >> >> >> >> > } >> >> >> >> > >> >> >> >> > @@ -465,7 +484,10 @@ LcdPlatformGetBpp ( >> >> >> >> > OUT LCD_BPP * CONST Bpp >> >> >> >> > ) >> >> >> >> > { >> >> >> >> > + ASSERT (Bpp != NULL); >> >> >> >> > + >> >> >> >> > if (ModeNumber >= LcdPlatformGetMaxMode ()) { >> >> >> >> > + ASSERT (FALSE); >> >> >> >> > return EFI_INVALID_PARAMETER; >> >> >> >> > } >> >> >> >> > >> >> >> >> > -- >> >> >> >> > Guid("CE165669-3EF3-493F-B85D-6190EE5B9759") >> >> >> >> > >> >> >> >> > _______________________________________________ >> >> >> >> > edk2-devel mailing list >> >> >> >> > edk2-devel@lists.01.org >> >> >> >> > https://lists.01.org/mailman/listinfo/edk2-devel >> >> >> >> _______________________________________________ >> >> >> >> edk2-devel mailing list >> >> >> >> edk2-devel@lists.01.org >> >> >> >> https://lists.01.org/mailman/listinfo/edk2-devel