public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: Laszlo Ersek <lersek@redhat.com>
To: Yonghong Zhu <yonghong.zhu@intel.com>, edk2-devel@ml01.01.org
Cc: Liming Gao <liming.gao@intel.com>
Subject: Re: [Patch] BaseTools: add error check for Macro usage in the INF file
Date: Thu, 23 Feb 2017 02:12:23 +0100	[thread overview]
Message-ID: <0dd4b927-a41f-e3a8-cb59-2421f20d8857@redhat.com> (raw)
In-Reply-To: <43a78396-e70c-287e-275e-2891154c0d5c@redhat.com>

On 02/23/17 02:02, Laszlo Ersek wrote:
> Hi,
> 
> On 02/21/17 02:18, Yonghong Zhu wrote:
>> Use of MACRO statements in the EDK II INF files is limited to local
>> usage only; global or external macros are not permitted. This patch
>> add the check for not defined macros.
>>
>> Cc: Liming Gao <liming.gao@intel.com>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Yonghong Zhu <yonghong.zhu@intel.com>
>> ---
>>  BaseTools/Source/Python/Workspace/MetaFileParser.py    | 9 ++++++++-
>>  BaseTools/Source/Python/Workspace/WorkspaceDatabase.py | 4 +++-
>>  2 files changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/BaseTools/Source/Python/Workspace/MetaFileParser.py b/BaseTools/Source/Python/Workspace/MetaFileParser.py
>> index 1a5fdf5..37a7f5d 100644
>> --- a/BaseTools/Source/Python/Workspace/MetaFileParser.py
>> +++ b/BaseTools/Source/Python/Workspace/MetaFileParser.py
>> @@ -1,9 +1,9 @@
>>  ## @file
>>  # This file is used to parse meta files
>>  #
>> -# Copyright (c) 2008 - 2016, Intel Corporation. All rights reserved.<BR>
>> +# Copyright (c) 2008 - 2017, Intel Corporation. All rights reserved.<BR>
>>  # (C) Copyright 2015-2016 Hewlett Packard Enterprise Development LP<BR>
>>  # This program and the accompanying materials
>>  # are licensed and made available under the terms and conditions of the BSD License
>>  # which accompanies this distribution.  The full text of the license may be found at
>>  # http://opensource.org/licenses/bsd-license.php
>> @@ -349,10 +349,17 @@ class MetaFileParser(object):
>>              EdkLogger.error('Parser', FORMAT_INVALID, "No value specified",
>>                              ExtraData=self._CurrentLine, File=self.MetaFile, Line=self._LineIndex + 1)
>>  
>>          self._ValueList = [ReplaceMacro(Value, self._Macros) for Value in self._ValueList]
>>          Name, Value = self._ValueList[1], self._ValueList[2]
>> +        MacroUsed = GlobalData.gMacroRefPattern.findall(Value)
>> +        if len(MacroUsed) != 0:
>> +            for Macro in MacroUsed:
>> +                if Macro in GlobalData.gGlobalDefines:
>> +                    EdkLogger.error("Parser", FORMAT_INVALID, "Global macro %s is not permitted." % (Macro), ExtraData=self._CurrentLine, File=self.MetaFile, Line=self._LineIndex + 1)
>> +            else:
>> +                EdkLogger.error("Parser", FORMAT_INVALID, "%s not defined" % (Macro), ExtraData=self._CurrentLine, File=self.MetaFile, Line=self._LineIndex + 1)
>>          # Sometimes, we need to make differences between EDK and EDK2 modules 
>>          if Name == 'INF_VERSION':
>>              if re.match(r'0[xX][\da-f-A-F]{5,8}', Value):
>>                  self._Version = int(Value, 0)   
>>              elif re.match(r'\d+\.\d+', Value):
>> diff --git a/BaseTools/Source/Python/Workspace/WorkspaceDatabase.py b/BaseTools/Source/Python/Workspace/WorkspaceDatabase.py
>> index e7bc87d..0686721 100644
>> --- a/BaseTools/Source/Python/Workspace/WorkspaceDatabase.py
>> +++ b/BaseTools/Source/Python/Workspace/WorkspaceDatabase.py
>> @@ -1,9 +1,9 @@
>>  ## @file
>>  # This file is used to create a database used by build tool
>>  #
>> -# Copyright (c) 2008 - 2016, Intel Corporation. All rights reserved.<BR>
>> +# Copyright (c) 2008 - 2017, Intel Corporation. All rights reserved.<BR>
>>  # (C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR>
>>  # This program and the accompanying materials
>>  # are licensed and made available under the terms and conditions of the BSD License
>>  # which accompanies this distribution.  The full text of the license may be found at
>>  # http://opensource.org/licenses/bsd-license.php
>> @@ -1828,10 +1828,12 @@ class InfBuildData(ModuleBuildClassObject):
>>              self.__Macros = {}
>>              # EDK_GLOBAL defined macros can be applied to EDK module
>>              if self.AutoGenVersion < 0x00010005:
>>                  self.__Macros.update(GlobalData.gEdkGlobal)
>>                  self.__Macros.update(GlobalData.gGlobalDefines)
>> +            else:
>> +                self.__Macros.update(self.Defines)
>>          return self.__Macros
>>  
>>      ## Get architecture
>>      def _GetArch(self):
>>          return self._Arch
>>
> 
> I don't understand how, but this patch (commit dc4c770763d0) breaks OVMF for me.
> 
> I bisected it, I didn't want to believe it, then I built the tree right before it, and that worked.
> 
> I also built the tree at current master (526f160f311c, "ArmPkg/ArmMmuLib: AARCH64: enable stack alignment checking", 2017-02-22), with this commit reverted on top, and that also works.
> 
> This is the error I see in the OVMF log:
> 
>> Loading driver E660EA85-058E-4B55-A54B-F02F83A24707
>> InstallProtocolInterface: 5B1B31A1-9562-11D2-8E3F-00A0C969723B 7E8752C0
>> Loading driver at 0x0007E4B7000 EntryPoint=0x0007E4B727B DisplayEngine.efi
>> InstallProtocolInterface: BC62157E-3E33-4FEC-9920-2D3B36D750DF 7E875518
>> ProtectUefiImageCommon - 0x7E8752C0
>>   - 0x000000007E4B7000 - 0x000000000001BAE0
>> ASSERT .../MdeModulePkg/Library/CustomizedDisplayLib/CustomizedDisplayLib.c(928): mCDLStringPackHandle != ((void *) 0)
> 
> I don't have the slightest clue what's going on. Apparently, this change causes BaseTools to mis-build OVMF.

