Re: [edk2-devel] [PATCH] BaseTools: Limit command line length.

["Bob Feng" <bob.c.feng@intel.com>]

Laszlo,

a) I think it would be a better option. Shih Hao and Atul, would you verify if this method works? 

b) Mingyue, please check this question and see if the patch needs to update. And please update the patch description to make the expression more clear.

Thanks,
Bob

-----Original Message-----
From: Laszlo Ersek <lersek@redhat.com> 
Sent: Monday, November 2, 2020 11:06 PM
To: Feng, Bob C <bob.c.feng@intel.com>; devel@edk2.groups.io; Liang, MingyueX <mingyuex.liang@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>; Chen, Christine <yuwei.chen@intel.com>; Ard Biesheuvel (ARM address) <ard.biesheuvel@arm.com>
Subject: Re: [edk2-devel] [PATCH] BaseTools: Limit command line length.

On 10/28/20 15:57, Feng, Bob C wrote:
> Laszlo,
> 
> 1) I try to provide more information.
> This problem happens because of the commit SHA-1: 05217d210e8da37b47d0be58ec363f7af2fa1c18. This commit changes the compilation command from one command compiles one c file to one command compile multiple c files. That change reduces the compilation time for MSVC toolchain but it increases the length of one compilation command line. Since the build tool can autosave the FLAGS and INC to the response file if the command line length larger than --cmd-len, there is no problem for normal build usage.
> 
> But for the case in BZ2528, user append additional string to the compilation command, that is adding another tool to launch the compiler.
> <path_to_other_tool>/tool.py   <path_to_msvc>/cl.exe ...
> So the it's easy to exceed the max length of command line and cause build failure.

Understood -- the build tool would not consider the user-originated prefix. The build tool would decide that the command is safe to start without using a response file, but the prefix would break that.

> 
> This patch is to save the c files in another response file to make the compilation command shorter.

I have two questions regarding this:


(a) wouldn't it be better to calculate the length of the user-originated prefix (such as the length of "<path_to_other_tool>/tool.py"), and simply include it in the sum that is then checked against "--cmd-len"?


(b) regarding the present patch, do we use the separate response file (which stores the C file names) *only* if there are multiple C files passed to the compiler? (This seems to be confirmed by the commit message.)

I'm asking (b) because, as I understand, the multi-C-file trick is specific to MSVC in the first place, so this further tweak should *also* be specific to MSVC. (With GCC, there is never more than 1 C file on a gcc command line, so the separate response file should never be used.)

> 
> 2) "--cmd-len" is still functional.

Thanks.

One more comment below, regarding the commit message:


> 
> Thanks,
> Bob
> 
> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Tuesday, October 27, 2020 7:58 PM
> To: devel@edk2.groups.io; Liang, MingyueX <mingyuex.liang@intel.com>
> Cc: Feng, Bob C <bob.c.feng@intel.com>; Liming Gao 
> <gaoliming@byosoft.com.cn>; Chen, Christine <yuwei.chen@intel.com>; 
> Ard Biesheuvel (ARM address) <ard.biesheuvel@arm.com>
> Subject: Re: [edk2-devel] [PATCH] BaseTools: Limit command line length.
> 
> (+Ard)
> 
> On 10/23/20 05:00, mliang2x wrote:
>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2528
>>
>> Currently, the command line is too long because the CL command is 
>> followed by multiple C files.
>>
>> Therefore, the number of C files
>> can be used to determine whether the command line needs to be written 
>> to the file. If the number of C files is greater than one, the 
>> command line is directly written to the file. On the contrary, 
>> whether to write to the file is determined by whether the length of 
>> the command line exceeds the limited length Documents.

I don't understand two expressions here:

(a) "On the contrary".

Is this an attempt to describe the current (= incorrect) behavior?

(b) "limited length Documents".

I have no idea what this means.

Thanks,
Laszlo


