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

2020-09-25 Thread 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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

2020-09-24 Thread Bret Barkelew via groups.io 
Andrew,

That’s actually what got me here. EccCheck runs as part of our CI now (but it 
didn’t when I wrote these tests a year ago). I need to either figure out how to 
get this code to pass EccCheck in a reasonable way, or just not contribute the 
tests and say “go to Mu for the tests, if you want them”.

Skipping the contribution isn’t a desirable outcome at all for a number of 
reasons, not the least of which is that we (MS and others) are trying encourage 
all contributions to come with tests, so the process of writing them needs to 
be simple and painless.

- Bret

From: Andrew Fish<mailto:af...@apple.com>
Sent: Thursday, September 24, 2020 7:48 PM
To: edk2-devel-groups-io<mailto:devel@edk2.groups.io>; Bret 
Barkelew<mailto:bret.barke...@microsoft.com>
Cc: Ken Taylor<mailto:ken_tay...@phoenix.com>
Subject: [EXTERNAL] Re: [edk2-devel] ECC: Won't somebody PLEASE think of the... 
test structures.

Bret,

I’ve had this issue with EFI code too. It will compile with for DEBUG and 
RELEASE as the optimizer removes the memcpy/memset. So you only see a build 
failure when you compiler NOOPT (and there are no intrinsic libs). I mostly see 
this in platform code when I try to compile a single driver/lib NOOPT and it 
fails to link due to the missing intrinsic.

The easy to enforce this is to compile with optimizations enabled and don’t 
enable intrinsic libs. Not sure if that is really practical from the test point 
of view.

Seems the tool caught the coding style violation so I guess we could try to 
“make running that tool easier”. Maybe hooking into patchcheck.py, making some 
kind of githook, or adding a git command?

Thanks,

Andrew Fish



On Sep 24, 2020, at 7:25 PM, Bret Barkelew via 
groups.io<https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fgroups.io%2F&data=02%7C01%7Cbret.barkelew%40microsoft.com%7Cd438e9952e1c476285f608d860fd7e71%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637365989149774199&sdata=SaAatm8guPgl%2F63Sc%2B0AAbReBqeBPuKBJZjAmEWstU8%3D&reserved=0>
 
mailto:bret.barkelew=microsoft@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.
Or we chose to not enforce quite as many things for tests.

I’d prefer the first.

- Bret

From: Ken Taylor<mailto:ken_tay...@phoenix.com>
Sent: Thursday, September 24, 2020 6:57 PM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Bret 
Barkelew<mailto:bret.barke...@microsoft.com>
Subject: [EXTERNAL] RE: ECC: Won't somebody PLEASE think of the... test 
structures.

If the structure is a non-static local variable, most compilers will silently 
inject an intrinsic call to memcpy in function initialization.  This leads to 
an intermittent linker error.

If the compiler you use automatically supports an intrinsic memcpy in the given 
architecture or optimizes out the memcpy, it will build for you and you won’t 
know you need to link to an intrinsic support library in order to build cross 
platform.  This leads to code that builds for you, but not for me.

Regards,
-Ken.

From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> 
[mailto:devel@edk2.groups.io] On Behalf Of Bret Barkelew via 
groups.io<https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fgroups.io%2F&data=02%7C01%7Cbret.barkelew%40microsoft.com%7Cd438e9952e1c476285f608d860fd7e71%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637365989149784192&sdata=RYkXf9eJ%2B%2BAAFofHhAD2BisFsokbP28A6pyE5iaqRpo%3D&reserved=0>
Sent: Thursday, September 24, 2020 6:23 PM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Subject: [edk2-devel] ECC: Won't somebody PLEASE think of the... test 
structures.

ERROR - EFI coding style error
ERROR - *Error code: 5007
ERROR - *There should be no initialization of a variable as part of its 
declaration
ERROR - *file: 
//home/corthon/_uefi/edk2_qemu_ci/edk2/MdeModulePkg/Library/VariablePolicyLib/VariablePolicyUnitTest/VariablePolicyUnitTest.c
ERROR - *Line number: 333
ERROR - *Variable Name: MatchCheckPolicy

