-----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