>>
>> Signed-off-by: Mingyue Liang <mingyuex.liang@intel.com>
>> Cc: Bob Feng <bob.c.feng@intel.com>
>> Cc: Liming Gao <gaoliming@byosoft.com.cn>
>> Cc: Yuwei Chen <yuwei.chen@intel.com>
>> ---
>>  BaseTools/Source/Python/AutoGen/GenMake.py    | 45 +++++++++++++++----
>>  .../Source/Python/AutoGen/IncludesAutoGen.py  | 13 +++++-
>>  2 files changed, 49 insertions(+), 9 deletions(-)
> 
> (1) I've read both BZ#2528 (up to comment 13), and the above commit message too.
> 
> I still don't have the slightest idea what the problem is. Please clarify.
> 
> (2) How do this patch (and this issue) relate to the "--cmd-len" option?
> 
> 
> The bugzilla ticket says the issue is related to MSVC, and that it was exposed by fixing BZ#1672 ("Enable multiple thread for MSVC compiler").
> But, at least superficially, the diffstat and the patch body seem to be more generic. What I really care about is that the gcc command lines should not change. It's annoying to look at a build log and see references to "cc_resp" text files, rather than the actual command lines. My build scripts use "--cmd-len=65536" for that reason -- is this patch going to keep that functional?
> 
> Thanks,
> Laszlo
> 
>>
>> diff --git a/BaseTools/Source/Python/AutoGen/GenMake.py
>> b/BaseTools/Source/Python/AutoGen/GenMake.py
>> index 0314d0ea34..0cb97dc18d 100755
>> --- a/BaseTools/Source/Python/AutoGen/GenMake.py
>> +++ b/BaseTools/Source/Python/AutoGen/GenMake.py
>> @@ -576,7 +576,8 @@ cleanlib:
>>              EdkLogger.error("build", AUTOGEN_ERROR, "Nothing to build",
>>                              ExtraData="[%s]" % str(MyAgo))
>>  
>> -        self.ProcessBuildTargetList()
>> +        self.ProcessBuildTargetList(MyAgo.OutputDir,ToolsDef)
>> +
>>          self.ParserGenerateFfsCmd()
>>  
>>          # Generate macros used to represent input files @@ -866,7
>> +867,6 @@ cleanlib:
>>                                          else:
>>                                              break
>>                                      SingleCommandLength += len(Str)
>> -
>>                              if SingleCommandLength > GlobalData.gCommandMaxLength:
>>                                  FlagDict[Tool]['Value'] = True
>>  
>> @@ -890,18 +890,18 @@ cleanlib:
>>                                      break
>>                              else:
>>                                  break
>> -
>>                          if self._AutoGenObject.ToolChainFamily == 'GCC':
>>                              RespDict[Key] = Value.replace('\\', '/')
>>                          else:
>>                              RespDict[Key] = Value
>> +
>>                          for Target in BuildTargets:
>>                              for i, SingleCommand in enumerate(BuildTargets[Target].Commands):
>>                                  if FlagDict[Flag]['Macro'] in SingleCommand:
>>                                      BuildTargets[Target].Commands[i] = SingleCommand.replace('$(INC)', '').replace(FlagDict[Flag]['Macro'], RespMacro)
>>          return RespDict
>>  
>> -    def ProcessBuildTargetList(self):
>> +    def ProcessBuildTargetList(self, RespFile, ToolsDef):
>>          #
>>          # Search dependency file list for each source file
>>          #
>> @@ -1002,6 +1002,7 @@ cleanlib:
>>                      self.ObjTargetDict[T.Target.SubDir] = set()
>>                  self.ObjTargetDict[T.Target.SubDir].add(NewFile)
>>          for Type in self._AutoGenObject.Targets:
>> +            resp_file_number = 0
>>              for T in self._AutoGenObject.Targets[Type]:
>>                  # Generate related macros if needed
>>                  if T.GenFileListMacro and T.FileListMacro not in self.FileListMacros:
>> @@ -1043,7 +1044,8 @@ cleanlib:
>>                          Deps.append("$(%s)" % T.ListFileMacro)
>>  
>>                  if self._AutoGenObject.BuildRuleFamily == TAB_COMPILER_MSFT and Type == TAB_C_CODE_FILE:
>> -                    T, CmdTarget, CmdTargetDict, CmdCppDict = self.ParserCCodeFile(T, Type, CmdSumDict, CmdTargetDict, CmdCppDict, DependencyDict)
>> +                    T, CmdTarget, CmdTargetDict, CmdCppDict = self.ParserCCodeFile(T, Type, CmdSumDict, CmdTargetDict, CmdCppDict, DependencyDict, RespFile, ToolsDef, resp_file_number)
>> +                    resp_file_number += 1
>>                      TargetDict = {"target": self.PlaceMacro(T.Target.Path, self.Macros), "cmd": "\n\t".join(T.Commands),"deps": CCodeDeps}
>>                      CmdLine = self._BUILD_TARGET_TEMPLATE.Replace(TargetDict).rstrip().replace('\t$(OBJLIST', '$(OBJLIST')
>>                      if T.Commands:
>> @@ -1060,7 +1062,7 @@ cleanlib:
>>                          AnnexeTargetDict = {"target": 
>> self.PlaceMacro(i.Path, self.Macros), "cmd": "", "deps": 
>> self.PlaceMacro(T.Target.Path, self.Macros)}
>>                          
>> self.BuildTargetList.append(self._BUILD_TARGET_TEMPLATE.Replace(Annex
>> e
>> TargetDict))
>>  
>> -    def ParserCCodeFile(self, T, Type, CmdSumDict, CmdTargetDict, CmdCppDict, DependencyDict):
>> +    def ParserCCodeFile(self, T, Type, CmdSumDict, CmdTargetDict, CmdCppDict, DependencyDict, RespFile, ToolsDef, resp_file_number):
>>          if not CmdSumDict:
>>              for item in self._AutoGenObject.Targets[Type]:
>>                  CmdSumDict[item.Target.SubDir] = 
>> item.Target.BaseName @@ -1080,6 +1082,7 @@ cleanlib:
>>                                  CmdCppDict[item.Target.SubDir].append(Path)
>>          if T.Commands:
>>              CommandList = T.Commands[:]
>> +            SaveFilePath = os.path.join(RespFile, "cc_resp_%s.txt" %
>> + resp_file_number)
>>              for Item in CommandList[:]:
>>                  SingleCommandList = Item.split()
>>                  if len(SingleCommandList) > 0 and self.CheckCCCmd(SingleCommandList):
>> @@ -1087,19 +1090,45 @@ cleanlib:
>>                          if Temp.startswith('/Fo'):
>>                              CmdSign = '%s%s' % (Temp.rsplit(TAB_SLASH, 1)[0], TAB_SLASH)
>>                              break
>> -                    else: continue
>> +                    else:
>> +                        continue
>>                      if CmdSign not in list(CmdTargetDict.keys()):
>>                          CmdTargetDict[CmdSign] = Item.replace(Temp, CmdSign)
>>                      else:
>>                          CmdTargetDict[CmdSign] = "%s %s" % 
>> (CmdTargetDict[CmdSign], SingleCommandList[-1])
>> +
>>                      Index = CommandList.index(Item)
>>                      CommandList.pop(Index)
>>                      if SingleCommandList[-1].endswith("%s%s.c" % (TAB_SLASH, CmdSumDict[CmdSign[3:].rsplit(TAB_SLASH, 1)[0]])):
>>                          Cpplist = CmdCppDict[T.Target.SubDir]
>>                          Cpplist.insert(0, '$(OBJLIST_%d): ' % list(self.ObjTargetDict.keys()).index(T.Target.SubDir))
>> -                        T.Commands[Index] = '%s\n\t%s' % (' \\\n\t'.join(Cpplist), CmdTargetDict[CmdSign])
>> +                        cmdtargetlist = CmdTargetDict[CmdSign].split(" ")
>> +                        # get Source files and Save resp file.
>> +                        c_files = []
>> +                        cmds = []
>> +                        if cmdtargetlist:
>> +                            for item in cmdtargetlist:
>> +                                if item.startswith('$(') or item.startswith('/Fo') or item.startswith('"$('):
>> +                                    cmds.append(item)
>> +                                if item.endswith('.c'):
>> +                                    c_files.append(item)
>> +                            c_files.insert(0, " ")
>> +                            if len(c_files) > 2:
>> +                                SaveFileOnChange(SaveFilePath," ".join(c_files), False)
>> +                                T.Commands[Index] = '%s\n\t%s $(cc_resp_%s)' % (' \\\n\t'.join(Cpplist), " ".join(cmds), resp_file_number)
>> +                                ToolsDef.append("cc_resp_%s = @%s" % 
>> + (resp_file_number, SaveFilePath))
>> +
>> +                            elif len(CmdTargetDict[CmdSign]) > GlobalData.gCommandMaxLength and len(c_files) <=2:
>> +                                SaveFileOnChange(SaveFilePath, " ".join(c_files), False)
>> +                                T.Commands[Index] = '%s\n\t%s $(cc_resp_%s)' % (' \\\n\t'.join(Cpplist), " ".join(cmds), resp_file_number)
>> +                                ToolsDef.append("cc_resp_%s = @%s" % 
>> + (resp_file_number, SaveFilePath))
>> +
>> +                            else:
>> +                                T.Commands[Index] = '%s\n\t%s' % (' 
>> + \\\n\t'.join(Cpplist), CmdTargetDict[CmdSign])
>> +
>>                      else:
>>                          T.Commands.pop(Index)
>> +
>>          return T, CmdSumDict, CmdTargetDict, CmdCppDict
>>  
>>      def CheckCCCmd(self, CommandList):
>> diff --git a/BaseTools/Source/Python/AutoGen/IncludesAutoGen.py
>> b/BaseTools/Source/Python/AutoGen/IncludesAutoGen.py
>> index 720d93395a..9f61d49b3a 100644
>> --- a/BaseTools/Source/Python/AutoGen/IncludesAutoGen.py
>> +++ b/BaseTools/Source/Python/AutoGen/IncludesAutoGen.py
>> @@ -203,7 +203,18 @@ ${END}
>>                              cc_options = line[len(cc_cmd)+2:].split()
>>                          else:
>>                              cc_options = line[len(cc_cmd):].split()
>> -                        SourceFileAbsPathMap = {os.path.basename(item):item for item in cc_options if not item.startswith("/") and os.path.exists(item)}
>> +
>> +                        for item in cc_options:
>> +                            if not item.startswith("/"):
>> +                                # if item.startswith("@"):
>> +                                if item.endswith(".txt") and item.startswith("@"):
>> +                                    with open(item[1:], "r") as file:
>> +                                        source_files = file.readlines()[0].split()
>> +                                        SourceFileAbsPathMap = {os.path.basename(file):file for file in source_files if os.path.exists(file)}
>> +                                else:
>> +                                    if os.path.exists(item):
>> +                                        
>> + SourceFileAbsPathMap.update({os.path.basename(item): item.strip()})
>> +
>>              if line in SourceFileAbsPathMap:
>>                  current_source = line
>>                  if current_source not in ModuleDepDict:
>>
>