public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH] BaseTools/tools_def: use separate PP definition for DTC
@ 2018-02-27 17:51 Ard Biesheuvel
  2018-02-27 18:33 ` Leif Lindholm
  0 siblings, 1 reply; 8+ messages in thread
From: Ard Biesheuvel @ 2018-02-27 17:51 UTC (permalink / raw)
  To: leif.lindholm, edk2-devel; +Cc: liming.gao, yonghong.zhu, 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



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] BaseTools/tools_def: use separate PP definition for DTC
  2018-02-27 17:51 [PATCH] BaseTools/tools_def: use separate PP definition for DTC Ard Biesheuvel
@ 2018-02-27 18:33 ` Leif Lindholm
  2018-02-27 18:36   ` Ard Biesheuvel
  0 siblings, 1 reply; 8+ messages in thread
From: Leif Lindholm @ 2018-02-27 18:33 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: edk2-devel, liming.gao, yonghong.zhu

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
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] BaseTools/tools_def: use separate PP definition for DTC
  2018-02-27 18:33 ` Leif Lindholm
@ 2018-02-27 18:36   ` Ard Biesheuvel
  2018-02-27 18:58     ` Leif Lindholm
  0 siblings, 1 reply; 8+ messages in thread
From: Ard Biesheuvel @ 2018-02-27 18:36 UTC (permalink / raw)
  To: Leif Lindholm; +Cc: edk2-devel@lists.01.org, Gao, Liming, Zhu, Yonghong

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.


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] BaseTools/tools_def: use separate PP definition for DTC
  2018-02-27 18:36   ` Ard Biesheuvel
@ 2018-02-27 18:58     ` Leif Lindholm
  2018-02-27 19:02       ` Ard Biesheuvel
  0 siblings, 1 reply; 8+ messages in thread
From: Leif Lindholm @ 2018-02-27 18:58 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: edk2-devel@lists.01.org, Gao, Liming, Zhu, Yonghong

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] BaseTools/tools_def: use separate PP definition for DTC
  2018-02-27 18:58     ` Leif Lindholm
@ 2018-02-27 19:02       ` Ard Biesheuvel
  2018-02-27 20:01         ` Leif Lindholm
  0 siblings, 1 reply; 8+ messages in thread
From: Ard Biesheuvel @ 2018-02-27 19:02 UTC (permalink / raw)
  To: Leif Lindholm; +Cc: edk2-devel@lists.01.org, Gao, Liming, Zhu, Yonghong

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.


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] BaseTools/tools_def: use separate PP definition for DTC
  2018-02-27 19:02       ` Ard Biesheuvel
@ 2018-02-27 20:01         ` Leif Lindholm
  2018-02-28  2:41           ` Gao, Liming
  0 siblings, 1 reply; 8+ messages in thread
From: Leif Lindholm @ 2018-02-27 20:01 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: edk2-devel@lists.01.org, Gao, Liming, Zhu, Yonghong

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>


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] BaseTools/tools_def: use separate PP definition for DTC
  2018-02-27 20:01         ` Leif Lindholm
@ 2018-02-28  2:41           ` Gao, Liming
  2018-02-28  8:13             ` Ard Biesheuvel
  0 siblings, 1 reply; 8+ messages in thread
From: Gao, Liming @ 2018-02-28  2:41 UTC (permalink / raw)
  To: Leif Lindholm, Ard Biesheuvel; +Cc: edk2-devel@lists.01.org, Zhu, Yonghong

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>


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] BaseTools/tools_def: use separate PP definition for DTC
  2018-02-28  2:41           ` Gao, Liming
@ 2018-02-28  8:13             ` Ard Biesheuvel
  0 siblings, 0 replies; 8+ messages in thread
From: Ard Biesheuvel @ 2018-02-28  8:13 UTC (permalink / raw)
  To: Gao, Liming; +Cc: Leif Lindholm, edk2-devel@lists.01.org, Zhu, Yonghong

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>


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2018-02-28  8:07 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-02-27 17:51 [PATCH] BaseTools/tools_def: use separate PP definition for DTC Ard Biesheuvel
2018-02-27 18:33 ` Leif Lindholm
2018-02-27 18:36   ` Ard Biesheuvel
2018-02-27 18:58     ` Leif Lindholm
2018-02-27 19:02       ` Ard Biesheuvel
2018-02-27 20:01         ` Leif Lindholm
2018-02-28  2:41           ` Gao, Liming
2018-02-28  8:13             ` Ard Biesheuvel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox