Re: [edk2-devel] [Patch] IntelSiliconPkg/ShadowMicrocodePei: Fix GCC build error.

["Andrew Fish" <afish@apple.com>]

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


> On Mar 1, 2020, at 3:41 PM, Michael D Kinney <michael.d.kinney@intel.com> wrote:
> 
> Andrew,
> 
> Thanks.  I did not realize this was a struct with 
> a single flexible array member.  I agree that does
> not make sense.
> 
> The top level struct here is a HOB that must be in
> contiguous memory.  The HOB can vary in size based on
> MicrocodeCount and StorageType.  We are trying to make
> it easy to write the C code that produces and consumes
> this HOB.  Using flexible array members helps improve
> the readability when structures have this attribute.
> 
> For the storage type defined, the StorageContext is a 
> single UINT64 value.  Other storage type GUIDs may have
> more complex StorageContext structures.  This array
> of structures is guaranteed to start on an 8-byte
> boundary, so the use case is to cast the start of
> the StorageContext to the defined type and access
> the array if structures and their fields.
> 
> Do you have any suggestions on how to make this
> better?


Mike,

The flexible array member can not be in a struct by its self or a member of a union (I looked into that, but clang warns on that too). It has to be a the end of a structure with size. 

If we can't build a compound structure with the preamble data as that data varies too much the best I can think of is this:

Add the known size element to enforce alignment, and make the flexible size member legal C. Add a macro that converts the pointer to raw data to the FLEXIBLE_ARRAY. Basically you need to subtract the fixed size element to get the pointer to the flexible array aligned and indexable. 

typedef struct {
  UINT64  ForceAlign;
  UINT8   Array[];
}  FLEXIBLE_ARRAY;


FLEXIBLE_ARRAY *gFlex = FLEXIBLE_ARRAY_ALIGN (RawData);

Now FLEXIBLE_ARRAY.Array[] is an index into the RawData, and the definition of the FLEXIBLE_ARRAY_ALIGN can explain the trick and why it is required or standard C. 

Then from the C code it looks more like code that requires aligning a buffer? 

Sorry that is the best I've got.  But at the end of the day a pointer is also a flexible array in C so that is the other option. 

Thanks,
 
Andrew Fish

