From: "Laszlo Ersek" <lersek@redhat.com>
To: devel@edk2.groups.io, bret.barkelew@microsoft.com,
Ken Taylor <Ken_Taylor@phoenix.com>
Subject: Re: [edk2-devel] ECC: Won't somebody PLEASE think of the... test structures.
Date: Fri, 25 Sep 2020 09:31:11 +0200 [thread overview]
Message-ID: <802fe601-f062-131c-5eae-4b4599c4447c@redhat.com> (raw)
In-Reply-To: <MW4PR21MB1857F3476D241BFDC4C5ABA3EF360@MW4PR21MB1857.namprd21.prod.outlook.com>
On 09/25/20 04:25, Bret Barkelew via groups.io wrote:
> So for context, this is a new host-based test that should only run
> within a platform OS, so intrinsics aren't the big deal that they
> would be in FW code.
>
> But we do need to figure out how to simultaneously adhere to the
> coding convention while enabling test authoring.
The EvaluatePolicyMatch() function -- very helpfully -- takes a
const-qualified pointer to VARIABLE_POLICY_ENTRY.
That allows me to see at once that the "MatchCheckPolicy" variable is
not going to be modified, in the PoliciesShouldMatchByNameAndGuid()
function
[MdeModulePkg/Library/VariablePolicyLib/VariablePolicyUnitTest/VariablePolicyUnitTest.c].
So the solution is to give "MatchCheckPolicy" static storage duration
rather than automatic. For good measure, make it CONST too:
> diff --git a/MdeModulePkg/Library/VariablePolicyLib/VariablePolicyUnitTest/VariablePolicyUnitTest.c b/MdeModulePkg/Library/VariablePolicyLib/VariablePolicyUnitTest/VariablePolicyUnitTest.c
> index 40e946a73814..ab05989c36e7 100644
> --- a/MdeModulePkg/Library/VariablePolicyLib/VariablePolicyUnitTest/VariablePolicyUnitTest.c
> +++ b/MdeModulePkg/Library/VariablePolicyLib/VariablePolicyUnitTest/VariablePolicyUnitTest.c
> @@ -330,7 +330,7 @@ PoliciesShouldMatchByNameAndGuid (
> IN UNIT_TEST_CONTEXT Context
> )
> {
> - SIMPLE_VARIABLE_POLICY_ENTRY MatchCheckPolicy = {
> + STATIC CONST SIMPLE_VARIABLE_POLICY_ENTRY MatchCheckPolicy = {
> {
> VARIABLE_POLICY_ENTRY_REVISION,
> sizeof(VARIABLE_POLICY_ENTRY) + sizeof(TEST_VAR_1_NAME),
In my opinion, this is an improvement *regardless* of the restrictions
that edk2 places on compiler intrinsics. Namely, even in a hosted C
environment, the actual data content needs to exist *somewhere* in the
executable file, and correspondingly, in the executing process. And so
when the struct with automatic storage duration is being initialized,
there is either a large quick copy (cue the intrinsic), or -- naively --
a bunch of member-wise assignments behind the curtain (where the
initializer values for the scalar fields could even be encoded as
constants in the instruction stream).
Either way, the data comes from *somewhere* -- so why not just *label*
that original data with a name, and refer to the data by that name?
And that's exactly what STATIC CONST does.
(I realize the actual data content might end up with a different
representation and/or in a different section of the on-disk executable,
but that fact does not invalidate my point.)
In case you do need the variable to be writeable and/or to have auto
storage duration (e.g., because you need to fix up a few fields before
use), the common pattern in edk2 is to have a template object (usually
at file scope, not at function scope), such as:
STATIC CONST SIMPLE_VARIABLE_POLICY_ENTRY mMatchCheckPolicy = { ... };
(note the "m" prefix on the name).
Subsequently, wherever you need a writeable copy (local variable, or
dynamically allocated object), use CopyMem() or AllocateCopyPool(),
respectively. Then fix up any fields that require that.
Thanks,
Laszlo
prev parent reply other threads:[~2020-09-25 7:31 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-25 1:23 ECC: Won't somebody PLEASE think of the... test structures Bret Barkelew
2020-09-25 1:56 ` Ken Taylor
2020-09-25 2:25 ` Bret Barkelew
2020-09-25 2:46 ` Bret Barkelew
2020-09-25 2:48 ` [edk2-devel] " Andrew Fish
2020-09-25 2:57 ` [EXTERNAL] " Bret Barkelew
2020-09-26 2:52 ` [EXTERNAL] " Andrew Fish
2020-09-27 3:40 ` Bret Barkelew
2020-09-28 18:16 ` Laszlo Ersek
2020-09-25 7:31 ` Laszlo Ersek [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=802fe601-f062-131c-5eae-4b4599c4447c@redhat.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox