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: 5069 bytes --]

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[]*_) => will 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*.
/*gSmbiosFeaturePkgTokenSpaceGuid.PcdSmbiosType0BiosInformation|{0x0}|SMBIOS_TABLE_TYPE0|0xD0000001 
{*//*
*//*    <HeaderFiles>*//*
*//*      IndustryStandard/SmBios.h*//*
*//*    <Packages>*//*
*//*      MdePkg/MdePkg.dec*//*
*//*SystemInformation/SmbiosFeaturePkg/SmbiosFeaturePkg.dec*//*
*//*  }*/
/*gSmbiosFeaturePkgTokenSpaceGuid.PcdSmbiosType0BiosInformation.BIOSCharacteristicsExtensionBytes[0]|0x33*//*
*//*gSmbiosFeaturePkgTokenSpaceGuid.PcdSmbiosType0BiosInformation.BIOSCharacteristicsExtensionBytes[1]|0x0F*/
This is the declaration to map PcdSmbiosType0BiosInformation with 
SMBIOS_TABLE_TYPE0 structure and
the declaration of 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 on compiler, it just comes from the 
way BasTools gen PcdValueInit.c
and define macros for this case (map PCD structure to any structure). I 
hope it satisfy your concerns.


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: 7403 bytes --]