* [PATCH v2] MdePkg/BasePrintLib: avoid absolute addresses for error strings
@ 2020-06-10 19:30 Ard Biesheuvel
2020-06-10 22:38 ` [edk2-devel] " Michael D Kinney
0 siblings, 1 reply; 3+ messages in thread
From: Ard Biesheuvel @ 2020-06-10 19:30 UTC (permalink / raw)
To: devel; +Cc: Ard Biesheuvel, Kinney, Michael D, Gao, Liming
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [edk2-devel] [PATCH v2] MdePkg/BasePrintLib: avoid absolute addresses for error strings
2020-06-10 19:30 [PATCH v2] MdePkg/BasePrintLib: avoid absolute addresses for error strings Ard Biesheuvel
@ 2020-06-10 22:38 ` Michael D Kinney
2020-06-11 7:34 ` Ard Biesheuvel
0 siblings, 1 reply; 3+ messages in thread
From: Michael D Kinney @ 2020-06-10 22:38 UTC (permalink / raw)
To: devel@edk2.groups.io, ard.biesheuvel@arm.com, Kinney, Michael D
Cc: Gao, Liming
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.
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.
Mike
> -----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]
> -=-=-=-=-=-=
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [edk2-devel] [PATCH v2] MdePkg/BasePrintLib: avoid absolute addresses for error strings
2020-06-10 22:38 ` [edk2-devel] " Michael D Kinney
@ 2020-06-11 7:34 ` Ard Biesheuvel
0 siblings, 0 replies; 3+ messages in thread
From: Ard Biesheuvel @ 2020-06-11 7:34 UTC (permalink / raw)
To: Kinney, Michael D, devel@edk2.groups.io; +Cc: Gao, Liming
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]
>> -=-=-=-=-=-=
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-06-11 7:34 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-06-10 19:30 [PATCH v2] MdePkg/BasePrintLib: avoid absolute addresses for error strings Ard Biesheuvel
2020-06-10 22:38 ` [edk2-devel] " Michael D Kinney
2020-06-11 7:34 ` Ard Biesheuvel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox