[edk2-devel] [PATCH v3] MdePkg/BasePrintLib: avoid absolute addresses for error strings
Andrew Fish via groups.io
afish=apple.com at groups.io
Tue Jun 16 21:12:36 UTC 2020
I was wondering what would happen if we converted the CHAR8 arrays in to strings with multiple nulls [1]. Looks like it saves space in uncompressed FVs, but not in compressed FVs.
Here are the Xcode numbers for X64 DEBUG Ovmf:
With this Patch:
SECFV [14%Full] 212992 total, 30128 used, 182864 free
PEIFV [29%Full] 917504 total, 273192 used, 644312 free
DXEFV [39%Full] 12582912 total, 4997120 used, 7585792 free
FVMAIN_COMPACT [36%Full] 3440640 total, 1271264 used, 2169376 free
Vs my patch:
SECFV [14%Full] 212992 total, 29872 used, 183120 free
PEIFV [29%Full] 917504 total, 271048 used, 646456 free
DXEFV [39%Full] 12582912 total, 4979552 used, 7603360 free
FVMAIN_COMPACT [36%Full] 3440640 total, 1271824 used, 2168816 free
SEC: -256
PEI: -2,144
Dxe: -17,568
Compact: +560
[1] $ git diff
diff --git a/MdePkg/Library/BasePrintLib/PrintLibInternal.c b/MdePkg/Library/BasePrintLib/PrintLibInternal.c
index 50c6e8559c43..db2533e7affb 100644
--- a/MdePkg/Library/BasePrintLib/PrintLibInternal.c
+++ b/MdePkg/Library/BasePrintLib/PrintLibInternal.c
@@ -30,53 +30,73 @@ GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8 mHexStr[] = {'0','1','2','3','4','5','
//
// 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
-};
+GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8 *mWarningString = \
+ "Success\0" /* RETURN_SUCCESS = 0 */ \
+ "Warning Unknown Glyph\0" /* RETURN_WARN_UNKNOWN_GLYPH = 1 */ \
+ "Warning Delete Failure\0" /* RETURN_WARN_DELETE_FAILURE = 2 */ \
+ "Warning Write Failure\0" /* RETURN_WARN_WRITE_FAILURE = 3 */ \
+ "Warning Buffer Too Small\0" /* 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
- "Bad Buffer Size", // RETURN_BAD_BUFFER_SIZE = 4 | MAX_BIT
- "Buffer Too Small", // RETURN_BUFFER_TOO_SMALL, = 5 | MAX_BIT
- "Not Ready", // RETURN_NOT_READY = 6 | MAX_BIT
- "Device Error", // RETURN_DEVICE_ERROR = 7 | MAX_BIT
- "Write Protected", // RETURN_WRITE_PROTECTED = 8 | MAX_BIT
- "Out of Resources", // RETURN_OUT_OF_RESOURCES = 9 | MAX_BIT
- "Volume Corrupt", // RETURN_VOLUME_CORRUPTED = 10 | MAX_BIT
- "Volume Full", // RETURN_VOLUME_FULL = 11 | MAX_BIT
- "No Media", // RETURN_NO_MEDIA = 12 | MAX_BIT
- "Media changed", // RETURN_MEDIA_CHANGED = 13 | MAX_BIT
- "Not Found", // RETURN_NOT_FOUND = 14 | MAX_BIT
- "Access Denied", // RETURN_ACCESS_DENIED = 15 | MAX_BIT
- "No Response", // RETURN_NO_RESPONSE = 16 | MAX_BIT
- "No mapping", // RETURN_NO_MAPPING = 17 | MAX_BIT
- "Time out", // RETURN_TIMEOUT = 18 | MAX_BIT
- "Not started", // RETURN_NOT_STARTED = 19 | MAX_BIT
- "Already started", // RETURN_ALREADY_STARTED = 20 | MAX_BIT
- "Aborted", // RETURN_ABORTED = 21 | MAX_BIT
- "ICMP Error", // RETURN_ICMP_ERROR = 22 | MAX_BIT
- "TFTP Error", // RETURN_TFTP_ERROR = 23 | MAX_BIT
- "Protocol Error", // RETURN_PROTOCOL_ERROR = 24 | MAX_BIT
- "Incompatible Version", // RETURN_INCOMPATIBLE_VERSION = 25 | MAX_BIT
- "Security Violation", // RETURN_SECURITY_VIOLATION = 26 | MAX_BIT
- "CRC Error", // RETURN_CRC_ERROR = 27 | MAX_BIT
- "End of Media", // RETURN_END_OF_MEDIA = 28 | MAX_BIT
- "Reserved (29)", // RESERVED = 29 | MAX_BIT
- "Reserved (30)", // RESERVED = 30 | MAX_BIT
- "End of File", // RETURN_END_OF_FILE = 31 | MAX_BIT
- "Invalid Language", // RETURN_INVALID_LANGUAGE = 32 | MAX_BIT
- "Compromised Data" // RETURN_COMPROMISED_DATA = 33 | MAX_BIT
-};
+GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8 *mErrorString = \
+ "Load Error\0" /* RETURN_LOAD_ERROR = 1 | MAX_BIT */ \
+ "Invalid Parameter\0" /* RETURN_INVALID_PARAMETER = 2 | MAX_BIT */ \
+ "Unsupported\0" /* RETURN_UNSUPPORTED = 3 | MAX_BIT */ \
+ "Bad Buffer Size\0" /* RETURN_BAD_BUFFER_SIZE = 4 | MAX_BIT */ \
+ "Buffer Too Small\0" /* RETURN_BUFFER_TOO_SMALL, = 5 | MAX_BIT */ \
+ "Not Ready\0" /* RETURN_NOT_READY = 6 | MAX_BIT */ \
+ "Device Error\0" /* RETURN_DEVICE_ERROR = 7 | MAX_BIT */ \
+ "Write Protected\0" /* RETURN_WRITE_PROTECTED = 8 | MAX_BIT */ \
+ "Out of Resources\0" /* RETURN_OUT_OF_RESOURCES = 9 | MAX_BIT */ \
+ "Volume Corrupt\0" /* RETURN_VOLUME_CORRUPTED = 10 | MAX_BIT */ \
+ "Volume Full\0" /* RETURN_VOLUME_FULL = 11 | MAX_BIT */ \
+ "No Media\0" /* RETURN_NO_MEDIA = 12 | MAX_BIT */ \
+ "Media changed\0" /* RETURN_MEDIA_CHANGED = 13 | MAX_BIT */ \
+ "Not Found\0" /* RETURN_NOT_FOUND = 14 | MAX_BIT */ \
+ "Access Denied\0" /* RETURN_ACCESS_DENIED = 15 | MAX_BIT */ \
+ "No Response\0" /* RETURN_NO_RESPONSE = 16 | MAX_BIT */ \
+ "No mapping\0" /* RETURN_NO_MAPPING = 17 | MAX_BIT */ \
+ "Time out\0" /* RETURN_TIMEOUT = 18 | MAX_BIT */ \
+ "Not started\0" /* RETURN_NOT_STARTED = 19 | MAX_BIT */ \
+ "Already started\0" /* RETURN_ALREADY_STARTED = 20 | MAX_BIT */ \
+ "Aborted\0" /* RETURN_ABORTED = 21 | MAX_BIT */ \
+ "ICMP Error\0" /* RETURN_ICMP_ERROR = 22 | MAX_BIT */ \
+ "TFTP Error\0" /* RETURN_TFTP_ERROR = 23 | MAX_BIT */ \
+ "Protocol Error\0" /* RETURN_PROTOCOL_ERROR = 24 | MAX_BIT */ \
+ "Incompatible Version\0" /* RETURN_INCOMPATIBLE_VERSION = 25 | MAX_BIT */ \
+ "Security Violation\0" /* RETURN_SECURITY_VIOLATION = 26 | MAX_BIT */ \
+ "CRC Error\0" /* RETURN_CRC_ERROR = 27 | MAX_BIT */ \
+ "End of Media\0" /* RETURN_END_OF_MEDIA = 28 | MAX_BIT */ \
+ "Reserved (29)\0" /* RESERVED = 29 | MAX_BIT */ \
+ "Reserved (30)\0" /* RESERVED = 30 | MAX_BIT */ \
+ "End of File\0" /* RETURN_END_OF_FILE = 31 | MAX_BIT */ \
+ "Invalid Language\0" /* RETURN_INVALID_LANGUAGE = 32 | MAX_BIT */ \
+ "Compromised Data"; /* RETURN_COMPROMISED_DATA = 33 | MAX_BIT */
+
+
+CONST CHAR8 *
+FindNthStr (
+ IN CONST CHAR8 *Start,
+ IN UINTN Index
+ )
+{
+ CONST CHAR8 *str;
+
+ for (str = Start; Index > 0; str++) {
+ if (*str == '\0') {
+ Index--;
+ if (Index == 0) {
+ str++;
+ break;
+ }
+ }
+ }
+
+ return str;
+}
/**
@@ -1005,12 +1025,12 @@ BasePrintLibSPrintMarker (
//
Index = Status & ~MAX_BIT;
if (Index > 0 && Index <= ERROR_STATUS_NUMBER) {
- ArgumentString = mErrorString [Index - 1];
+ ArgumentString = FindNthStr (mErrorString, Index - 1);
}
} else {
Index = Status;
if (Index <= WARNING_STATUS_NUMBER) {
- ArgumentString = mWarningString [Index];
+ ArgumentString = FindNthStr (mWarningString, Index);
}
}
if (ArgumentString == ValueBuffer) {
Thanks,
Andrew Fish
> On Jun 15, 2020, at 6:57 PM, Liming Gao <liming.gao at intel.com> wrote:
>
> This data is great. The change is good. Reviewed-by: Liming Gao <liming.gao at intel.com <mailto:liming.gao at intel.com>>
>
> From: Liu, Zhiguang <zhiguang.liu at intel.com <mailto:zhiguang.liu at intel.com>>
> Sent: 2020年6月15日 15:34
> To: Ard Biesheuvel <ard.biesheuvel at arm.com <mailto:ard.biesheuvel at arm.com>>; Gao, Liming <liming.gao at intel.com <mailto:liming.gao at intel.com>>; devel at edk2.groups.io <mailto:devel at edk2.groups.io>
> Cc: Kinney, Michael D <michael.d.kinney at intel.com <mailto:michael.d.kinney at intel.com>>
> Subject: RE: [edk2-devel] [PATCH v3] MdePkg/BasePrintLib: avoid absolute addresses for error strings
>
> Hi Ard,
>
> I also collected the image size of OVMFX64 size building with VS2015x86.
>
> Debug
> Release
>
> before
> after
> After-before
> before
> after
> after-before
> sec
> 27664
> 27409
> -255
> 13968
> 13968
> 0
> pei
> 223016
> 221000
> -2016
> 107208
> 106984
> -224
> dxe
> 4507000
> 4481336
> -25664
> 2987064
> 2979384
> -7680
> compact
> 1179776
> 1172528
> -7248
> 922664
> 920304
> -2360
>
> It can reduce the image size in X64.
> Reviewed-by: Zhiguang Liu <zhiguang.liu at intel.com <mailto:zhiguang.liu at intel.com>>
>
> > -----Original Message-----
> > From: Ard Biesheuvel <ard.biesheuvel at arm.com <mailto:ard.biesheuvel at arm.com>>
> > Sent: Friday, June 12, 2020 11:11 PM
> > To: Gao, Liming <liming.gao at intel.com <mailto:liming.gao at intel.com>>; devel at edk2.groups.io <mailto:devel at edk2.groups.io>
> > Cc: Kinney, Michael D <michael.d.kinney at intel.com <mailto:michael.d.kinney at intel.com>>; Liu, Zhiguang
> > <zhiguang.liu at intel.com <mailto:zhiguang.liu at intel.com>>
> > Subject: Re: [edk2-devel] [PATCH v3] MdePkg/BasePrintLib: avoid absolute
> > addresses for error strings
> >
> > 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 at edk2.groups.io <mailto:devel at edk2.groups.io> <devel at edk2.groups.io <mailto:devel at edk2.groups.io>> On Behalf Of Ard
> > Biesheuvel
> > >> Sent: Friday, June 12, 2020 6:35 AM
> > >> To: devel at edk2.groups.io <mailto:devel at edk2.groups.io>
> > >> Cc: Ard Biesheuvel <ard.biesheuvel at arm.com <mailto:ard.biesheuvel at arm.com>>; Kinney, Michael D
> > <michael.d.kinney at intel.com <mailto:michael.d.kinney at intel.com>>; Gao, Liming <liming.gao at intel.com <mailto:liming.gao at 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 at intel.com <mailto:michael.d.kinney at intel.com>>
> > >> Cc: "Gao, Liming" <liming.gao at intel.com <mailto:liming.gao at intel.com>>
> > >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel at arm.com <mailto:ard.biesheuvel at 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 <http://groups.io/> Links: You receive all messages sent to this group.
> > >>
> > >> View/Reply Online (#61170):
> > https://edk2.groups.io/g/devel/message/61170 <https://edk2.groups.io/g/devel/message/61170>
> > >> Mute This Topic: https://groups.io/mt/74829004/1759384 <https://groups.io/mt/74829004/1759384>
> > >> Group Owner: devel+owner at edk2.groups.io <mailto:devel+owner at edk2.groups.io>
> > >> Unsubscribe: https://edk2.groups.io/g/devel/unsub <https://edk2.groups.io/g/devel/unsub>
> > [liming.gao at intel.com <mailto:liming.gao at intel.com>]
> > >> -=-=-=-=-=-=
> > >
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#61359): https://edk2.groups.io/g/devel/message/61359
Mute This Topic: https://groups.io/mt/74829004/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/edk2-devel-archive/attachments/20200616/e2a46c76/attachment.htm>
More information about the edk2-devel-archive
mailing list