EccCheck no likey:
SIMPLE_VARIABLE_POLICY_ENTRY   ValidationPolicy = {
{
  VARIABLE_POLICY_ENTRY_REVISION,
  sizeof(VARIABLE_POLICY_ENTRY) + sizeof(TEST_VAR_1_NAME),
  sizeof(VARIABLE_POLICY_ENTRY),
  TEST_GUID_1,
  TEST_POLICY_MIN_SIZE_NULL,
  TEST_POLICY_MAX_SIZE_NULL,
  TEST_POLICY_ATTRIBUTES_NULL,
  TEST_POLICY_ATTRIBUTES_NULL,
  VARIABLE_POLICY_TYPE_NO_LOCK
},
TEST_VAR_1_NAME
  };

But you can’t init this structure separately without addressing each field.
Can a brother get an override?

- Bret



<95B370A7BEED49AAB797022E8F260C8F.png>




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Rep

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

2020-09-24 Thread Andrew Fish via groups.io 
Bret,

I’ve had this issue with EFI code too. It will compile with for DEBUG and 
RELEASE as the optimizer removes the memcpy/memset. So you only see a build 
failure when you compiler NOOPT (and there are no intrinsic libs). I mostly see 
this in platform code when I try to compile a single driver/lib NOOPT and it 
fails to link due to the missing intrinsic. 

The easy to enforce this is to compile with optimizations enabled and don’t 
enable intrinsic libs. Not sure if that is really practical from the test point 
of view. 

Seems the tool caught the coding style violation so I guess we could try to 
“make running that tool easier”. Maybe hooking into patchcheck.py, making some 
kind of githook, or adding a git command?

Thanks,

Andrew Fish


> On Sep 24, 2020, at 7:25 PM, 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.
> Or we chose to not enforce quite as many things for tests.
>
> I’d prefer the first. 
>
> - Bret 
>
> From: Ken Taylor 
> Sent: Thursday, September 24, 2020 6:57 PM
> To: devel@edk2.groups.io ; Bret Barkelew 
> 
> Subject: [EXTERNAL] RE: ECC: Won't somebody PLEASE think of the... test 
> structures.
>
> If the structure is a non-static local variable, most compilers will silently 
> inject an intrinsic call to memcpy in function initialization.  This leads to 
> an intermittent linker error.
>
> If the compiler you use automatically supports an intrinsic memcpy in the 
> given architecture or optimizes out the memcpy, it will build for you and you 
> won’t know you need to link to an intrinsic support library in order to build 
> cross platform.  This leads to code that builds for you, but not for me.
>
> Regards,
> -Ken.
>
> From: devel@edk2.groups.io  
> [mailto:devel@edk2.groups.io ] On Behalf Of Bret 
> Barkelew via groups.io 
> Sent: Thursday, September 24, 2020 6:23 PM
> To: devel@edk2.groups.io 
> Subject: [edk2-devel] ECC: Won't somebody PLEASE think of the... test 
> structures.
>
> ERROR - EFI coding style error
> ERROR - *Error code: 5007
> ERROR - *There should be no initialization of a variable as part of its 
> declaration
> ERROR - *file: 
> //home/corthon/_uefi/edk2_qemu_ci/edk2/MdeModulePkg/Library/VariablePolicyLib/VariablePolicyUnitTest/VariablePolicyUnitTest.c
> ERROR - *Line number: 333
> ERROR - *Variable Name: MatchCheckPolicy
>
> EccCheck no likey:
> SIMPLE_VARIABLE_POLICY_ENTRY   ValidationPolicy = {
> {
>   VARIABLE_POLICY_ENTRY_REVISION,
>   sizeof(VARIABLE_POLICY_ENTRY) + sizeof(TEST_VAR_1_NAME),
>   sizeof(VARIABLE_POLICY_ENTRY),
>   TEST_GUID_1,
>   TEST_POLICY_MIN_SIZE_NULL,
>   TEST_POLICY_MAX_SIZE_NULL,
>   TEST_POLICY_ATTRIBUTES_NULL,
>   TEST_POLICY_ATTRIBUTES_NULL,
>   VARIABLE_POLICY_TYPE_NO_LOCK
> },
> TEST_VAR_1_NAME
>   };
>
> But you can’t init this structure separately without addressing each field.
> Can a brother get an override?
>
> - Bret 
>
>
> 
> <95B370A7BEED49AAB797022E8F260C8F.png>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#65603): https://edk2.groups.io/g/devel/message/65603
Mute This Topic: https://groups.io/mt/77071159/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

