From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web12.2604.1591860858460234812 for ; Thu, 11 Jun 2020 00:34:18 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: ard.biesheuvel@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 06C0E1FB; Thu, 11 Jun 2020 00:34:17 -0700 (PDT) Received: from [192.168.1.69] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 72C943F73D; Thu, 11 Jun 2020 00:34:16 -0700 (PDT) Subject: Re: [edk2-devel] [PATCH v2] MdePkg/BasePrintLib: avoid absolute addresses for error strings To: "Kinney, Michael D" , "devel@edk2.groups.io" Cc: "Gao, Liming" References: <20200610193023.171092-1-ard.biesheuvel@arm.com> From: "Ard Biesheuvel" Message-ID: <20238067-91c0-6deb-6168-03da0926b8f1@arm.com> Date: Thu, 11 Jun 2020 09:34:09 +0200 User-Agent: Mozilla/5.0 (X11; Linux aarch64; rv:68.0) Gecko/20100101 Thunderbird/68.9.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit On 6/11/20 12:38 AM, Kinney, Michael D wrote: > Ard, > > Can you add a comment block before each array and explain the > hardcoded 25 and 21 values. I assume that is the length of > the longest string +1 for the Null-terminator. > Yes, will do. > I assume if a string is added or modified that is longer > than the fixed size the compiler will throw an error. So > hopefully, the worst that happens with an incorrect value > that builds is some extra filler of zeros. > Exactly. >> -----Original Message----- >> From: devel@edk2.groups.io On >> Behalf Of Ard Biesheuvel >> Sent: Wednesday, June 10, 2020 12:30 PM >> To: devel@edk2.groups.io >> Cc: Ard Biesheuvel ; Kinney, >> Michael D ; Gao, Liming >> >> Subject: [edk2-devel] [PATCH v2] MdePkg/BasePrintLib: >> avoid absolute addresses for error strings >> >> The mStatusString[] array is constructed as an array of >> pointer-to-char, >> which means that on X64 or AARCH64, it is emitted as a >> single linear list >> of 64-bit quantities, each containing the absolute >> address of one of the >> string literals in memory. >> >> This means that each string takes up 8 bytes of >> additional space, along >> with 2 bytes of relocation data. It also means that >> extra work needs to >> be done at runtime to process these relocations, every >> time a module is >> loaded that incorporates this library. >> >> So fix both issues, by splitting mStatusString into two >> arrays of char >> arrays. The memory footprint decreases from 955 to 843 >> bytes, and given >> that in the latter case, the overhead consists of 278 >> NUL characters rather >> than 390 bytes worth of absolute addresses and >> relocation records, the size >> of a compressed image is reduced even further. For >> example, when building >> ArmVirtQemu.dsc in RELEASE mode for AARCH64 with the >> GCC5 profile, I get: >> >> Before >> >> FV Space Information >> FVMAIN [100%Full] 5329920 total, 5329920 used, 0 free >> FVMAIN_COMPACT [38%Full] 2093056 total, 811840 used, >> 1281216 free >> >> After >> >> FV Space Information >> FVMAIN [100%Full] 5321728 total, 5321728 used, 0 free >> FVMAIN_COMPACT [38%Full] 2093056 total, 809696 used, >> 1283360 free >> >> So the uncompressed contents of the compressed image are >> 8 KB smaller, >> whereas the resulting flash image (consisting of the >> compressed image >> along with SEC, PEI_CORE and a set of PEIMs that execute >> in place) is >> ~2 KB smaller. >> >> Cc: "Kinney, Michael D" >> Cc: "Gao, Liming" >> Signed-off-by: Ard Biesheuvel >> --- >> v2: >> - split off this patch from the StandaloneMmPkg series, >> since they are not >> interdependent anyway, and so they can be discussed >> separately >> - remove mention of StandaloneMmPkg from the commit log >> - the space savings >> by themselves are sufficient justification >> - update the before/after numbers with the current >> results >> - split the warnings and errors into a separate array, >> so that the latter >> can use smaller entries >> - clarify the commit log to explain the effect on >> compressed as well as >> XIP images (which both get smaller) >> >> MdePkg/Library/BasePrintLib/PrintLibInternal.c | 9 >> ++++++--- >> 1 file changed, 6 insertions(+), 3 deletions(-) >> >> diff --git >> a/MdePkg/Library/BasePrintLib/PrintLibInternal.c >> b/MdePkg/Library/BasePrintLib/PrintLibInternal.c >> index b6ec5ac4fbb9..522c3bb5dcb9 100644 >> --- a/MdePkg/Library/BasePrintLib/PrintLibInternal.c >> +++ b/MdePkg/Library/BasePrintLib/PrintLibInternal.c >> @@ -27,13 +27,16 @@ >> >> >> GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8 mHexStr[] = >> {'0','1','2','3','4','5','6','7','8','9','A','B','C','D' >> ,'E','F'}; >> >> >> >> -GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8 * CONST >> mStatusString[] = { >> >> +GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8 >> mWarningString[][25] = { >> >> "Success", // RETURN_SUCCESS >> = 0 >> >> "Warning Unknown Glyph", // >> RETURN_WARN_UNKNOWN_GLYPH = 1 >> >> "Warning Delete Failure", // >> RETURN_WARN_DELETE_FAILURE = 2 >> >> "Warning Write Failure", // >> RETURN_WARN_WRITE_FAILURE = 3 >> >> "Warning Buffer Too Small", // >> RETURN_WARN_BUFFER_TOO_SMALL = 4 >> >> "Warning Stale Data", // >> RETURN_WARN_STALE_DATA = 5 >> >> +}; >> >> + >> >> +GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8 >> mErrorString[][21] = { >> >> "Load Error", // RETURN_LOAD_ERROR >> = 1 | MAX_BIT >> >> "Invalid Parameter", // >> RETURN_INVALID_PARAMETER = 2 | MAX_BIT >> >> "Unsupported", // >> RETURN_UNSUPPORTED = 3 | MAX_BIT >> >> @@ -996,12 +999,12 @@ BasePrintLibSPrintMarker ( >> // >> >> Index = Status & ~MAX_BIT; >> >> if (Index > 0 && Index <= >> ERROR_STATUS_NUMBER) { >> >> - ArgumentString = mStatusString [Index + >> WARNING_STATUS_NUMBER]; >> >> + ArgumentString = mErrorString [Index - 1]; >> >> } >> >> } else { >> >> Index = Status; >> >> if (Index <= WARNING_STATUS_NUMBER) { >> >> - ArgumentString = mStatusString [Index]; >> >> + ArgumentString = mWarningString [Index]; >> >> } >> >> } >> >> if (ArgumentString == ValueBuffer) { >> >> -- >> 2.26.2 >> >> >> -=-=-=-=-=-= >> Groups.io Links: You receive all messages sent to this >> group. >> >> View/Reply Online (#61089): >> https://edk2.groups.io/g/devel/message/61089 >> Mute This Topic: https://groups.io/mt/74804313/1643496 >> Group Owner: devel+owner@edk2.groups.io >> Unsubscribe: https://edk2.groups.io/g/devel/unsub >> [michael.d.kinney@intel.com] >> -=-=-=-=-=-= >