Hi Ard, The size reduction is very interesting. Would be good to see uncompressed size differences as well because this lib is also used in pre-memory XIP modules that are fixed up by the build tools that handle the relocation fixups.
The more general concern I have is if there are standard C coding practices (such as a pre-initialized array of pointers to strings) that are not compatible with StandaloneMmPkg use cases. Mike > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On > Behalf Of Ard Biesheuvel > Sent: Wednesday, June 10, 2020 1:37 AM > To: devel@edk2.groups.io > Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Gao, > Liming <liming....@intel.com>; Yao, Jiewen > <jiewen....@intel.com>; Sami Mujawar > <sami.muja...@arm.com>; Ilias Apalodimas > <ilias.apalodi...@linaro.org> > Subject: Re: [edk2-devel] [PATCH 1/5] > MdePkg/BasePrintLib: avoid absolute addresses for error > strings > > On 6/10/20 10:17 AM, Ard Biesheuvel wrote: > > The mStatusString[] 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 > the array cannot be > > used until PE/COFF relocation has completed, and so > the following > > invocation > > > > Status = PeCoffLoaderRelocateImage > (&ImageContext); > > ASSERT_EFI_ERROR (Status); > > > > that we will be introducing into StandaloneMmCore > entrypoint for AARCH64 > > to relocate the executable on the fly is guaranteed to > return bogus output > > or crash, which is less than helpful. > > > > So fix both issues, by emitting mStatusString as an > array of char arrays > > instead. The memory footprint increases from 955 to > 975 bytes, but given > > that in the latter case, the overhead consists of 410 > NUL characters > > rather than 390 bytes worth of absolute addresses and > relocation records, > > the impact on a compressed image is actually positive. > For example, when > > building ArmVirtQemu.dsc in RELEASE mode for AARCH64 > with the GCC5 profile, > > I get: > > > > Before > > > > FV Space Information > > FVMAIN [99%Full] 4784768 total, 4784720 used, 48 > free > > FVMAIN_COMPACT [38%Full] 2093056 total, 811560 > used, 1281496 free > > > > After > > > > FV Space Information > > FVMAIN [99%Full] 4780672 total, 4780624 used, 48 > free > > FVMAIN_COMPACT [38%Full] 2093056 total, 813488 > used, 1279568 free > > > > So the compressed image is 4 KB smaller, whereas the > entire image is > > < 2 KB larger, which is in the order of 0.2 % > > > > Signed-off-by: Ard Biesheuvel <ard.biesheu...@arm.com> > > Actually, I found a slightly better way of doing this, > by applying the > following delta on top, i.e., split the errors and > warnings, and use an > array of 21 byte character arrays for the former. > > My build went from > > FV Space Information > > FVMAIN [99%Full] 4784768 total, 4784720 used, 48 free > FVMAIN_COMPACT [37%Full] 2093056 total, 792984 used, > 1300072 free > > to > > FV Space Information > FVMAIN [99%Full] 4780672 total, 4780624 used, 48 free > FVMAIN_COMPACT [37%Full] 2093056 total, 791072 used, > 1301984 free > > > in this case, i.e., both compressed and non-compressed > images are > slightly smaller. > > > > --- 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 > mStatusString[][25] = { > +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) { > > > > --- > > MdePkg/Library/BasePrintLib/PrintLibInternal.c | 2 > +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git > a/MdePkg/Library/BasePrintLib/PrintLibInternal.c > b/MdePkg/Library/BasePrintLib/PrintLibInternal.c > > index b6ec5ac4fbb9..c8b932c7e07a 100644 > > --- a/MdePkg/Library/BasePrintLib/PrintLibInternal.c > > +++ b/MdePkg/Library/BasePrintLib/PrintLibInternal.c > > @@ -27,7 +27,7 @@ > > > > 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 > mStatusString[][25] = { > > "Success", // RETURN_SUCCESS > = 0 > > "Warning Unknown Glyph", // > RETURN_WARN_UNKNOWN_GLYPH = 1 > > "Warning Delete Failure", // > RETURN_WARN_DELETE_FAILURE = 2 > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#61081): https://edk2.groups.io/g/devel/message/61081 Mute This Topic: https://groups.io/mt/74792288/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-