> 
> Mike
> 
>> -----Original Message-----
>> From: afish@apple.com <mailto:afish@apple.com> <afish@apple.com <mailto:afish@apple.com>>
>> Sent: Thursday, February 27, 2020 7:43 AM
>> To: devel@edk2.groups.io <mailto:devel@edk2.groups.io>; Kinney, Michael D
>> <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>>
>> Cc: Gao, Liming <liming.gao@intel.com <mailto:liming.gao@intel.com>>; Fu, Siyuan
>> <siyuan.fu@intel.com <mailto:siyuan.fu@intel.com>>; Ni, Ray <ray.ni@intel.com <mailto:ray.ni@intel.com>>;
>> Chaganty, Rangasai V <rangasai.v.chaganty@intel.com <mailto:rangasai.v.chaganty@intel.com>>
>> Subject: Re: [edk2-devel] [Patch]
>> IntelSiliconPkg/ShadowMicrocodePei: Fix GCC build
>> error.
>> 
>> Mike,
>> 
>> Flexible array members must be the last element of a
>> struct  but they can not be the only element.
>> 
>> This is non standard behavior from the compilers that
>> are not throwing the error.
>> 
>> Why not just use a pointer?
>> 
>> Sent from my iPhone
>> 
>>> On Feb 26, 2020, at 10:03 PM, Michael D Kinney
>> <michael.d.kinney@intel.com> wrote:
>>> 
>>> Liming,
>>> 
>>> This does not make sense.  Those compilers
>>> should support C99 flexible array members.
>>> 
>>> Structured PCDs also require use of flexible
>>> array members.
>>> 
>>> We need to make sure the compiler flags for
>>> those tool chain are correct to support flexible
>>> array members.
>>> 
>>> Mike
>>> 
>>>> -----Original Message-----
>>>> From: Gao, Liming <liming.gao@intel.com>
>>>> Sent: Wednesday, February 26, 2020 9:58 PM
>>>> To: devel@edk2.groups.io; Kinney, Michael D
>>>> <michael.d.kinney@intel.com>; Fu, Siyuan
>>>> <siyuan.fu@intel.com>
>>>> Cc: Ni, Ray <ray.ni@intel.com>; Chaganty, Rangasai V
>>>> <rangasai.v.chaganty@intel.com>
>>>> Subject: RE: [edk2-devel] [Patch]
>>>> IntelSiliconPkg/ShadowMicrocodePei: Fix GCC build
>>>> error.
>>>> 
>>>> Mike:
>>>> I find this issue on GCC5 tool chain tag with GCC
>> 5.5
>>>> and CLANGPDB tool chain tag with CLANG 9.0.0
>>>> 
>>>> Thanks
>>>> Liming
>>>>> -----Original Message-----
>>>>> From: devel@edk2.groups.io <devel@edk2.groups.io>
>> On
>>>> Behalf Of Michael D Kinney
>>>>> Sent: Thursday, February 27, 2020 1:54 PM
>>>>> To: Gao, Liming <liming.gao@intel.com>;
>>>> devel@edk2.groups.io; Fu, Siyuan
>> <siyuan.fu@intel.com>;
>>>> Kinney, Michael D
>>>>> <michael.d.kinney@intel.com>
>>>>> Cc: Ni, Ray <ray.ni@intel.com>; Chaganty, Rangasai
>> V
>>>> <rangasai.v.chaganty@intel.com>
>>>>> Subject: Re: [edk2-devel] [Patch]
>>>> IntelSiliconPkg/ShadowMicrocodePei: Fix GCC build
>>>> error.
>>>>> 
>>>>> Which GCC and CLANG tool chain tags?
>>>>> 
>>>>> Flexible array member is a standard C feature
>>>>> documented in C99.
>>>>> 
>>>>> Mike
>>>>> 
>>>>>> -----Original Message-----
>>>>>> From: Gao, Liming <liming.gao@intel.com>
>>>>>> Sent: Wednesday, February 26, 2020 9:38 PM
>>>>>> To: Kinney, Michael D
>> <michael.d.kinney@intel.com>;
>>>>>> devel@edk2.groups.io; Fu, Siyuan
>>>> <siyuan.fu@intel.com>
>>>>>> Cc: Ni, Ray <ray.ni@intel.com>; Chaganty, Rangasai
>>>> V
>>>>>> <rangasai.v.chaganty@intel.com>
>>>>>> Subject: RE: [edk2-devel] [Patch]
>>>>>> IntelSiliconPkg/ShadowMicrocodePei: Fix GCC build
>>>>>> error.
>>>>>> 
>>>>>> Mike:
>>>>>> I find GCC and CLANG will report the error for
>>>> the
>>>>>> empty struct.
>>>>>> 
>>>>>> d:\allpkg\edk2-
>>>>>> 
>>>> 
>> platforms\Silicon\Intel\IntelSiliconPkg\Include\Guid/Mi
>>>>>> crocodeShadowInfoHob.h:61:11: error: flexible
>> array
>>>>>> member 'MicrocodeAddressInFlash' not allowed in
>>>>>> otherwise empty struct
>>>>>> UINT64  MicrocodeAddressInFlash[];
>>>>>>         ^
>>>>>> 1 error generated.
>>>>>> 
>>>>>> Thanks
>>>>>> Liming
>>>>>>> -----Original Message-----
>>>>>>> From: Kinney, Michael D
>>>> <michael.d.kinney@intel.com>
>>>>>>> Sent: Thursday, February 27, 2020 1:25 PM
>>>>>>> To: devel@edk2.groups.io; Fu, Siyuan
>>>>>> <siyuan.fu@intel.com>; Kinney, Michael D
>>>>>> <michael.d.kinney@intel.com>
>>>>>>> Cc: Ni, Ray <ray.ni@intel.com>; Chaganty,
>>>> Rangasai V
>>>>>> <rangasai.v.chaganty@intel.com>; Gao, Liming
>>>>>> <liming.gao@intel.com>
>>>>>>> Subject: RE: [edk2-devel] [Patch]
>>>>>> IntelSiliconPkg/ShadowMicrocodePei: Fix GCC build
>>>>>> error.
>>>>>>> 
>>>>>>> What compiler does not like the flexible array
>>>>>>> member syntax [].
>>>>>>> 
>>>>>>> Mike
>>>>>>> 
>>>>>>>> -----Original Message-----
>>>>>>>> From: devel@edk2.groups.io
>>>> <devel@edk2.groups.io>
>>>>>> On
>>>>>>>> Behalf Of Siyuan, Fu
>>>>>>>> Sent: Wednesday, February 26, 2020 5:58 PM
>>>>>>>> To: devel@edk2.groups.io
>>>>>>>> Cc: Ni, Ray <ray.ni@intel.com>; Chaganty,
>>>> Rangasai
>>>>>> V
>>>>>>>> <rangasai.v.chaganty@intel.com>; Gao, Liming
>>>>>>>> <liming.gao@intel.com>
>>>>>>>> Subject: [edk2-devel] [Patch]
>>>>>>>> IntelSiliconPkg/ShadowMicrocodePei: Fix GCC
>>>> build
>>>>>>>> error.
>>>>>>>> 
>>>>>>>> This patch fixes compiler error introduced by
>>>>>> commit
>>>>>>>> b0099a39bd.
>>>>>>>> 
>>>>>>>> BZ:
>>>>>>>> 
>>>>>> 
>>>> 
>> https://tianocore.acgmultimedia.com/show_bug.cgi?id=244
>>>>>>>> 9
>>>>>>>> Cc: Ray Ni <ray.ni@intel.com>
>>>>>>>> Cc: Rangasai V Chaganty
>>>>>> <rangasai.v.chaganty@intel.com>
>>>>>>>> Cc: Liming Gao <liming.gao@intel.com>
>>>>>>>> Signed-off-by: Siyuan Fu <siyuan.fu@intel.com>
>>>>>>>> ---
>>>>>>>> 
>>>> .../Feature/ShadowMicrocode/ShadowMicrocodePei.c
>>>>>>>> | 2 +-
>>>>>>>> 
>>>>>>>> 
>>>>>> 
>>>> 
>> .../Intel/IntelSiliconPkg/Include/Guid/MicrocodeShadowI
>>>>>>>> nfoHob.h | 2 +-
>>>>>>>> 2 files changed, 2 insertions(+), 2
>>>> deletions(-)
>>>>>>>> 
>>>>>>>> diff --git
>>>>>>>> 
>>>>>> 
>>>> 
>> a/Silicon/Intel/IntelSiliconPkg/Feature/ShadowMicrocode
>>>>>>>> /ShadowMicrocodePei.c
>>>>>>>> 
>>>>>> 
>>>> 
>> b/Silicon/Intel/IntelSiliconPkg/Feature/ShadowMicrocode
>>>>>>>> /ShadowMicrocodePei.c
>>>>>>>> index 7e4084247e..8d6574f667 100644
>>>>>>>> ---
>>>>>>>> 
>>>>>> 
>>>> 
>> a/Silicon/Intel/IntelSiliconPkg/Feature/ShadowMicrocode
>>>>>>>> /ShadowMicrocodePei.c
>>>>>>>> +++
>>>>>>>> 
>>>>>> 
>>>> 
>> b/Silicon/Intel/IntelSiliconPkg/Feature/ShadowMicrocode
>>>>>>>> /ShadowMicrocodePei.c
>>>>>>>> @@ -247,7 +247,7 @@ ShadowMicrocodePatchWorker
>>>> (
>>>>>>>>      (VOID *) Patches[Index].Address,
>>>>>>>>      Patches[Index].Size
>>>>>>>>      );
>>>>>>>> -    MicrocodeAddressInMemory[Index] = (UINT64)
>>>>>> Walker;
>>>>>>>> +    MicrocodeAddressInMemory[Index] = (UINT64)
>>>>>> (UINTN)
>>>>>>>> Walker;
>>>>>>>>    Flashcontext-
>>>>> MicrocodeAddressInFlash[Index]
>>>>>> =
>>>>>>>> (UINT64) Patches[Index].Address;
>>>>>>>>    Walker += Patches[Index].Size;
>>>>>>>>  }
>>>>>>>> diff --git
>>>>>>>> 
>>>>>> 
>>>> 
>> a/Silicon/Intel/IntelSiliconPkg/Include/Guid/MicrocodeS
>>>>>>>> hadowInfoHob.h
>>>>>>>> 
>>>>>> 
>>>> 
>> b/Silicon/Intel/IntelSiliconPkg/Include/Guid/MicrocodeS
>>>>>>>> hadowInfoHob.h
>>>>>>>> index d887b39123..1daae1234a 100644
>>>>>>>> ---
>>>>>>>> 
>>>>>> 
>>>> 
>> a/Silicon/Intel/IntelSiliconPkg/Include/Guid/MicrocodeS
>>>>>>>> hadowInfoHob.h
>>>>>>>> +++
>>>>>>>> 
>>>>>> 
>>>> 
>> b/Silicon/Intel/IntelSiliconPkg/Include/Guid/MicrocodeS
>>>>>>>> hadowInfoHob.h
>>>>>>>> @@ -58,7 +58,7 @@ typedef struct {
>>>>>>>>  // microcode patch address on flash. The
>>>> address
>>>>>> is
>>>>>>>> placed in same
>>>>>>>>  // order as the microcode patches in
>>>>>>>> MicrocodeAddrInMemory.
>>>>>>>>  //
>>>>>>>> -  UINT64  MicrocodeAddressInFlash[];
>>>>>>>> +  UINT64  MicrocodeAddressInFlash[0];
>>>>>>>> } EFI_MICROCODE_STORAGE_TYPE_FLASH_CONTEXT;
>>>>>>>> 
>>>>>>>> #endif
>>>>>>>> --
>>>>>>>> 2.19.1.windows.1
>>>>>>>> 
>>>>>>>> 
>>>>>>>> 
>>>>> 
>>>>> 
>>>>> 
>>> 
>>> 
>>> 
>>> 
> 
> 


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