Re: [edk2-devel] [PATCH V3 5/5] RedfishPkg: Fix compile issue on Linux

[Minh Nguyen <minhnguyen1@amperemail.onmicrosoft.com>]

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

Hi Pedro,
  I just re-sent in readable format.

Thanks,
Minh Nguyen

On 5/7/2023 12:39 PM, Minh Nguyen wrote:
> Hi Pedro,
>   Firstly, thanks for your comments, let me explain.
>     - "C99 flexible array members have been well defined and well
>       supported for maybe 20 years in GCC (https://godbolt.org/z/9qxKar4f6)"
>       =>*Yes, I agree and there's no****comment from my side*  *but this is completely different with current issue, why? Please see 
> the bellow****explanation.*
>     - When we define the PCD to map with any structure in .dec file, BaseTools will gen/*PcdValueInit.c*/.
>       In this file, there're a lot of using "/*__FIELD_SIZE(REST_EX_SERVICE_DEVICE_PATH_DATA, DevicePath);*/"
>       and this is the definiton of this macro "/*#define __FIELD_SIZE(TYPE, Field) (sizeof((TYPE *)0)->Field)*/".
>       When using "/*__STATIC_ASSERT((__FIELD_SIZE(REST_EX_SERVICE_DEVICE_PATH_DATA, 
> DevicePath) >= 3) || *//**//*(__FIELD_SIZE(REST_EX_SERVICE_DEVICE_PATH_DATA, DevicePath) == 0), 
> "Input buffer exceeds the buffer array");*/",
>        "/*(sizeof((TYPE *)0)->Field*/" will be an error (*error: invalid application of ‘sizeof’ to incomplete type **EFI_DEVICE_PATH_PROTOCOL[]*)
>       => cause error on "__STATIC_ASSERT" because _Static_assert requires its first parameter to be a constant expression => the error message when compile
>       this code is "*Line 257 Value {0x03,0x0b,0x25,0x00,0x00,0x1b,****0x21,0xdc,0x35,0xb0,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,****0x00,0x00,0x00,0x00,0x00,0x01,0x7f,0xff,0x04,0x00}:293:19: error: 
> expression in static assertion is not an integer*".

     - An other demonstation on other platforms in this behavior: You can check at*edk2-platforms/Features/Intel/SystemInformation/SmbiosFeaturePkg/SmbiosFeaturePkg.dec*  and I try it in my platform.

>   
> gAmpereTokenSpaceGuid.PcdSmbiosType0BiosInformation|{0x0}|SMBIOS_TABLE_TYPE0|0xB000000C{
>     <HeaderFiles>
> IndustryStandard/SmBios.h
>     <Packages>
>       MdePkg/MdePkg.dec
>       Silicon/Ampere/AmpereSiliconPkg/AmpereSiliconPkg.dec
>   }
>   gAmpereTokenSpaceGuid.PcdSmbiosType0BiosInformation.Vendor|0x1
>   
> gAmpereTokenSpaceGuid.PcdSmbiosType0BiosInformation.BIOSCharacteristicsExtensionBytes[0]|0x33 
>

      This is the declaration to map PcdSmbiosType0BiosInformation with SMBIOS_TABLE_TYPE0 structure and
      the declaration BIOSCharacteristicsExtensionBytes is "*UINT8 BIOSCharacteristicsExtensionBytes[2];*".
      If we remove the number of elements (2) to flexible array, we will see the same error like
      "*Index of BIOSCharacteristicsExtensionBytes[0]:361:19: error: expression 
in static assertion is not an integer."*
      
  => CONCLUSION: This is not problem of compiler, it just comes from the way BaseTools gen
/*PcdValueInit.c*/  and definition of macros. I hope above explanation satisfy your concern.

Thanks,
Minh Nguyen

> On 5/6/2023 5:57 AM, Pedro Falcato wrote:
>> On Fri, May 5, 2023 at 6:12 PM Minh Nguyen via groups.io
>> <minhnguyen1=os.amperecomputing.com@groups.io>  wrote:
>>> From: Vu Nguyen<vunguyen@os.amperecomputing.com>
>>>
>>> It requires a fixed size array to store the content of device path PCD.
>>> Add the array size to solve this issue.
>>>
>>> Signed-off-by: Minh Nguyen<minhnguyen1@os.amperecomputing.com>
>>> Cc: Abner Chang<abner.chang@amd.com>
>>> Cc: Igor Kulchytskyy<igork@ami.com>
>>> Cc: Nick Ramirez<nramirez@nvidia.com>
>>> Reviewed-by: Abner Chang<Abner.Chang@amd.com>
>>> ---
>>>   RedfishPkg/Include/Pcd/RestExServiceDevicePath.h | 5 ++++-
>>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/RedfishPkg/Include/Pcd/RestExServiceDevicePath.h b/RedfishPkg/Include/Pcd/RestExServiceDevicePath.h
>>> index 91b1198297c2..57fc199f61f2 100644
>>> --- a/RedfishPkg/Include/Pcd/RestExServiceDevicePath.h
>>> +++ b/RedfishPkg/Include/Pcd/RestExServiceDevicePath.h
>>> @@ -4,6 +4,7 @@
>>>
>>>     Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
>>>     (C) Copyright 2020 Hewlett Packard Enterprise Development LP<BR>
>>> +  Copyright (c) 2023, Ampere Computing LLC. All rights reserved.<BR>
>>>
>>>       SPDX-License-Identifier: BSD-2-Clause-Patent
>>>
>>> @@ -14,6 +15,8 @@
>>>
>>>   #include <Protocol/DevicePath.h>
>>>
>>> +#define MAX_DEVICE_PATH_NODE      40
>>> +
>>>   typedef enum {
>>>     DEVICE_PATH_MATCH_MAC_NODE = 1,
>>>     DEVICE_PATH_MATCH_PCI_NODE = 2,
>>> @@ -32,7 +35,7 @@ typedef struct {
>>>     //    0x03,0x0b,0x25,0x00,0x00,0x50,0x56,0xc0,0x00,0x02,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x01,
>>>     //    0x7f,0xff,0x04,0x00}
>>>     //
>>> -  EFI_DEVICE_PATH_PROTOCOL    DevicePath[];
>>> +  EFI_DEVICE_PATH_PROTOCOL      DevicePath[MAX_DEVICE_PATH_NODE];
>>>   } REST_EX_SERVICE_DEVICE_PATH_DATA;
>> This doesn't work (changes the meaning) and may possibly break ABI.
>>
>> What error do you get? What compiler?
>>
>> C99 flexible array members have been well defined and well supported
>> for maybe 20 years in GCC (https://godbolt.org/z/9qxKar4f6)
>>

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