[edk2-devel] ECC: Won't somebody PLEASE think of the... test structures.

[Laszlo Ersek]

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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#65610): https://edk2.groups.io/g/devel/message/65610
Mute This Topic: https://groups.io/mt/77071159/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-