[PATCH] BaseTools/tools_def: use separate PP definition for DTC

[PATCH] BaseTools/tools_def: use separate PP definition for DTC

[Ard Biesheuvel]

Clang's preprocessor behaves differently from GCC's, and produces
intermediate device tree source that still contains #pragma pack()
and other directives that the device tree compiler chokes on.

For assembling device tree sources, it matters very little which
preprocessor is being used, so let's just use GNU CPP explicitly.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
We may need to revisit this in the future, but at present, it is highly
unlikely that someone has DTC installed but not GNU CPP, so for the time
being, this should be a sufficient fix.

 BaseTools/Conf/build_rule.template |  2 +-
 BaseTools/Conf/tools_def.template  | 10 +++-------
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/BaseTools/Conf/build_rule.template b/BaseTools/Conf/build_rule.template
index 77ed282e0311..92482341ab11 100755
--- a/BaseTools/Conf/build_rule.template
+++ b/BaseTools/Conf/build_rule.template
@@ -278,7 +278,7 @@
         $(OUTPUT_DIR)(+)${s_dir}(+)${s_base}.dtb
 
     <Command.GCC>
-        "$(PP)" $(DTCPP_FLAGS) $(INC) ${src} > ${d_path}(+)${s_base}.i
+        "$(DTCPP)" $(DTCPP_FLAGS) $(INC) ${src} > ${d_path}(+)${s_base}.i
         "$(DTC)" $(DTC_FLAGS) -I dts -O dtb -o ${dst} ${d_path}(+)${s_base}.i
 
 [Visual-Form-Representation-File]
diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template
index 427ad60b0e26..a5463797c330 100755
--- a/BaseTools/Conf/tools_def.template
+++ b/BaseTools/Conf/tools_def.template
@@ -237,6 +237,7 @@ DEFINE IPHONE_TOOLS            = /Developer/Platforms/iPhoneOS.platform/Develope
 
 DEFINE SOURCERY_CYGWIN_TOOLS = /cygdrive/c/Program Files/CodeSourcery/Sourcery G++ Lite/bin
 
+DEFINE DTCPP_BIN               = ENV(DTCPP_PREFIX)cpp
 DEFINE DTC_BIN                 = ENV(DTC_PREFIX)dtc
 
 ####################################################################################
