Andrew, My responses embedded below.
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? 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. > > > >> > >> ## 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. > > 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. > > >> ## 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. > 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. > 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. For Dynamic or DynamicEx PCDs, individual fields can be accessed without having to read/write the entire structure. > 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? > > "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