If it's any help, I think that the second hunk breaks library constructors somehow.

I compared the build report files between "good" (--> without this patch) and "bad" (--> with this patch), and the differences I see follow this pattern, under every module that links against library instance with a constructor:

 >----------------------------------------------------------------------------------------------------------------------<
 Library
 ------------------------------------------------------------------------------------------------------------------------
 MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf
 {MemoryAllocationLib}
 MdePkg/Library/PeiServicesTablePointerLibIdt/PeiServicesTablePointerLibIdt.inf
 {PeiServicesTablePointerLib}
 MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
 {PcdLib}
 MdePkg/Library/BaseLib/BaseLib.inf
 {BaseLib}
 MdePkg/Library/BaseMemoryLibRepStr/BaseMemoryLibRepStr.inf
 {BaseMemoryLib}
 MdePkg/Library/PeiHobLib/PeiHobLib.inf
 {HobLib}
 MdePkg/Library/PeiServicesLib/PeiServicesLib.inf
 {PeiServicesLib}
 MdePkg/Library/BaseDebugPrintErrorLevelLib/BaseDebugPrintErrorLevelLib.inf
 {DebugPrintErrorLevelLib}
 MdePkg/Library/BasePrintLib/BasePrintLib.inf
 {PrintLib}
 MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
 {IoLib}
 OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf
-{DebugLib:  C = PlatformDebugLibIoPortConstructor}
+{DebugLib:  C = PlatformDebugLibIoPortConstructor PlatformDebugLibIoPortConstructor}
 MdePkg/Library/PeimEntryPoint/PeimEntryPoint.inf
 {PeimEntryPoint}
 <---------------------------------------------------------------------------------------------------------------------->

Note "DebugLib". In the "bad" case, the constructor name seems to be duplicated.

This applies to all other constructor functions as well.

Thanks
Laszlo


  reply	other threads:[~2017-02-23  1:12 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-02-21  1:18 [Patch] BaseTools: add error check for Macro usage in the INF file Yonghong Zhu
2017-02-21 10:03 ` Gao, Liming
2017-02-22 11:44   ` Ard Biesheuvel
2017-02-22 13:38     ` Zhu, Yonghong
2017-02-22 16:04     ` Zhu, Yonghong
2017-02-23  1:02 ` Laszlo Ersek
2017-02-23  1:12   ` Laszlo Ersek [this message]
2017-02-23  1:14     ` Gao, Liming
2017-02-23 10:22       ` Laszlo Ersek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=0dd4b927-a41f-e3a8-cb59-2421f20d8857@redhat.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox