> 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. > >> >> 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. >> >>>> ## 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. > >> 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. >> 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. 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|gUsbClassDevicePath 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|gUsbClassDevicePath 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|gUsbClassDevicePath 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