2020-09-24 Thread Bret Barkelew via groups.io 
One of the things we’re really trying to do here is make a strong case for 
contributing good tests, so I want them to be both:

  *   Good citizens of code hygiene, and
  *   Good examples of real-world, non-trivial tests (I actually used the new 
UnitTestFrameworkPkg and test-driven design while writing VarPol way back when)

So I’m happy [for some definitions of ‘happy’] to reform and reflow these as 
needed, but I’m getting the suspicion that there are walls I’m going to hit.

I also don’t want test writing to be so onerous as to prevent contributors for 
writing them. For example: I’m not going to docstring every test case. The test 
case name and framework strings should be clear enough to explain what the case 
is doing (otherwise it’s not a good test case).

- Bret

From: Bret Barkelew via groups.io<mailto:bret.barkelew=microsoft@groups.io>
Sent: Thursday, September 24, 2020 7:26 PM
To: Ken Taylor<mailto:ken_tay...@phoenix.com>; 
devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Subject: [EXTERNAL] Re: [edk2-devel] ECC: Won't somebody PLEASE think of the... 
test structures.

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.
Or we chose to not enforce quite as many things for tests.

I’d prefer the first.

- Bret

From: Ken Taylor<mailto:ken_tay...@phoenix.com>
Sent: Thursday, September 24, 2020 6:57 PM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Bret 
Barkelew<mailto:bret.barke...@microsoft.com>
Subject: [EXTERNAL] RE: ECC: Won't somebody PLEASE think of the... test 
structures.

If the structure is a non-static local variable, most compilers will silently 
inject an intrinsic call to memcpy in function initialization.  This leads to 
an intermittent linker error.

If the compiler you use automatically supports an intrinsic memcpy in the given 
architecture or optimizes out the memcpy, it will build for you and you won’t 
know you need to link to an intrinsic support library in order to build cross 
platform.  This leads to code that builds for you, but not for me.

Regards,
-Ken.

From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Bret 
Barkelew via groups.io
Sent: Thursday, September 24, 2020 6:23 PM
To: devel@edk2.groups.io
Subject: [edk2-devel] ECC: Won't somebody PLEASE think of the... test 
structures.

ERROR - EFI coding style error
ERROR - *Error code: 5007
ERROR - *There should be no initialization of a variable as part of its 
declaration
ERROR - *file: 
//home/corthon/_uefi/edk2_qemu_ci/edk2/MdeModulePkg/Library/VariablePolicyLib/VariablePolicyUnitTest/VariablePolicyUnitTest.c
ERROR - *Line number: 333
ERROR - *Variable Name: MatchCheckPolicy

EccCheck no likey:
SIMPLE_VARIABLE_POLICY_ENTRY   ValidationPolicy = {
{
  VARIABLE_POLICY_ENTRY_REVISION,
  sizeof(VARIABLE_POLICY_ENTRY) + sizeof(TEST_VAR_1_NAME),
  sizeof(VARIABLE_POLICY_ENTRY),
  TEST_GUID_1,
  TEST_POLICY_MIN_SIZE_NULL,
  TEST_POLICY_MAX_SIZE_NULL,
  TEST_POLICY_ATTRIBUTES_NULL,
  TEST_POLICY_ATTRIBUTES_NULL,
  VARIABLE_POLICY_TYPE_NO_LOCK
},
TEST_VAR_1_NAME
  };

But you can’t init this structure separately without addressing each field.
Can a brother get an override?

- Bret






-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#65602): https://edk2.groups.io/g/devel/message/65602
Mute This Topic: https://groups.io/mt/77071159/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

2020-09-24 Thread Bret Barkelew via groups.io 
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.
Or we chose to not enforce quite as many things for tests.

I’d prefer the first.

- Bret

From: Ken Taylor
Sent: Thursday, September 24, 2020 6:57 PM
To: devel@edk2.groups.io; Bret 
Barkelew
Subject: [EXTERNAL] RE: ECC: Won't somebody PLEASE think of the... test 
structures.