@@ -4499,6 +4500,8 @@ DEBUG_*_*_OBJCOPY_ADDDEBUGFLAG     = --add-gnu-debuglink=$(DEBUG_DIR)/$(MODULE_N
 RELEASE_*_*_OBJCOPY_ADDDEBUGFLAG   =
 NOOPT_*_*_OBJCOPY_ADDDEBUGFLAG     = --add-gnu-debuglink=$(DEBUG_DIR)/$(MODULE_NAME).debug
 *_*_*_DTC_FLAGS                    = -H epapr
+*_*_*_DTCPP_PATH                   = DEF(DTCPP_BIN)
+*_*_*_DTC_PATH                     = DEF(DTC_BIN)
 
 DEFINE GCC_ALL_CC_FLAGS            = -g -Os -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -include AutoGen.h -fno-common
 DEFINE GCC_IA32_CC_FLAGS           = DEF(GCC_ALL_CC_FLAGS) -m32 -malign-double -freorder-blocks -freorder-blocks-and-partition -O2 -mno-stack-arg-probe
@@ -4912,7 +4915,6 @@ RELEASE_GCC45_X64_CC_FLAGS       = DEF(GCC45_X64_CC_FLAGS) -Os
 *_GCC46_*_MAKE_PATH                    = DEF(GCC46_IA32_PREFIX)make
 *_GCC46_*_*_DLL                        = ENV(GCC46_DLL)
 *_GCC46_*_ASL_PATH                     = DEF(UNIX_IASL_BIN)
-*_GCC46_*_DTC_PATH                     = DEF(DTC_BIN)
 
 *_GCC46_*_PP_FLAGS                     = DEF(GCC_PP_FLAGS)
 *_GCC46_*_ASLPP_FLAGS                  = DEF(GCC_ASLPP_FLAGS)
@@ -5023,7 +5025,6 @@ RELEASE_GCC46_ARM_CC_FLAGS       = DEF(GCC46_ARM_CC_FLAGS) -Wno-unused-but-set-v
 *_GCC47_*_MAKE_PATH                    = DEF(GCC47_IA32_PREFIX)make
 *_GCC47_*_*_DLL                        = ENV(GCC47_DLL)
 *_GCC47_*_ASL_PATH                     = DEF(UNIX_IASL_BIN)
-*_GCC47_*_DTC_PATH                     = DEF(DTC_BIN)
 
 *_GCC47_*_PP_FLAGS                     = DEF(GCC_PP_FLAGS)
 *_GCC47_*_ASLPP_FLAGS                  = DEF(GCC_ASLPP_FLAGS)
@@ -5163,7 +5164,6 @@ RELEASE_GCC47_AARCH64_CC_FLAGS   = DEF(GCC47_AARCH64_CC_FLAGS) -Wno-unused-but-s
 *_GCC48_*_MAKE_PATH                    = DEF(GCC48_IA32_PREFIX)make
 *_GCC48_*_*_DLL                        = ENV(GCC48_DLL)
 *_GCC48_*_ASL_PATH                     = DEF(UNIX_IASL_BIN)
-*_GCC48_*_DTC_PATH                     = DEF(DTC_BIN)
 
 *_GCC48_*_PP_FLAGS                     = DEF(GCC_PP_FLAGS)
 *_GCC48_*_ASLPP_FLAGS                  = DEF(GCC_ASLPP_FLAGS)
@@ -5303,7 +5303,6 @@ RELEASE_GCC48_AARCH64_CC_FLAGS   = DEF(GCC48_AARCH64_CC_FLAGS) -Wno-unused-but-s
 *_GCC49_*_MAKE_PATH                    = DEF(GCC49_IA32_PREFIX)make
 *_GCC49_*_*_DLL                        = ENV(GCC49_DLL)
 *_GCC49_*_ASL_PATH                     = DEF(UNIX_IASL_BIN)
-*_GCC49_*_DTC_PATH                     = DEF(DTC_BIN)
 
 *_GCC49_*_PP_FLAGS                     = DEF(GCC_PP_FLAGS)
 *_GCC49_*_ASLPP_FLAGS                  = DEF(GCC_ASLPP_FLAGS)
@@ -5449,7 +5448,6 @@ RELEASE_GCC49_AARCH64_DLINK_FLAGS  = DEF(GCC49_AARCH64_DLINK_FLAGS)
 *_GCC5_*_MAKE_PATH               = DEF(GCC5_IA32_PREFIX)make
 *_GCC5_*_*_DLL                   = ENV(GCC5_DLL)
 *_GCC5_*_ASL_PATH                = DEF(UNIX_IASL_BIN)
-*_GCC5_*_DTC_PATH                = DEF(DTC_BIN)
 
 *_GCC5_*_PP_FLAGS                = DEF(GCC_PP_FLAGS)
 *_GCC5_*_ASLPP_FLAGS             = DEF(GCC_ASLPP_FLAGS)
@@ -5606,7 +5604,6 @@ RELEASE_GCC5_AARCH64_DLINK_FLAGS = DEF(GCC5_AARCH64_DLINK_FLAGS) -flto -Os -L$(W
 *_CLANG35_*_MAKE_PATH            = make
 *_CLANG35_*_*_DLL                = ENV(CLANG35_DLL)
 *_CLANG35_*_ASL_PATH             = DEF(UNIX_IASL_BIN)
-*_CLANG35_*_DTC_PATH             = DEF(DTC_BIN)
 
 *_CLANG35_*_PP_FLAGS             = DEF(GCC_PP_FLAGS)
 *_CLANG35_*_ASLCC_FLAGS          = DEF(GCC_ASLCC_FLAGS)
@@ -5684,7 +5681,6 @@ RELEASE_CLANG35_AARCH64_CC_FLAGS = DEF(CLANG35_AARCH64_CC_FLAGS) $(ARCHCC_FLAGS)
 *_CLANG38_*_MAKE_PATH               = make
 *_CLANG38_*_*_DLL                   = ENV(CLANG38_DLL)
 *_CLANG38_*_ASL_PATH                = DEF(UNIX_IASL_BIN)
-*_CLANG38_*_DTC_PATH                = DEF(DTC_BIN)
 
 *_CLANG38_*_APP_FLAGS               =
 *_CLANG38_*_ASL_FLAGS               = DEF(IASL_FLAGS)
-- 
2.11.0

Re: [PATCH] BaseTools/tools_def: use separate PP definition for DTC

[Leif Lindholm]

On Tue, Feb 27, 2018 at 05:51:32PM +0000, Ard Biesheuvel wrote:
> Clang's preprocessor behaves differently from GCC's, and produces
> intermediate device tree source that still contains #pragma pack()
> and other directives that the device tree compiler chokes on.
> 
> For assembling device tree sources, it matters very little which
> preprocessor is being used, so let's just use GNU CPP explicitly.

Ah, right, same fundamental issue as
5b8766bb92debfa7b2f45a4a6d683b4227360d66.

However, this time triggered by autogen seemingly forcing inclusion of
lots of central header files that are not even used, like
MdePkg/Include/IndustryStandard/Bluetooth.h
MdePkg/Include/IndustryStandard/Acpi10.h
and so on.

Is there any way to suppress these implicit includes from .dts
processing?

/
    Leif

> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> ---
> We may need to revisit this in the future, but at present, it is highly
> unlikely that someone has DTC installed but not GNU CPP, so for the time
> being, this should be a sufficient fix.
> 
>  BaseTools/Conf/build_rule.template |  2 +-
>  BaseTools/Conf/tools_def.template  | 10 +++-------
>  2 files changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/BaseTools/Conf/build_rule.template b/BaseTools/Conf/build_rule.template
> index 77ed282e0311..92482341ab11 100755
> --- a/BaseTools/Conf/build_rule.template
> +++ b/BaseTools/Conf/build_rule.template
> @@ -278,7 +278,7 @@
>          $(OUTPUT_DIR)(+)${s_dir}(+)${s_base}.dtb
>  
>      <Command.GCC>
> -        "$(PP)" $(DTCPP_FLAGS) $(INC) ${src} > ${d_path}(+)${s_base}.i
> +        "$(DTCPP)" $(DTCPP_FLAGS) $(INC) ${src} > ${d_path}(+)${s_base}.i
>          "$(DTC)" $(DTC_FLAGS) -I dts -O dtb -o ${dst} ${d_path}(+)${s_base}.i
>  
>  [Visual-Form-Representation-File]
> diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template
> index 427ad60b0e26..a5463797c330 100755
> --- a/BaseTools/Conf/tools_def.template
> +++ b/BaseTools/Conf/tools_def.template
> @@ -237,6 +237,7 @@ DEFINE IPHONE_TOOLS            = /Developer/Platforms/iPhoneOS.platform/Develope
>  
>  DEFINE SOURCERY_CYGWIN_TOOLS = /cygdrive/c/Program Files/CodeSourcery/Sourcery G++ Lite/bin
>  
> +DEFINE DTCPP_BIN               = ENV(DTCPP_PREFIX)cpp
>  DEFINE DTC_BIN                 = ENV(DTC_PREFIX)dtc
>  
>  ####################################################################################
> @@ -4499,6 +4500,8 @@ DEBUG_*_*_OBJCOPY_ADDDEBUGFLAG     = --add-gnu-debuglink=$(DEBUG_DIR)/$(MODULE_N
>  RELEASE_*_*_OBJCOPY_ADDDEBUGFLAG   =
>  NOOPT_*_*_OBJCOPY_ADDDEBUGFLAG     = --add-gnu-debuglink=$(DEBUG_DIR)/$(MODULE_NAME).debug
>  *_*_*_DTC_FLAGS                    = -H epapr
> +*_*_*_DTCPP_PATH                   = DEF(DTCPP_BIN)
> +*_*_*_DTC_PATH                     = DEF(DTC_BIN)
>  
>  DEFINE GCC_ALL_CC_FLAGS            = -g -Os -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -include AutoGen.h -fno-common
>  DEFINE GCC_IA32_CC_FLAGS           = DEF(GCC_ALL_CC_FLAGS) -m32 -malign-double -freorder-blocks -freorder-blocks-and-partition -O2 -mno-stack-arg-probe
> @@ -4912,7 +4915,6 @@ RELEASE_GCC45_X64_CC_FLAGS       = DEF(GCC45_X64_CC_FLAGS) -Os
>  *_GCC46_*_MAKE_PATH                    = DEF(GCC46_IA32_PREFIX)make
>  *_GCC46_*_*_DLL                        = ENV(GCC46_DLL)
>  *_GCC46_*_ASL_PATH                     = DEF(UNIX_IASL_BIN)
> -*_GCC46_*_DTC_PATH                     = DEF(DTC_BIN)
>  
>  *_GCC46_*_PP_FLAGS                     = DEF(GCC_PP_FLAGS)
>  *_GCC46_*_ASLPP_FLAGS                  = DEF(GCC_ASLPP_FLAGS)
> @@ -5023,7 +5025,6 @@ RELEASE_GCC46_ARM_CC_FLAGS       = DEF(GCC46_ARM_CC_FLAGS) -Wno-unused-but-set-v
>  *_GCC47_*_MAKE_PATH                    = DEF(GCC47_IA32_PREFIX)make
>  *_GCC47_*_*_DLL                        = ENV(GCC47_DLL)
>  *_GCC47_*_ASL_PATH                     = DEF(UNIX_IASL_BIN)
> -*_GCC47_*_DTC_PATH                     = DEF(DTC_BIN)
>  
>  *_GCC47_*_PP_FLAGS                     = DEF(GCC_PP_FLAGS)
>  *_GCC47_*_ASLPP_FLAGS                  = DEF(GCC_ASLPP_FLAGS)
> @@ -5163,7 +5164,6 @@ RELEASE_GCC47_AARCH64_CC_FLAGS   = DEF(GCC47_AARCH64_CC_FLAGS) -Wno-unused-but-s
>  *_GCC48_*_MAKE_PATH                    = DEF(GCC48_IA32_PREFIX)make
>  *_GCC48_*_*_DLL                        = ENV(GCC48_DLL)
>  *_GCC48_*_ASL_PATH                     = DEF(UNIX_IASL_BIN)
> -*_GCC48_*_DTC_PATH                     = DEF(DTC_BIN)
>  
>  *_GCC48_*_PP_FLAGS                     = DEF(GCC_PP_FLAGS)
>  *_GCC48_*_ASLPP_FLAGS                  = DEF(GCC_ASLPP_FLAGS)
> @@ -5303,7 +5303,6 @@ RELEASE_GCC48_AARCH64_CC_FLAGS   = DEF(GCC48_AARCH64_CC_FLAGS) -Wno-unused-but-s
>  *_GCC49_*_MAKE_PATH                    = DEF(GCC49_IA32_PREFIX)make
>  *_GCC49_*_*_DLL                        = ENV(GCC49_DLL)
>  *_GCC49_*_ASL_PATH                     = DEF(UNIX_IASL_BIN)
> -*_GCC49_*_DTC_PATH                     = DEF(DTC_BIN)
>  
>  *_GCC49_*_PP_FLAGS                     = DEF(GCC_PP_FLAGS)
>  *_GCC49_*_ASLPP_FLAGS                  = DEF(GCC_ASLPP_FLAGS)
> @@ -5449,7 +5448,6 @@ RELEASE_GCC49_AARCH64_DLINK_FLAGS  = DEF(GCC49_AARCH64_DLINK_FLAGS)
>  *_GCC5_*_MAKE_PATH               = DEF(GCC5_IA32_PREFIX)make
>  *_GCC5_*_*_DLL                   = ENV(GCC5_DLL)
>  *_GCC5_*_ASL_PATH                = DEF(UNIX_IASL_BIN)
> -*_GCC5_*_DTC_PATH                = DEF(DTC_BIN)
>  
>  *_GCC5_*_PP_FLAGS                = DEF(GCC_PP_FLAGS)
>  *_GCC5_*_ASLPP_FLAGS             = DEF(GCC_ASLPP_FLAGS)
> @@ -5606,7 +5604,6 @@ RELEASE_GCC5_AARCH64_DLINK_FLAGS = DEF(GCC5_AARCH64_DLINK_FLAGS) -flto -Os -L$(W
>  *_CLANG35_*_MAKE_PATH            = make
>  *_CLANG35_*_*_DLL                = ENV(CLANG35_DLL)
>  *_CLANG35_*_ASL_PATH             = DEF(UNIX_IASL_BIN)
> -*_CLANG35_*_DTC_PATH             = DEF(DTC_BIN)
>  
>  *_CLANG35_*_PP_FLAGS             = DEF(GCC_PP_FLAGS)
>  *_CLANG35_*_ASLCC_FLAGS          = DEF(GCC_ASLCC_FLAGS)
> @@ -5684,7 +5681,6 @@ RELEASE_CLANG35_AARCH64_CC_FLAGS = DEF(CLANG35_AARCH64_CC_FLAGS) $(ARCHCC_FLAGS)
>  *_CLANG38_*_MAKE_PATH               = make
>  *_CLANG38_*_*_DLL                   = ENV(CLANG38_DLL)
>  *_CLANG38_*_ASL_PATH                = DEF(UNIX_IASL_BIN)
> -*_CLANG38_*_DTC_PATH                = DEF(DTC_BIN)
>  
>  *_CLANG38_*_APP_FLAGS               =
>  *_CLANG38_*_ASL_FLAGS               = DEF(IASL_FLAGS)
> -- 
> 2.11.0
> 

Re: [PATCH] BaseTools/tools_def: use separate PP definition for DTC

[Ard Biesheuvel]

On 27 February 2018 at 18:33, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Tue, Feb 27, 2018 at 05:51:32PM +0000, Ard Biesheuvel wrote:
>> Clang's preprocessor behaves differently from GCC's, and produces
>> intermediate device tree source that still contains #pragma pack()
>> and other directives that the device tree compiler chokes on.
>>
>> For assembling device tree sources, it matters very little which
>> preprocessor is being used, so let's just use GNU CPP explicitly.
>
> Ah, right, same fundamental issue as
> 5b8766bb92debfa7b2f45a4a6d683b4227360d66.
>

Yes, and I fail to see why changing just those two files makes a
meaningful difference.

> However, this time triggered by autogen seemingly forcing inclusion of
> lots of central header files that are not even used, like
> MdePkg/Include/IndustryStandard/Bluetooth.h
> MdePkg/Include/IndustryStandard/Acpi10.h
> and so on.
>
> Is there any way to suppress these implicit includes from .dts
> processing?
>

There is 'Trim', which can filter #include'd content, but using that
also robs us of the ability to #include .dtsi files, which was kind of
the point of using the preprocessor in the first place.

Re: [PATCH] BaseTools/tools_def: use separate PP definition for DTC

[Leif Lindholm]

On Tue, Feb 27, 2018 at 06:36:08PM +0000, Ard Biesheuvel wrote:
> On 27 February 2018 at 18:33, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > On Tue, Feb 27, 2018 at 05:51:32PM +0000, Ard Biesheuvel wrote:
> >> Clang's preprocessor behaves differently from GCC's, and produces
> >> intermediate device tree source that still contains #pragma pack()
> >> and other directives that the device tree compiler chokes on.
> >>
> >> For assembling device tree sources, it matters very little which
> >> preprocessor is being used, so let's just use GNU CPP explicitly.
> >
> > Ah, right, same fundamental issue as
> > 5b8766bb92debfa7b2f45a4a6d683b4227360d66.
> 
> Yes, and I fail to see why changing just those two files makes a
> meaningful difference.

Probably because Secure96Dxe.inf has a lot more [Packages] and
[LibraryClasses] dependencies than DeveloperBox.inf.

This patch would undoubtedly also have resolved that issue.
(As would clang preprocessor not throwing out C syntax when explicitly
asked to provide asm.)

> > However, this time triggered by autogen seemingly forcing inclusion of
> > lots of central header files that are not even used, like
> > MdePkg/Include/IndustryStandard/Bluetooth.h
> > MdePkg/Include/IndustryStandard/Acpi10.h
> > and so on.
> >
> > Is there any way to suppress these implicit includes from .dts
> > processing?
> 
> There is 'Trim', which can filter #include'd content, but using that
> also robs us of the ability to #include .dtsi files, which was kind of
> the point of using the preprocessor in the first place.

Yeah, that'd definitely be missing the point slightly.

I was thinking more along the lines of either excluding the autogen
bits from the preprocessing or using a separate .inf for the .dts.

If neither of those feels practical, the proposed patch is fine.
It just feels a little bit clunky.

/
    Leif

Re: [PATCH] BaseTools/tools_def: use separate PP definition for DTC

[Ard Biesheuvel]

On 27 February 2018 at 18:58, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Tue, Feb 27, 2018 at 06:36:08PM +0000, Ard Biesheuvel wrote:
>> On 27 February 2018 at 18:33, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>> > On Tue, Feb 27, 2018 at 05:51:32PM +0000, Ard Biesheuvel wrote:
>> >> Clang's preprocessor behaves differently from GCC's, and produces
>> >> intermediate device tree source that still contains #pragma pack()
>> >> and other directives that the device tree compiler chokes on.
>> >>
>> >> For assembling device tree sources, it matters very little which
>> >> preprocessor is being used, so let's just use GNU CPP explicitly.
>> >
>> > Ah, right, same fundamental issue as
>> > 5b8766bb92debfa7b2f45a4a6d683b4227360d66.
>>
>> Yes, and I fail to see why changing just those two files makes a
>> meaningful difference.
>
> Probably because Secure96Dxe.inf has a lot more [Packages] and
> [LibraryClasses] dependencies than DeveloperBox.inf.
>
> This patch would undoubtedly also have resolved that issue.
> (As would clang preprocessor not throwing out C syntax when explicitly
> asked to provide asm.)
>
>> > However, this time triggered by autogen seemingly forcing inclusion of
>> > lots of central header files that are not even used, like
>> > MdePkg/Include/IndustryStandard/Bluetooth.h
>> > MdePkg/Include/IndustryStandard/Acpi10.h
>> > and so on.
>> >
>> > Is there any way to suppress these implicit includes from .dts
>> > processing?
>>
>> There is 'Trim', which can filter #include'd content, but using that
>> also robs us of the ability to #include .dtsi files, which was kind of
>> the point of using the preprocessor in the first place.
>
> Yeah, that'd definitely be missing the point slightly.
>
> I was thinking more along the lines of either excluding the autogen
> bits from the preprocessing or using a separate .inf for the .dts.
>

We need the AutoGen.h bits for the fixed PCD values, which was the
other reason for using the preprocessor in the first place.

Separate .inf would mean registering file GUIDs in the package .DEC,
so that the driver can find the binary image at runtime, which is what
I was trying to get away from by pulling it into the driver,
especially because we are dealing with overlay DT snippets, of which
there may be many, rather than a single DT (with a well known file
GUID) for the entire platform.

> If neither of those feels practical, the proposed patch is fine.
> It just feels a little bit clunky.
>

Yeah, I don't see any unclunky solutions, unfortunately.

Re: [PATCH] BaseTools/tools_def: use separate PP definition for DTC

[Leif Lindholm]

On Tue, Feb 27, 2018 at 07:02:06PM +0000, Ard Biesheuvel wrote:
> On 27 February 2018 at 18:58, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > On Tue, Feb 27, 2018 at 06:36:08PM +0000, Ard Biesheuvel wrote:
> >> On 27 February 2018 at 18:33, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> >> > On Tue, Feb 27, 2018 at 05:51:32PM +0000, Ard Biesheuvel wrote:
> >> >> Clang's preprocessor behaves differently from GCC's, and produces
> >> >> intermediate device tree source that still contains #pragma pack()
> >> >> and other directives that the device tree compiler chokes on.
> >> >>
> >> >> For assembling device tree sources, it matters very little which
> >> >> preprocessor is being used, so let's just use GNU CPP explicitly.
> >> >
> >> > Ah, right, same fundamental issue as
> >> > 5b8766bb92debfa7b2f45a4a6d683b4227360d66.
> >>
> >> Yes, and I fail to see why changing just those two files makes a
> >> meaningful difference.
> >
> > Probably because Secure96Dxe.inf has a lot more [Packages] and
> > [LibraryClasses] dependencies than DeveloperBox.inf.
> >
> > This patch would undoubtedly also have resolved that issue.
> > (As would clang preprocessor not throwing out C syntax when explicitly
> > asked to provide asm.)
> >
> >> > However, this time triggered by autogen seemingly forcing inclusion of
> >> > lots of central header files that are not even used, like
> >> > MdePkg/Include/IndustryStandard/Bluetooth.h
> >> > MdePkg/Include/IndustryStandard/Acpi10.h
> >> > and so on.
> >> >
> >> > Is there any way to suppress these implicit includes from .dts
> >> > processing?
> >>
> >> There is 'Trim', which can filter #include'd content, but using that
> >> also robs us of the ability to #include .dtsi files, which was kind of
> >> the point of using the preprocessor in the first place.
> >
> > Yeah, that'd definitely be missing the point slightly.
> >
> > I was thinking more along the lines of either excluding the autogen
> > bits from the preprocessing or using a separate .inf for the .dts.
> >
> 
> We need the AutoGen.h bits for the fixed PCD values, which was the
> other reason for using the preprocessor in the first place.

Yeah, I was afraid of that.

> Separate .inf would mean registering file GUIDs in the package .DEC,
> so that the driver can find the binary image at runtime, which is what
> I was trying to get away from by pulling it into the driver,
> especially because we are dealing with overlay DT snippets, of which
> there may be many, rather than a single DT (with a well known file
> GUID) for the entire platform.
>
> > If neither of those feels practical, the proposed patch is fine.
> > It just feels a little bit clunky.
> 
> Yeah, I don't see any unclunky solutions, unfortunately.

Fair enough.
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

Re: [PATCH] BaseTools/tools_def: use separate PP definition for DTC

[Gao, Liming]

Reviewed-by: Liming Gao <liming.gao@intel.com>

> -----Original Message-----
> From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> Sent: Wednesday, February 28, 2018 4:01 AM
> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: edk2-devel@lists.01.org; Gao, Liming <liming.gao@intel.com>; Zhu, Yonghong <yonghong.zhu@intel.com>
> Subject: Re: [PATCH] BaseTools/tools_def: use separate PP definition for DTC
> 
> On Tue, Feb 27, 2018 at 07:02:06PM +0000, Ard Biesheuvel wrote:
> > On 27 February 2018 at 18:58, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > > On Tue, Feb 27, 2018 at 06:36:08PM +0000, Ard Biesheuvel wrote:
> > >> On 27 February 2018 at 18:33, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > >> > On Tue, Feb 27, 2018 at 05:51:32PM +0000, Ard Biesheuvel wrote:
> > >> >> Clang's preprocessor behaves differently from GCC's, and produces
> > >> >> intermediate device tree source that still contains #pragma pack()
> > >> >> and other directives that the device tree compiler chokes on.
> > >> >>
> > >> >> For assembling device tree sources, it matters very little which
> > >> >> preprocessor is being used, so let's just use GNU CPP explicitly.
> > >> >
> > >> > Ah, right, same fundamental issue as
> > >> > 5b8766bb92debfa7b2f45a4a6d683b4227360d66.
> > >>
> > >> Yes, and I fail to see why changing just those two files makes a
> > >> meaningful difference.
> > >
> > > Probably because Secure96Dxe.inf has a lot more [Packages] and
> > > [LibraryClasses] dependencies than DeveloperBox.inf.
> > >
> > > This patch would undoubtedly also have resolved that issue.
> > > (As would clang preprocessor not throwing out C syntax when explicitly
> > > asked to provide asm.)
> > >
> > >> > However, this time triggered by autogen seemingly forcing inclusion of
> > >> > lots of central header files that are not even used, like
> > >> > MdePkg/Include/IndustryStandard/Bluetooth.h
> > >> > MdePkg/Include/IndustryStandard/Acpi10.h
> > >> > and so on.
> > >> >
> > >> > Is there any way to suppress these implicit includes from .dts
> > >> > processing?
> > >>
> > >> There is 'Trim', which can filter #include'd content, but using that
> > >> also robs us of the ability to #include .dtsi files, which was kind of
> > >> the point of using the preprocessor in the first place.
> > >
> > > Yeah, that'd definitely be missing the point slightly.
> > >
> > > I was thinking more along the lines of either excluding the autogen
> > > bits from the preprocessing or using a separate .inf for the .dts.
> > >
> >
> > We need the AutoGen.h bits for the fixed PCD values, which was the
> > other reason for using the preprocessor in the first place.
> 
> Yeah, I was afraid of that.
> 
> > Separate .inf would mean registering file GUIDs in the package .DEC,
> > so that the driver can find the binary image at runtime, which is what
> > I was trying to get away from by pulling it into the driver,
> > especially because we are dealing with overlay DT snippets, of which
> > there may be many, rather than a single DT (with a well known file
> > GUID) for the entire platform.
> >
> > > If neither of those feels practical, the proposed patch is fine.
> > > It just feels a little bit clunky.
> >
> > Yeah, I don't see any unclunky solutions, unfortunately.
> 
> Fair enough.
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

Re: [PATCH] BaseTools/tools_def: use separate PP definition for DTC

[Ard Biesheuvel]

On 28 February 2018 at 02:41, Gao, Liming <liming.gao@intel.com> wrote:
> Reviewed-by: Liming Gao <liming.gao@intel.com>
>

Thanks all

Pushed as a68749f39a2e04ef68e5656b7b72fca25a2e23dc

>> -----Original Message-----
>> From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
>> Sent: Wednesday, February 28, 2018 4:01 AM
>> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: edk2-devel@lists.01.org; Gao, Liming <liming.gao@intel.com>; Zhu, Yonghong <yonghong.zhu@intel.com>
>> Subject: Re: [PATCH] BaseTools/tools_def: use separate PP definition for DTC
>>
>> On Tue, Feb 27, 2018 at 07:02:06PM +0000, Ard Biesheuvel wrote:
>> > On 27 February 2018 at 18:58, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>> > > On Tue, Feb 27, 2018 at 06:36:08PM +0000, Ard Biesheuvel wrote:
>> > >> On 27 February 2018 at 18:33, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>> > >> > On Tue, Feb 27, 2018 at 05:51:32PM +0000, Ard Biesheuvel wrote:
>> > >> >> Clang's preprocessor behaves differently from GCC's, and produces
>> > >> >> intermediate device tree source that still contains #pragma pack()
>> > >> >> and other directives that the device tree compiler chokes on.
>> > >> >>
>> > >> >> For assembling device tree sources, it matters very little which
>> > >> >> preprocessor is being used, so let's just use GNU CPP explicitly.
>> > >> >
>> > >> > Ah, right, same fundamental issue as
>> > >> > 5b8766bb92debfa7b2f45a4a6d683b4227360d66.
>> > >>
>> > >> Yes, and I fail to see why changing just those two files makes a
>> > >> meaningful difference.
>> > >
>> > > Probably because Secure96Dxe.inf has a lot more [Packages] and
>> > > [LibraryClasses] dependencies than DeveloperBox.inf.
>> > >
>> > > This patch would undoubtedly also have resolved that issue.
>> > > (As would clang preprocessor not throwing out C syntax when explicitly
>> > > asked to provide asm.)
>> > >
>> > >> > However, this time triggered by autogen seemingly forcing inclusion of
>> > >> > lots of central header files that are not even used, like
>> > >> > MdePkg/Include/IndustryStandard/Bluetooth.h
>> > >> > MdePkg/Include/IndustryStandard/Acpi10.h
>> > >> > and so on.
>> > >> >
>> > >> > Is there any way to suppress these implicit includes from .dts
>> > >> > processing?
>> > >>
>> > >> There is 'Trim', which can filter #include'd content, but using that
>> > >> also robs us of the ability to #include .dtsi files, which was kind of
>> > >> the point of using the preprocessor in the first place.
>> > >
>> > > Yeah, that'd definitely be missing the point slightly.
>> > >
>> > > I was thinking more along the lines of either excluding the autogen
>> > > bits from the preprocessing or using a separate .inf for the .dts.
>> > >
>> >
>> > We need the AutoGen.h bits for the fixed PCD values, which was the
>> > other reason for using the preprocessor in the first place.
>>
>> Yeah, I was afraid of that.
>>
>> > Separate .inf would mean registering file GUIDs in the package .DEC,
>> > so that the driver can find the binary image at runtime, which is what
>> > I was trying to get away from by pulling it into the driver,
>> > especially because we are dealing with overlay DT snippets, of which
>> > there may be many, rather than a single DT (with a well known file
>> > GUID) for the entire platform.
>> >
>> > > If neither of those feels practical, the proposed patch is fine.
>> > > It just feels a little bit clunky.
>> >
>> > Yeah, I don't see any unclunky solutions, unfortunately.
>>
>> Fair enough.
>> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>