Re: [edk2-devel] [PATCH v3] MdePkg/BasePrintLib: avoid absolute addresses for error strings

["Ard Biesheuvel" <ard.biesheuvel@arm.com>]

On 6/12/20 4:33 PM, Gao, Liming wrote:
> Ard:
>    I will collect the image size on OVMF X64 platform with this patch.
> 

Building OvmfPkgX64.dsc in RELEASE mode using GCC5 profile gives me


Before:
SECFV [5%Full] 212992 total, 11760 used, 201232 free
PEIFV [9%Full] 917504 total, 89384 used, 828120 free
DXEFV [22%Full] 12582912 total, 2806320 used, 9776592 free
FVMAIN_COMPACT [26%Full] 3440640 total, 918760 used, 2521880 free

After:
SECFV [5%Full] 212992 total, 11760 used, 201232 free
PEIFV [9%Full] 917504 total, 89192 used, 828312 free
DXEFV [22%Full] 12582912 total, 2802928 used, 9779984 free
FVMAIN_COMPACT [26%Full] 3440640 total, 916784 used, 2523856 free

So no change for SECFV, -192 bytes for PEIFV, -3392 bytes for DXEFV and 
-1976 bytes for the resulting outer FV image.



>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard Biesheuvel
>> Sent: Friday, June 12, 2020 6:35 AM
>> To: devel@edk2.groups.io
>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>
>> Subject: [edk2-devel] [PATCH v3] 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" <michael.d.kinney@intel.com>
>> Cc: "Gao, Liming" <liming.gao@intel.com>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> ---
>> v3:
>> - add code comments to explain what the inner dimension of each array is
>>    based on
>>
>> 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 | 15 ++++++++++++---
>>   1 file changed, 12 insertions(+), 3 deletions(-)
>>
>> diff --git a/MdePkg/Library/BasePrintLib/PrintLibInternal.c b/MdePkg/Library/BasePrintLib/PrintLibInternal.c
>> index b6ec5ac4fbb9..50c6e8559c43 100644
>> --- a/MdePkg/Library/BasePrintLib/PrintLibInternal.c
>> +++ b/MdePkg/Library/BasePrintLib/PrintLibInternal.c
>> @@ -27,13 +27,22 @@
>>
>>
>>   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[] = {
>>
>> +//
>>
>> +// Longest string: RETURN_WARN_BUFFER_TOO_SMALL => 24 characters plus NUL byte
>>
>> +//
>>
>> +GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8 mWarningString[][24+1] = {
>>
>>     "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
>>
>> +};
>>
>> +
>>
>> +//
>>
>> +// Longest string: RETURN_INCOMPATIBLE_VERSION => 20 characters plus NUL byte
>>
>> +//
>>
>> +GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8 mErrorString[][20+1] = {
>>
>>     "Load Error",                   //  RETURN_LOAD_ERROR             = 1  | MAX_BIT
>>
>>     "Invalid Parameter",            //  RETURN_INVALID_PARAMETER      = 2  | MAX_BIT
>>
>>     "Unsupported",                  //  RETURN_UNSUPPORTED            = 3  | MAX_BIT
>>
>> @@ -996,12 +1005,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 (#61170): https://edk2.groups.io/g/devel/message/61170
>> Mute This Topic: https://groups.io/mt/74829004/1759384
>> Group Owner: devel+owner@edk2.groups.io
>> Unsubscribe: https://edk2.groups.io/g/devel/unsub  [liming.gao@intel.com]
>> -=-=-=-=-=-=
>