ECC: Won't somebody PLEASE think of the... test structures.

ECC: Won't somebody PLEASE think of the... test structures.

[Bret Barkelew]

[-- Attachment #1: Type: text/plain, Size: 932 bytes --]

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


[-- Attachment #2: Type: text/html, Size: 3086 bytes --]

Re: ECC: Won't somebody PLEASE think of the... test structures.

[Ken Taylor]

[-- Attachment #1: Type: text/plain, Size: 1702 bytes --]

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



[-- Attachment #2: Type: text/html, Size: 6205 bytes --]

Re: ECC: Won't somebody PLEASE think of the... test structures.

[Bret Barkelew]

[-- Attachment #1.1: Type: text/plain, Size: 2359 bytes --]

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_Taylor@phoenix.com>
Sent: Thursday, September 24, 2020 6:57 PM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Bret Barkelew<mailto:Bret.Barkelew@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




[-- Attachment #1.2: Type: text/html, Size: 6802 bytes --]

[-- Attachment #2: 95B370A7BEED49AAB797022E8F260C8F.png --]
[-- Type: image/png, Size: 151 bytes --]

Re: ECC: Won't somebody PLEASE think of the... test structures.

[Bret Barkelew]

[-- Attachment #1.1: Type: text/plain, Size: 3474 bytes --]

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.com@groups.io>
Sent: Thursday, September 24, 2020 7:26 PM
To: Ken Taylor<mailto:Ken_Taylor@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_Taylor@phoenix.com>
Sent: Thursday, September 24, 2020 6:57 PM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Bret Barkelew<mailto:Bret.Barkelew@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





[-- Attachment #1.2: Type: text/html, Size: 11062 bytes --]

[-- Attachment #2: 1AD9288314A1415D86C3547B724C354A.png --]
[-- Type: image/png, Size: 140 bytes --]

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

[Andrew Fish]

[-- Attachment #1: Type: text/plain, Size: 3505 bytes --]

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 <bret.barkelew=microsoft.com@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_Taylor@phoenix.com>
> Sent: Thursday, September 24, 2020 6:57 PM
> To: devel@edk2.groups.io <mailto:devel@edk2.groups.io>; Bret Barkelew <mailto:Bret.Barkelew@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 <mailto:devel@edk2.groups.io>] On Behalf Of Bret Barkelew via groups.io <http://groups.io/>
> 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>


[-- Attachment #2: Type: text/html, Size: 12724 bytes --]

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

[Bret Barkelew]

[-- Attachment #1.1: Type: text/plain, Size: 4876 bytes --]

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:afish@apple.com>
Sent: Thursday, September 24, 2020 7:48 PM
To: edk2-devel-groups-io<mailto:devel@edk2.groups.io>; Bret Barkelew<mailto:Bret.Barkelew@microsoft.com>
Cc: Ken Taylor<mailto:Ken_Taylor@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> <bret.barkelew=microsoft.com@groups.io<mailto:bret.barkelew=microsoft.com@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_Taylor@phoenix.com>
Sent: Thursday, September 24, 2020 6:57 PM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Bret Barkelew<mailto:Bret.Barkelew@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>



[-- Attachment #1.2: Type: text/html, Size: 12911 bytes --]

[-- Attachment #2: 956D9B34BE2C49FA8D0841584A830F55.png --]
[-- Type: image/png, Size: 139 bytes --]

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

[Andrew Fish]

[-- Attachment #1: Type: text/plain, Size: 5878 bytes --]

Bret,

Details aside this ECC issue got me thinking it might be useful to some how tag code that follows different, subsetted, or alternate rules, or uses different build environments. I was kind of thinking of doing something like how we tag the licenses in the header comments [1]. I would say nothing means edk2 rules. I can see vendors maybe having different internal rules, or us wanting to distinguish test code from production code. The general idea if we start this tools can be smarter and that seems like a good thing. 

 This is just a wild idea, so I’d like to see what other people think?

[1] SPDX-License-Identifier: BSD-2-Clause-Patent

Thanks,

Andrew Fish

> On Sep 24, 2020, at 7:57 PM, Bret Barkelew <Bret.Barkelew@microsoft.com> wrote:
> 
> 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:afish@apple.com>
> Sent: Thursday, September 24, 2020 7:48 PM
> To: edk2-devel-groups-io <mailto:devel@edk2.groups.io>; Bret Barkelew <mailto:Bret.Barkelew@microsoft.com>
> Cc: Ken Taylor <mailto:Ken_Taylor@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> <bret.barkelew=microsoft.com@groups.io <mailto:bret.barkelew=microsoft.com@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_Taylor@phoenix.com>
> Sent: Thursday, September 24, 2020 6:57 PM
> To: devel@edk2.groups.io <mailto:devel@edk2.groups.io>; Bret Barkelew <mailto:Bret.Barkelew@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 <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>
>
>


[-- Attachment #2: Type: text/html, Size: 21024 bytes --]

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

[Bret Barkelew]

[-- Attachment #1: Type: text/plain, Size: 6231 bytes --]

I’m not opposed to it, but I think our team would suggest that it’s a good use of a multi-repo solution where each repo can have its own enforced style.

What would your thoughts be on that?

- Bret

From: Andrew Fish<mailto:afish@apple.com>
Sent: Friday, September 25, 2020 7:52 PM
To: Bret Barkelew<mailto:Bret.Barkelew@microsoft.com>
Cc: edk2-devel-groups-io<mailto:devel@edk2.groups.io>; Ken Taylor<mailto:Ken_Taylor@phoenix.com>
Subject: Re: [EXTERNAL] [edk2-devel] ECC: Won't somebody PLEASE think of the... test structures.

Bret,

Details aside this ECC issue got me thinking it might be useful to some how tag code that follows different, subsetted, or alternate rules, or uses different build environments. I was kind of thinking of doing something like how we tag the licenses in the header comments [1]. I would say nothing means edk2 rules. I can see vendors maybe having different internal rules, or us wanting to distinguish test code from production code. The general idea if we start this tools can be smarter and that seems like a good thing.

 This is just a wild idea, so I’d like to see what other people think?

[1] SPDX-License-Identifier: BSD-2-Clause-Patent

Thanks,

Andrew Fish


On Sep 24, 2020, at 7:57 PM, Bret Barkelew <Bret.Barkelew@microsoft.com<mailto:Bret.Barkelew@microsoft.com>> wrote:

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:afish@apple.com>
Sent: Thursday, September 24, 2020 7:48 PM
To: edk2-devel-groups-io<mailto:devel@edk2.groups.io>; Bret Barkelew<mailto:Bret.Barkelew@microsoft.com>
Cc: Ken Taylor<mailto:Ken_Taylor@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%7Cf0f32ca53bd744c42f2308d861c7422f%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637366855730968867&sdata=xEsoTWc5tpwrKQP7s24kKRKKmR768vzm3vphI7GGJQA%3D&reserved=0> <bret.barkelew=microsoft.com@groups.io<mailto:bret.barkelew=microsoft.com@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_Taylor@phoenix.com>
Sent: Thursday, September 24, 2020 6:57 PM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Bret Barkelew<mailto:Bret.Barkelew@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%7Cf0f32ca53bd744c42f2308d861c7422f%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637366855730978863&sdata=mGjZ1q6XdEvg9JFgjHw2v%2BgFx%2BywT%2FXuQjfy2IQDBLE%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>





[-- Attachment #2: Type: text/html, Size: 17248 bytes --]

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

[Laszlo Ersek]

On 09/26/20 04:52, Andrew Fish via groups.io wrote:
> Bret,
> 
> Details aside this ECC issue got me thinking it might be useful to some how tag code that follows different, subsetted, or alternate rules, or uses different build environments. I was kind of thinking of doing something like how we tag the licenses in the header comments [1]. I would say nothing means edk2 rules. I can see vendors maybe having different internal rules, or us wanting to distinguish test code from production code. The general idea if we start this tools can be smarter and that seems like a good thing. 
> 
>  This is just a wild idea, so I’d like to see what other people think?

Concerning header files that help edk2 interface with other systems --
although it's more work, I prefer re-coding those header files manually
(even better: coding them afresh from published specs!), over importing
(otherwise license-compatible) headers from other projects. In my
experience, this sustains a very "native" look & feel to edk2, plus it's
an awesome incentive for programmers to add the structs and macros that
are *really* needed for the upcoming drivers / applications.

Thanks!
Laszlo

Re: [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