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

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

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 <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 <ard.biesheuvel@arm.com>; Kinney,
>> Michael D <michael.d.kinney@intel.com>; Gao, Liming
>> <liming.gao@intel.com>
>> 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" <michael.d.kinney@intel.com>
>> Cc: "Gao, Liming" <liming.gao@intel.com>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> ---
>> 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]
>> -=-=-=-=-=-=
>