If the structure is a non-static local variable, most compilers will silently 
inject an intrinsic call to memcpy in function initialization.  This leads to 
an intermittent linker error.

If the compiler you use automatically supports an intrinsic memcpy in the given 
architecture or optimizes out the memcpy, it will build for you and you won’t 
know you need to link to an intrinsic support library in order to build cross 
platform.  This leads to code that builds for you, but not for me.

Regards,
-Ken.

From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Bret 
Barkelew via groups.io
Sent: Thursday, September 24, 2020 6:23 PM
To: devel@edk2.groups.io
Subject: [edk2-devel] ECC: Won't somebody PLEASE think of the... test 
structures.

ERROR - EFI coding style error
ERROR - *Error code: 5007
ERROR - *There should be no initialization of a variable as part of its 
declaration
ERROR - *file: 
//home/corthon/_uefi/edk2_qemu_ci/edk2/MdeModulePkg/Library/VariablePolicyLib/VariablePolicyUnitTest/VariablePolicyUnitTest.c
ERROR - *Line number: 333
ERROR - *Variable Name: MatchCheckPolicy

EccCheck no likey:
SIMPLE_VARIABLE_POLICY_ENTRY   ValidationPolicy = {
{
  VARIABLE_POLICY_ENTRY_REVISION,
  sizeof(VARIABLE_POLICY_ENTRY) + sizeof(TEST_VAR_1_NAME),
  sizeof(VARIABLE_POLICY_ENTRY),
  TEST_GUID_1,
  TEST_POLICY_MIN_SIZE_NULL,
  TEST_POLICY_MAX_SIZE_NULL,
  TEST_POLICY_ATTRIBUTES_NULL,
  TEST_POLICY_ATTRIBUTES_NULL,
  VARIABLE_POLICY_TYPE_NO_LOCK
},
TEST_VAR_1_NAME
  };

But you can’t init this structure separately without addressing each field.
Can a brother get an override?

- Bret





-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#65599): https://edk2.groups.io/g/devel/message/65599
Mute This Topic: https://groups.io/mt/77071159/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

2020-09-24 Thread Ken Taylor 
If the structure is a non-static local variable, most compilers will silently 
inject an intrinsic call to memcpy in function initialization.  This leads to 
an intermittent linker error.

If the compiler you use automatically supports an intrinsic memcpy in the given 
architecture or optimizes out the memcpy, it will build for you and you won't 
know you need to link to an intrinsic support library in order to build cross 
platform.  This leads to code that builds for you, but not for me.

Regards,
-Ken.

From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Bret 
Barkelew via groups.io
Sent: Thursday, September 24, 2020 6:23 PM
To: devel@edk2.groups.io
Subject: [edk2-devel] ECC: Won't somebody PLEASE think of the... test 
structures.

ERROR - EFI coding style error
ERROR - *Error code: 5007
ERROR - *There should be no initialization of a variable as part of its 
declaration
ERROR - *file: 
//home/corthon/_uefi/edk2_qemu_ci/edk2/MdeModulePkg/Library/VariablePolicyLib/VariablePolicyUnitTest/VariablePolicyUnitTest.c
ERROR - *Line number: 333
ERROR - *Variable Name: MatchCheckPolicy

EccCheck no likey:
SIMPLE_VARIABLE_POLICY_ENTRY   ValidationPolicy = {
{
  VARIABLE_POLICY_ENTRY_REVISION,
  sizeof(VARIABLE_POLICY_ENTRY) + sizeof(TEST_VAR_1_NAME),
  sizeof(VARIABLE_POLICY_ENTRY),
  TEST_GUID_1,
  TEST_POLICY_MIN_SIZE_NULL,
  TEST_POLICY_MAX_SIZE_NULL,
  TEST_POLICY_ATTRIBUTES_NULL,
  TEST_POLICY_ATTRIBUTES_NULL,
  VARIABLE_POLICY_TYPE_NO_LOCK
},
TEST_VAR_1_NAME
  };

But you can't init this structure separately without addressing each field.
Can a brother get an override?

- Bret




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#65597): https://edk2.groups.io/g/devel/message/65597
Mute This Topic: https://groups.io/mt/77071159/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-