From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:400c:c0c::241; helo=mail-wr0-x241.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wr0-x241.google.com (mail-wr0-x241.google.com [IPv6:2a00:1450:400c:c0c::241]) (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 0ECF522183C9C for ; Fri, 1 Dec 2017 09:53:37 -0800 (PST) Received: by mail-wr0-x241.google.com with SMTP id x49so10954005wrb.13 for ; Fri, 01 Dec 2017 09:58:04 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=RmSSvdCdsH063iGKZf/h838pCgezBO2N7+kcBJr7f0A=; b=O/D47pu6sGhrb0iljE7POm3CXC4m1O6xhZj+5he/66XLGzd1F0E81CTVA+FTHP4c5I TxDAZGRaBmWF2uuwAVrXxIR5icTyptPTMNwZdyJMi2Upy0snlnGIIffOASssXWhOGQac /kBJMM+liJfHd5kh9EMvphLbW87kpebyDXjeY= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=RmSSvdCdsH063iGKZf/h838pCgezBO2N7+kcBJr7f0A=; b=XIQ07SgjlIpN5Hj2ySnATUUU5vj2Y4T/JmZQOUneZkwsqlCpBZfniJb8vxRqWR6lgp Z1LGfZi0SGhVHplzvbRckaiRcqr/tP4rDtJQ/HEGMUyrxJmbnsiBIoeKDQhSC8yCx4/T qC/zD12+Gbu/lhREUp9PX0gvX0gSuV+PoK1e8oWZUNP2r5j6qh5Z8h26BI3EijhjXn/s S15ArlBe87Xk+ssMQ8pX3aoLaCgdlC7ULBBRn6LZ7s3xnK4oHo+UGbheeHJzz8mynXTS zwu2sp2b6CvQ6cszrkJRYoPQl/VMi7AYNVADO5d02SvgUmHtzs/f73vee0m6Yeu+uVEY r98A== X-Gm-Message-State: AJaThX6tVvwx1phXYe6lGH2L41ea7wodCd+AC86bcKcFkdUGlP1lNSjk JFtEzH333G1sosIHlFfi6OXDDQ== X-Google-Smtp-Source: AGs4zMb+5cgPP9PluTvDC9ffYr4LA3NOZYDWSHkxbLpoNt0cr5qde4x+O1sqZTmtpPD7GUy6x91R+A== X-Received: by 10.223.151.212 with SMTP id t20mr6409926wrb.2.1512151083181; Fri, 01 Dec 2017 09:58:03 -0800 (PST) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id c54sm9796642wra.84.2017.12.01.09.58.01 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Fri, 01 Dec 2017 09:58:02 -0800 (PST) Date: Fri, 1 Dec 2017 17:58:00 +0000 From: Leif Lindholm To: Ard Biesheuvel Cc: evan.lloyd@arm.com, matteo.carlini@arm.com, edk2-devel@lists.01.org Message-ID: <20171201175800.3qsgayenbervrypv@bivouac.eciton.net> References: <20170926201529.11644-1-evan.lloyd@arm.com> <20170926201529.11644-5-evan.lloyd@arm.com> MIME-Version: 1.0 In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [PATCH 04/19] ArmPlatformPkg: LcdGraphicsOurputDxe: Add debug asserts X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 01 Dec 2017 17:53:38 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Fri, Dec 01, 2017 at 05:34:52PM +0000, Ard Biesheuvel wrote: > On 1 December 2017 at 16:33, Evan Lloyd wrote: > > Responses inline: > > > >> -----Original Message----- > >> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] > >> Sent: 13 October 2017 08:33 > >> To: Evan Lloyd > >> Cc: edk2-devel@lists.01.org; "ard.biesheuvel@linaro.org"@arm.com; > >> "leif.lindholm@linaro.org"@arm.com; > >> "Matteo.Carlini@arm.com"@arm.com; "nd@arm.com"@arm.com > >> Subject: Re: [PATCH 04/19] ArmPlatformPkg: LcdGraphicsOurputDxe: Add > >> debug asserts > >> > >> On 26 September 2017 at 21:15, wrote: > >> > From: Girish Pathak > >> > > >> > This change adds some debug assertions e.g to catch NULL pointer > >> > errors missing in PL11Lcd and HdLcd modules. > >> > > >> > This change also improves related error handling code. > >> > > >> > Contributed-under: TianoCore Contribution Agreement 1.1 > >> > Signed-off-by: Girish Pathak > >> > Signed-off-by: Evan Lloyd > >> > --- > >> > > >> ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdAr > >> mVExpress.c | 44 ++++++++++++++++++-- > >> > > >> ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL11 > >> 1LcdArmVExpress.c | 43 ++++++++++++++++++- > >> > ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c > >> | 8 ++-- > >> > ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c > >> | 8 ++-- > >> > 4 files changed, 90 insertions(+), 13 deletions(-) > >> > > >> > diff --git > >> > > >> a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd > >> ArmVE > >> > xpress.c > >> > > >> b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd > >> ArmVE > >> > xpress.c index > >> > > >> b9859a56988f7e5be0adbaa49048a683fe586bfe..58dd9f0c77e1bc9af559a > >> 71d0c7c > >> > ce72d71c6da5 100644 > >> > --- > >> > > >> a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd > >> ArmVE > >> > xpress.c > >> > +++ > >> b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd > >> A > >> > +++ rmVExpress.c > >> > @@ -140,6 +140,7 @@ LcdPlatformInitializeDisplay ( > >> > * buffer in bytes > >> > * > >> > * @retval EFI_SUCCESS Frame buffer memory allocation success. > >> > + * @retval EFI_INVALID_PARAMETER VramBaseAddress or VramSize > >> are NULL. > >> > * @retval !(EFI_SUCCESS) Other errors. > >> > **/ > >> > EFI_STATUS > >> > @@ -151,6 +152,13 @@ LcdPlatformGetVram ( > >> > EFI_STATUS Status; > >> > EFI_ALLOCATE_TYPE AllocationType; > >> > > >> > + // Check VramBaseAddress and VramSize are not NULL. > >> > + if (VramBaseAddress == NULL || VramSize == NULL) { > >> > + ASSERT (VramBaseAddress != NULL); > >> > + ASSERT (VramSize != NULL); > >> > + return EFI_INVALID_PARAMETER; > >> > + } > >> > + > >> > // Set the vram size > >> > *VramSize = LCD_VRAM_SIZE; > >> > > >> > @@ -169,6 +177,7 @@ LcdPlatformGetVram ( > >> > VramBaseAddress > >> > ); > >> > if (EFI_ERROR (Status)) { > >> > + ASSERT_EFI_ERROR (Status); > >> > return Status; > >> > } > >> > > >> > @@ -179,8 +188,8 @@ LcdPlatformGetVram ( > >> > *VramSize, > >> > EFI_MEMORY_WC > >> > ); > >> > - ASSERT_EFI_ERROR (Status); > >> > if (EFI_ERROR (Status)) { > >> > + ASSERT_EFI_ERROR (Status); > >> > >> What is the point of this change? > > [[Evan Lloyd]] It is a minor efficiency improvement. Since the > > ASSERT can only fire when the if condition is true, it removes a > > duplicated test from the main (non-error) code flow. This is > > irrelevant on hardware, but actually significant when running > > debug builds on an emulator environment. > > Fair enough. But I'd prefer to finally fix the DEBUG vs NOOPT build > targets for ARM/AARCH64. DEBUG was never intended to be -O0 (and it is > not on X86 either). > > Code is complex enough as it is, and how to move trivial tests like > this around for 'performance' reasons is a dimension I would like to > avoid. Echoing from an irc discussion with Ard: I think NOOPT (and a !-O0 DEBUG) would be good to have for the ARM architectures as well, both for GCC and CLANG. Evan, have you done any investigation into the impact of optimization on emulation performance? My completely unfounded guess would be -O1 should be a good fit for that environment, and hence for DEBUG. It would remove some of the more insane bloat of -O0 without starting heavy inlining. But as always, real numbers beat guesses. (If -O2 is better than -O1, is -Os better than either?) / Leif