> On May 20, 2016, at 10:48 AM, Kinney, Michael D <michael.d.kin...@intel.com>
> wrote:
>
> Andrew,
>
> 1) I think the strongest concern is on use of CLANG/AST. I will look at your
> ideas and
> some other ideas I have thought of as well to use standard C compiler that
> is already
> installed to build FW to support Structured PCD features.
>
> 2) The other concern I see here is extending PcdLib to access fields. You
> prefer to
> not add those APIs and always read/write entire structure. I have requests
> to support
> field access, so that is a bit of a requirement conflict.
>
Mike,
Given the complexity we ended up with the basic edk2 design, that was also
driven by complex requirements, I wanted to start the conversation of how some
of the requirements maybe driving a disproportionate amount of the complexity.
I would hate for a "nice to have" feature drive 80% of the complexity. I'm
still feeling guilty about those PCD section names....
I think there are two vectors of complexity:
1) Implementation - Requiring clang/AST
I think stepping away with the assumption of clang/AST being required helps
this vector out a lot.
2) Workflow - how a developer uses the feature day to day.
I think my comments about the default values in the .DEC relate to the
complexities a developer will face. As you point out there is going to be a
.DEC grammar extension required just to support this feature. Also if I
understand correctly changing a #define or a structure definition in another
package could change the default. We currently don't have this behavior and I
think that will lead to more confusion and hard to track down bugs. Thus I
think we should consider sticking with the current .DEC syntax for the default
variable if it is required. We could advocate comments that explain the fields
and maybe even output information like that in build log PCD section so you can
copy paste into the .DEC.
Using an INF file and new file type for the structured PCD is also trying to
manage developer complexity by making it symmetric with C and ACPI code. You
can make the argument that PCDs always worked a certain way, but I will make
the a counter argument that managing the C include file infrastructure has
always worked a certain way too, and that is the reason to consider not
building a parallel include infrastructure definition for the .DSC (and .DEC)
files and use an INF file, not to mention it is probably easier to implement
since it is a solved problem.
I was also thinking about the PcdLib field accessors. If you don't support bit
fields and don't need pedantic type safety I think this idea may get you 80% of
what you want (I'm guessing you might have already thought of this). If the
build system knows the C type of the structure and places it in the AutoGen.h
and you know the structure member name in the calling code the Code calling the
PcdLib.h can derive the structure offset and size of the field. It may also be
possible to verify the size of the argument via a sizeof() in a macro.
I just double checked that nested structures/unions would work, and clang looks
happy.
#include <stdio.h>
#define OFFSET_OF(TYPE, Field) __builtin_offsetof(TYPE, Field)
#define SIZE_OF(TYPE, Field) (sizeof(((TYPE *)0)->Field))
typedef struct {
int a;
int b;
int c;
int leaf;
} STRUCT_LEAF;
typedef struct {
STRUCT_LEAF Leaf;
int i;
} STRUCT;
int
main ()
{
STRUCT S;
printf ("Offset 0x%lx size 0x%lx\n", OFFSET_OF(STRUCT, Leaf.leaf),
SIZE_OF(STRUCT, Leaf.leaf));
return 0;
}
So maybe the complexity tradeoff is how you support bitfields (union with a
real type and granularity of access is that type) and how pedantic the type
checking (C is not type safe, so this is no different than a cast in the C
code) is going to be for the PcdLib structure field accessor.
Thanks,
Andrew Fish
> More responses below.
>
> Mike
>
>
>> -----Original Message-----
>> From: af...@apple.com [mailto:af...@apple.com]
>> Sent: Thursday, May 19, 2016 3:25 PM
>> To: Kinney, Michael D <michael.d.kin...@intel.com>
>> Cc: edk2-devel@lists.01.org <edk2-de...@ml01.01.org>
>> Subject: Re: [edk2] [RFC] Structured PCD Proposal
>>
>>
>>> On May 19, 2016, at 12:16 PM, Kinney, Michael D
>>> <michael.d.kin...@intel.com> wrote:
>>>
>>> Andrew,
>>>
>>> My responses embedded below.
>>>
>>
>> Mike,
>>
>> Likewise.
>>
>>> Mike
>>>
>>>> -----Original Message-----
>>>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>>>> Andrew Fish
>>>> Sent: Thursday, May 19, 2016 10:53 AM
>>>> To: Kinney, Michael D <michael.d.kin...@intel.com>;
>>>> edk2-devel@lists.01.org <edk2-
>>>> de...@ml01.01.org>
>>>> Subject: Re: [edk2] [RFC] Structured PCD Proposal
>>>>
>>>>
>>>>> On May 19, 2016, at 2:26 AM, Laszlo Ersek <ler...@redhat.com> wrote:
>>>>>
>>>>> On 05/19/16 01:28, Kinney, Michael D wrote:
>>>>>
>>>>> [snip]
>>>>>
>>>>>> ## C Structure Syntax Proposal
>>>>>> * Use C typedef syntax to describe complex C structures
>>>>>> * Use a C language parser with support for struct/union/array/bit
>>>>>> fields/pack
>>>>>> * Recommend use of libclang C language parser into Abstract Syntax Tree
>>>>>> (AST)
>>>>>> - Included with LLVM release
>>>>>> - http://llvm.org/releases/download.html
>>>>>> - http://clang.llvm.org/doxygen/group__CINDEX.html
>>>>>> * Recommend use of Python bindings for CLANG for EDK II Python based
>>>>>> BaseTools
>>>>>> - pip install clang
>>>>>
>>>>> What versions are necessary? On RHEL-7, the following versions are
>>>>> available (from EPEL only; RHEL does not provide clang):
>>>>>
>>>>> clang-devel: 3.4.2-8.el7
>>>>> python-pip: 7.1.0-1.el7
>>>>>
>>>>
>>>> Will it be possible to check everything into the edk2 git tree? Currently
>>>> we only
>>>> require the developer to have Xcode installed and everything else is in
>>>> the git
>> tree.
>>>> I'm not sure it is possible to run pip on our production build servers?
>>>
>>> Is clang and clang libs included in your environment?
>>
>> clang is the compiler for Xcode so we have that for sure. I'm not sure about
>> the libs
>> but they are likely accessible.
>>
>>> That is not edk2 git today.
>>> The only other component is the Python wrapper on top of the clang lib and
>>> that
>>> is available from another github project if you want to pull python sources
>>> to it.
>>>
>>
>> If is just Python code it should not be too complex?
>>
>>>>
>>>>
>>>>>>
>>>>>> ## DEC File Extensions
>>>>>> * Declare link between PCD and C data structure and token range for
>>>>>> fields
>>>>>> * @Include path to include file with C typedef [Required]
>>>>>> * Replace |VOID*| with name of C typedef
>>>>>> * @TokenMap path to file with token number assignments [Optional]
>>>>>> * Range of token numbers to assign to fields in C typedef [Required]
>>>>>> * Example:
>>>>>>
>>>>>> ```
>>>>>> # @Include Include/Pcd/AcpiLowerPowerIdleTable.h
>>>>>> # @TokenMap Include/Pcd/AcpiLowerPowerIdleTable.map
>>>>>>
>>>>
>> gPlatformTokenSpaceGuid.AcpiLowPowerIdleTable|{}|ACPI_LOW_POWER_IDLE_TABLE|0x00010080
>>>> |0x0E000200-0x0E0002FF
>>>>>> ```
>>>>>
>>>>> What does 0x00010080 mean here?
>>>>>
>>>>>>
>>>>>> * Recommended File Paths
>>>>>> - <PackageName>/Include/Pcd/<StructuredPcdName>.h
>>>>>> - <PackageName>/Include/Pcd/<StructuredPcdName>.map [Optional]
>>>>>> - <PackageName>/Include/Pcd/<StructuredPcdName>.uni [Optional]
>>>>>> * C Pre-Processor Support
>>>>>> - Use of #include, #define, #if supported
>>>>>> - #include limited to files in same package
>>>>>> - Including files from other packages being evaluated
>>>>>>
>>>>
>>>> I think you need to share the C headers with the edk2 C and ASL code, so
>>>> you
>> should
>>>> have full access including all the things set in the [BuildOptions] flags
>>>> in the
>> DSC.
>>>> So use something like PP_FLAGS or ASLPP_FLAGS and allow customization via
>>>> the
>>>> Conf/build_rule.txt. I'd rather we not makeup a new thing and just use one
>>>> of the
>>>> existing pre processor schemes.
>>>
>>> I agree. Any flags set up in [BuildOptions] or -D flags on command line
>>> would be
>>> passed in, so the .h file associated with the Structured PCD is treated the
>>> same
>>> as any other .h file used to build modules/libs. There is no intention to
>>> create
>>> any new build rules to support Structured PCDs.
>>
>> OK I was confused by this statement. "Including files from other packages
>> being
>> evaluated". The more I think about it it is probably more related to this
>> not being
>> symmetric to how C and ASL works (you have an INF file to declare your
>> dependencies).
>> I'm worried that could cause issue. What if I needed a include file from a
>> package
>> that was not included any other way? If I was writing C or ASL I'd just
>> update the
>> INF file.
>
> Today, all of the content in a DEC file of a package does not have any
> dependencies
> outside that package. Modules/library implementations within a package may
> have
> dependencies on content from other packages as defined by the [Packages]
> section
> in an INF file. A PCD, declared in a package DEC files, that is associated
> with
> a typedef struct in an include file must follow the DEC file constraint where
> there
> are no dependencies on other packages.
>
> What is being evaluated is how to support a use case where an include file
> used for
> a Structured PCD needs definitions from include files in another package
> (e.g. MdePkg for some industry standard defines/structs)
>
> We support multiple package dependencies in modules/library INF files today
> with a
> [Packages] section in the INF. One idea (not part of this proposal at this
> time),
> is to add a [Packages] section to a DEC file. The include paths used to
> process a .h file associated with a Structured PCD can be appended with all
> the
> dependent package include paths. The [Packages] section in a DEC would only
> be
> used for DEC file dependencies. It would never be used to package
> dependencies of
> modules/libraries within that package.
>
> Adding a [Packages] section to a DEC file has wider impacts to other
> BaseTools,
> which is why I did not provide any more details than "under evaluation" at
> this
> time. My suggestion with the current proposal is to make sure all the .h file
> content required for a Structured PCD be within the package itself.
>
>>
>>>
>>>>
>>>> IMHO it will be easy enough to follow the proposed restrictive rules when
>>>> you are
>>>> starting from scratch, but I'm more worried about the case when you have an
>> existing
>>>> (very complex) include infrastructure that supports feature X in C and ASL
>>>> and you
>>>> decide to add a PCD for X. You don't want it to not work due to more
>>>> restrictive
>>>> rules around the PCD.
>>>
>>> A .h file can have lots of content in it. Only the typedef structure name
>> referenced
>>> in the DEC file declaration has to follow any of the restrictions
>>> associated with
>>> a Structured PCD. The C Preprocessor statements above are there to show
>>> that there
>>> .h file for a Structured PCD can use the same C Preprocessor directives
>>> that the
>> rest
>>> of the C code in modules/libs can use. The biggest restriction is in the
>>> data
>> types
>>> of the leaf fields of the structures. Those have to be compatible with the
>>> PCD
>>> get/set APIs.
>>>
>>
>> I think this makes my point about the complexity driving restrictions. If
>> you did not
>> have leaf field accessors you would not have that restriction.
>
> I agree that removing the requirement for leaf accessors reduces complexity.
> I have
> a request to support leaf accessors, so that is why they are part of this
> proposal.
>
>>
>>>>
>>>>>> ## C Structure Syntax
>>>>>> * Support struct/union/array with nesting
>>>>>> * Support bit fields
>>>>>
>>>>> Bit fields will require new PcdLib interaces, right?
>>>>>
>>>>
>>>> Yikes bit fileds. My over all feedback is this feature seems very complex
>>>> and
>>>> designed by committee (too many features). The complexity already seems to
>>>> be
>>>> introducing restrictions and will possible make it harder for developers to
>>>> understand.
>>>
>>> The goal was to allow use of C syntax for a typedef struct with as few
>>> restrictions
>>> as possible, so exiting C structs associated with VOID* PCDs could be used
>>> "as is".
>>> Yes. Supporting full C syntax can be complex, but by using CLANG front end
>>> for
>>> parsing into an AST, the complexity is handled by a well supported parser.
>>>
>>>>
>>>> I'll take a quick stab at simplification.
>>>> 1) Add the new syntax to a new file type: *.pcd. This file would contain
>>>> [Pcd*]
>>>> sections and the C like syntax.
>>>
>>> C like syntax. That is what concerns me. Now we have to tell developers
>>> that
>>> they can use full C syntax for modules/libs, but there are different C
>>> rules for
>>> PCDs. I think it is easier is developers can use came C syntax for
>>> modules/libs
>>> and PCDs.
>>
>> I would assume the structure initialization is pure C, but there is meta data
>> associated with the PCD that needs to get collected and associated
>> initialized C
>> structure. This is going to be true for any scheme.
>
> Yes. There is optional meta data in a comment block for a field of a
> structure.
> I have used the same meta-data tags and syntax that is defined in DEC file
> comment blocks, to take advantage of the source style a developer already
> knowns
> when declaring PCDs in a DEC file. Only @TokenNumber (likely very rarely
> used) and
> @DefaultValue are new tags. But even the value syntax for these reuses the
> value
> syntax for a token number and default value in a PCD declaration in a DEC
> file.
>
>>
>>>
>>>> 2) Don't support default values in the .dec with the C like syntax.
>>>> Recommend no
>>>> defaults and a build break if a PCD is not set by the platform build for
>>>> complex
>>>> structures.
>>>
>>> Why not support defaults in scope of DEC file or associated .h file? The
>>> @DefaultValue tag in .h file is optional, so what you suggest here is
>>> supported.
>>>
>>
>> To simplify the .DEC parser so it does not require AST.
>
>
> This is pushing more work onto platform porting in DSC/FDF file. The PCD
> concept
> from the beginning included the idea that the developer that designs a new
> PCD
> and adds it to a DEC file gets to support a "preferred" default value that is
> applicable to most platforms, and the DSC/FDF files only need to provide
> override
> values if the "preferred" default does not work for that specific platform.
> Why
> should we change this fundamental PCD concept just to simplify the tools.
>
> I agree that dependency on CLANG/AST/Python bindings may not be the best
> Implementation choice based on feedback received so far, but should not mean
> burden the platform port and don't implement in better tools.
>
>>
>>>> 3) Use the C pre processor with PP_FLAGS or ASLPP_FLAGS to handle the
>>>> include
>>>> infrastructure for maximum compatibility. Just like other rules in
>>>> Conf/build_rule.txt. ?.pcd could be added to build_rule.txt.
>>>
>>> That is part of the current proposal.
>>>
>>>> 4) Don't add new PcdLib APIs. Return a pointer to the structure and use C
>>>> code to
>>>> access the data.
>>>
>>> That is supported today without this proposal.
>>>
>>> By adding new APIs, we can support access to individual fields within a
>>> Structured
>>> PCD without having to require a local variable of the full C typedef.
>>
>> I don't see that as solving a real problem. From a walk up an use standpoint
>> I
>> initialize a C structure in a file, there is an API to return that structure
>> and I
>> dereference it with a pointer. That is how you write C code in any
>> environment. Thus
>> why do we need to invent another way to do this?
>>
>>> For Dynamic
>>> or DynamicEx PCDs, individual fields can be accessed without having to
>>> read/write
>>> the entire structure.
>>
>> That could be an implementation optimization.
>>
>>>
>>>> 5) OK someone is going to complain about type checking so add a new
>>>> version of
>>>> PcdGetPtr() that casts the pointer to the C pointer type returned. The
>>>> type is in
>> the
>>>> AutoGen.h file (_PCD_GET_PTR_TYPE_##TokenName). If you don't use the new
>>>> .pcd file
>>>> syntax then you get a build break.
>>>> 6) Don't be too restrictive on type checking. Support something like a
>>>> device
>> path.
>>>> For example the globals in PlatformData.c of PlatformBdsLib could be PCD
>>>> entries.
>>>> This could be phase 2, and require a modified syntax if needed.
>>>
>>> This could be supported today using a VOID* PCD for each of this structures.
>>> Configuring through PCDs in a DSC file today as list of hex bytes is not
>>> very
>>> developer friendly. I agree that a phase 2 could help improve the developer
>>> experience configuring a VOID* buffer that is a sequence of Device Path
>>> nodes.
>>> Maybe even support the Device Path To String syntax a value syntax
>>> extension.
>>>
>>>>
>>>> This simpler scheme only requires initializing a global variable and being
>>>> able to
>>>> extract that information vs AST. The real value in this scheme is unifying
>>>> the
>>>> include infrastructure for C, ASL, and PCD. C can already access C
>>>> structure
>> members,
>>>> seems like reinventing the wheel to come up with a complex scheme to
>>>> abstract how
>> to
>>>> access C structure elements when a C pointer solves that problem.
>>>
>>> How would you initialize the global for a nested structure of many types
>>> without
>>> a parser?
>>> How would you handle unions for a corner case when multiple union
>>> members from the same union need to be initialized?
>>
>> Use the compiler we already have installed on the system as it is already
>> configured
>> to do the correct thing, and the produce and consumer are using the same
>> tools.
>>
>> How are you going to deal with something like long (it is not the same width
>> on all
>> the IA32 compilers supported by edk2)? Or even structure packing rules? The
>> default
>> output of clang in not compatible with the EFI ABI and we have compiler
>> flags and
>> triples that fix that.
>
> I believe I have resolved all of the issues you raise here using CLANG/AST.
> Type
> 'long' is not supported as a leaf field type and generated an error in the
> prototype.
> The supported leaf field types are in this proposal. I use the same
> ProcessorBind.h
> mappings for CLANG for the supported leaf field types and it all works for
> me.
> CLANG/AST is only used to parse a C structure in a .h file. It is not used
> for
> compiling code, so there are never any EFI ABI issues with this use. CLANG
> supports
> byte packing using the same #pragma pack syntax that is used in FW sources
> and the
> byte/bit offsets computed by CLANG match the byte/bit offsets from the
> compiler
> used to compile FW.
>
> As I mentioned at the top of email, I will evaluate using the current tool
> chain tag
> instead of CLANG/AST. I am confident the results will be the same for the
> concerns
> you raise here.
>
>>
>> So the syntax probably needs some work as I'm making this up as I go along
>> but you
>> could have a *.pcd file that looks like:
>>
>> #include <BdsPlatform.h>
>>
>>
>> // @ PCDTYPE [PcdsFixedAtBuild.common]
>>
>>
>> // @PCD
>> gMdeModulePkgTokenGuid|PcdUsbClassKeyboardDevicePath|EFI_DEVICE_PATH|gUsbClassDeviceP
>> ath
>> USB_CLASS_FORMAT_DEVICE_PATH gUsbClassDevicePath = {
>> {
>> {
>> MESSAGING_DEVICE_PATH,
>> MSG_USB_CLASS_DP,
>> {
>> (UINT8) (sizeof (USB_CLASS_DEVICE_PATH)),
>> (UINT8) ((sizeof (USB_CLASS_DEVICE_PATH)) >> 8)
>> }
>> },
>> 0xffff, // VendorId
>> 0xffff, // ProductId
>> CLASS_HID, // DeviceClass
>> SUBCLASS_BOOT, // DeviceSubClass
>> PROTOCOL_KEYBOARD // DeviceProtocol
>> },
>>
>> {
>> END_DEVICE_PATH_TYPE,
>> END_ENTIRE_DEVICE_PATH_SUBTYPE,
>> {
>> END_DEVICE_PATH_LENGTH,
>> 0
>> }
>> }
>> };
>>
>> You do a quick parse to extract the meta data from the file: @ PCDTYPE
>> [PcdsFixedAtBuild.common] and @PCD
>> gMdeModulePkgTokenGuid|PcdUsbClassKeyboardDevicePath|EFI_DEVICE_PATH|gUsbClassDeviceP
>> ath
>> AutoGen a C program to do what you want and append it to the .pcd file.
>> Compile and run the AutoGen code to create the output that could be as
>> simple as the
>> current PCD Pointer syntax.
>>
>> autogen code output looks like:
>> // cat *.pcd file contents.
>> // AutoGen Start.
>> #include <stdio.h>
>>
>> int main() {
>> char *c;
>> int i;
>>
>> // @ PCDTYPE [PcdsFixedAtBuild.common]
>> printf (" [PcdsFixedAtBuild.common]\n");
>> // @PCD
>> gMdeModulePkgTokenGuid|PcdUsbClassKeyboardDevicePath|EFI_DEVICE_PATH|gUsbClassDeviceP
>> ath
>> printf
>> ("gMdeModulePkgTokenGuid|PcdUsbClassKeyboardDevicePath|EFI_DEVICE_PATH|{");
>> for (i=0, c = (char *)(unsigned long long) &gUsbClassDevicePath; i < sizeof
>> (gUsbClassDevicePath); i++) {
>> printf (" 0x%02x,", c[i]);
>> }
>> printf (" }\n");
>>
>> return 0;
>> }
>>
>> Thus basically use the same compiler on both sides, and some slightly
>> modified form
>> of the current PCD syntax (add the type). You could actually do this today
>> with an
>> INF file and a custom rule in the Conf/build_rule.txt. The only modification
>> to the
>> build system would adding the output files to the PCD database creation.
>> There are
>> other ways to do it but I'm a fan of making it work like C and ASL, as I
>> think the
>> symmetry is intuitive for the developer, and you don't need to learn a new
>> way to
>> specify packages, BuildOptions etc. as you are using the existing INF syntax
>> to build
>> and process for all file types.
>>
>> Another option is to abstract all the meta data in a C macro and follow the
>> macro
>> with the = {}; data. That is what we did, basically 1st pass the macro
>> expands to the
>> global. 2nd pass the macro expands to the code to write out the date. You
>> still have
>> to strip to bits you don't want before you compile the program (you don't
>> want 2
>> copies of all the preprocessed include stuff etc.
>>
>> So my point is you can get an 80% solution that is rock solid with zero
>> constraints
>> without going down the AST route, have it be a lot simpler, and not require
>> a bunch
>> of extra tools to be installed. I'm also a fan of using a new file type and
>> an INF
>> file vs. coming up with yet another edk2 unique syntax to do something the
>> edk2
>> already has a syntax to support.
>>
>> Ironically we asked the clang developers (that wrote the AST stuff) what was
>> the
>> safest way to do something like this and they recommended using the compiler
>> in this
>> fashion of my example. Our example was slightly more complex as we
>> constructed an
>> entire database, but close enough (no issue with the packing between
>> structures in
>> the PCD case).
>>
>>
>> Thanks,
>>
>> Andrew Fish
>>
>>>
>>>>
>>>> "The real art is knowing what to leave out, not what to put in" quote from
>>>> Steve
>>>> Jobs.
>>>>
>>>> Thanks,
>>>>
>>>> Andrew Fish
>>>>
>>>> _______________________________________________
>>>> edk2-devel mailing list
>>>> edk2-devel@lists.01.org
>>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>> _______________________________________________
>>> edk2-devel mailing list
>>> edk2-devel@lists.01.org
>>> https://lists.01.org/mailman/listinfo/edk2-devel
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
