* [PATCH 0/3] Add PE/COFF resource sections support for XCODE
@ 2020-05-24 21:20 Andrew Fish
2020-05-24 21:20 ` [PATCH 1/3] BaseTools/GenFv: Add PE/COFF resource sections injection to GenFw Andrew Fish
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Andrew Fish @ 2020-05-24 21:20 UTC (permalink / raw)
To: devel; +Cc: Andrew Fish
This patch series adds PE/COFF resource section support for XCODE.
It should be possible to use the build_rule.template and GenFw.c
changes on any toolchain. RVCT also has this issue so could be
fixed.
I test this on XCODE on the inject an extra section, and append to
last section path.
Andrew Fish (3):
BaseTools/GenFv: Add PE/COFF resource sections injection to GenFw
BaseTools: Add PE/COFF resource sections support for XCODE
OvmfwPkg: Don't exclude XCODE Modules
BaseTools/Conf/build_rule.template | 15 +-
BaseTools/Source/C/GenFw/GenFw.c | 370 +++++++++++++++++++++++++++++
OvmfPkg/OvmfPkgIa32.dsc | 3 +-
OvmfPkg/OvmfPkgIa32.fdf | 2 -
OvmfPkg/OvmfPkgIa32X64.dsc | 4 +-
OvmfPkg/OvmfPkgIa32X64.fdf | 2 -
OvmfPkg/OvmfPkgX64.dsc | 3 +-
OvmfPkg/OvmfPkgX64.fdf | 2 -
OvmfPkg/OvmfXen.dsc | 3 +-
OvmfPkg/OvmfXen.fdf | 2 -
10 files changed, 386 insertions(+), 20 deletions(-)
--
2.24.1 (Apple Git-126)
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 1/3] BaseTools/GenFv: Add PE/COFF resource sections injection to GenFw 2020-05-24 21:20 [PATCH 0/3] Add PE/COFF resource sections support for XCODE Andrew Fish @ 2020-05-24 21:20 ` Andrew Fish 2020-05-27 3:15 ` Bob Feng 2020-05-24 21:20 ` [PATCH 2/3] BaseTools: Add PE/COFF resource sections support for XCODE Andrew Fish 2020-05-24 21:20 ` [PATCH 3/3] OvmfwPkg: Don't exclude XCODE Modules Andrew Fish 2 siblings, 1 reply; 9+ messages in thread From: Andrew Fish @ 2020-05-24 21:20 UTC (permalink / raw) To: devel; +Cc: Andrew Fish, Bob Feng, Liming Gao BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=557 The XCODE toolchain does not suport injecting resource sections via libraries so add --rc to GenFw to inject $(MODULE_NAME)hii.rc into the final PE/COFF image. Since moving exiting code around would break source level debugging we must reuse an existing empty section, or add a new section header. If there is not space to add a new section header append to the last section. The resource entry if found via a directory entry so the PE/COFF loading code does not depend on the section type. Signed-off-by: Andrew Fish <afish@apple.com> Cc: Bob Feng <bob.c.feng@intel.com> Cc: Liming Gao <liming.gao@intel.com> --- BaseTools/Source/C/GenFw/GenFw.c | 370 +++++++++++++++++++++++++++++++ 1 file changed, 370 insertions(+) diff --git a/BaseTools/Source/C/GenFw/GenFw.c b/BaseTools/Source/C/GenFw/GenFw.c index 8cab70ba4d5f..748af5dff259 100644 --- a/BaseTools/Source/C/GenFw/GenFw.c +++ b/BaseTools/Source/C/GenFw/GenFw.c @@ -96,6 +96,16 @@ ZeroDebugData ( BOOLEAN ZeroDebug ); +STATIC +EFI_STATUS +PatchResourceData ( + IN UINT32 Type, + IN UINT8 *ResourceData, + IN UINT32 ResourceDataSize, + IN OUT UINT8 **PeCoff, + IN OUT UINT32 *PeCoffSize + ); + STATIC EFI_STATUS SetStamp ( @@ -267,6 +277,11 @@ Returns: except for -o option. It is a action option.\n\ If it is combined with other action options, the later\n\ input action option will override the previous one.\n"); + fprintf (stdout, " --rc FlieName Append a Hii resource section to the\n\ + last PE/COFF section. The FileName is the resource section to append\n\ + If FileName does not exist this operation is skipped. This feature is\n\ + only intended for toolchains, like XCODE, that don't suport $(RC).\n\ + This option can only be combined with -e\n"); fprintf (stdout, " --rebase NewAddress Rebase image to new base address. New address \n\ is also set to the first none code section header.\n\ It can't be combined with other action options\n\ @@ -1059,10 +1074,12 @@ Returns: CHAR8 **InputFileName; char *OutImageName; char *ModuleType; + char *RcFileName; CHAR8 *TimeStamp; FILE *fpIn; FILE *fpOut; FILE *fpInOut; + FILE *fpRc; UINT32 Data; UINT32 *DataPointer; UINT32 *OldDataPointer; @@ -1080,6 +1097,8 @@ Returns: UINT32 OutputFileLength; UINT8 *InputFileBuffer; UINT32 InputFileLength; + UINT8 *RcFileBuffer; + UINT32 RcFileLength; RUNTIME_FUNCTION *RuntimeFunction; UNWIND_INFO *UnwindInfo; STATUS Status; @@ -1116,6 +1135,7 @@ Returns: time_t OutputFileTime; struct stat Stat_Buf; BOOLEAN ZeroDebugFlag; + BOOLEAN InsertRcFile; SetUtilityName (UTILITY_NAME); @@ -1128,6 +1148,7 @@ Returns: mInImageName = NULL; OutImageName = NULL; ModuleType = NULL; + RcFileName = NULL; Type = 0; Status = STATUS_SUCCESS; FileBuffer = NULL; @@ -1164,6 +1185,7 @@ Returns: InputFileTime = 0; OutputFileTime = 0; ZeroDebugFlag = FALSE; + InsertRcFile = FALSE; if (argc == 1) { Error (NULL, 0, 1001, "Missing options", "No input options."); @@ -1436,6 +1458,20 @@ Returns: continue; } + if (stricmp (argv[0], "--rc") == 0) { + RcFileName = argv[1]; + argc -= 2; + argv += 2; + + if (stat (RcFileName, &Stat_Buf) == 0) { + // + // File exists + // + InsertRcFile = TRUE; + } + continue; + } + if (argv[0][0] == '-') { Error (NULL, 0, 1000, "Unknown option", argv[0]); goto Finish; @@ -1568,6 +1604,10 @@ Returns: break; } + if (InsertRcFile) { + VerboseMsg ("RC input file %s", RcFileName); + } + if (ReplaceFlag) { VerboseMsg ("Overwrite the input file with the output content."); } @@ -2052,6 +2092,52 @@ Returns: } } + // + // Insert Resources into the image. + // + if (InsertRcFile) { + fpRc = fopen (LongFilePath (RcFileName), "rb"); + if (fpRc == NULL) { + Error (NULL, 0, 0001, "Error opening file", RcFileName); + goto Finish; + } + + RcFileLength = _filelength (fileno (fpRc)); + RcFileBuffer = malloc (RcFileLength); + if (FileBuffer == NULL) { + Error (NULL, 0, 4001, "Resource", "memory cannot be allocated!"); + fclose (fpRc); + goto Finish; + } + + fread (RcFileBuffer, 1, RcFileLength, fpRc); + fclose (fpRc); + + Status = PatchResourceData (Type, RcFileBuffer, RcFileLength, &FileBuffer, &FileLength); + if (EFI_ERROR (Status)) { + Error (NULL, 0, 3000, "Invalid", "RC Patch Data Error status is 0x%x", (int) Status); + goto Finish; + } + + fpOut = fopen (LongFilePath (OutImageName), "wb"); + if (fpOut == NULL) { + Error (NULL, 0, 0001, "Error opening output file", OutImageName); + goto Finish; + } + + fwrite (FileBuffer, 1, FileLength, fpOut); + + fclose (fpOut); + fpOut = NULL; + VerboseMsg ("the size of output file is %u bytes", (unsigned) FileLength); + + // + // Write the updated Image + // + goto Finish; + } + + // // Convert ELF image to PeImage // @@ -2761,6 +2847,290 @@ Finish: return GetUtilityStatus (); } +#define ALIGN_VALUE(Value, Alignment) ((Value) + (((Alignment) - (Value)) & ((Alignment) - 1))) + +STATIC +EFI_STATUS +PatchResourceData ( + IN UINT32 Type, + IN UINT8 *ResourceData, + IN UINT32 ResourceDataSize, + IN OUT UINT8 **PeCoff, + IN OUT UINT32 *PeCoffSize + ) +/*++ + +Routine Description: + + Embed Resource data into a PE/COFF image. + + If there is free space between the header and the image add a new + .rsrc section to the PE/COFF image. If no space exists then append + the resource data to the last section. + +Arguments: + + Type - If not zero them update PE/COFF header module type. + ResourceData - *hii.rc data to insert into the image. + ResourceDataSize - Size of ResourceData in bytes. + PeCoff - On input existing PE/COFF, on output input PE/COFF with + ResourceData embedded. + PeCoffSize - Size of PeCoff in bytes. + +Returns: + + EFI_ABORTED - PeImage is invalid. + EFI_SUCCESS - Zero debug data successfully. + +--*/ +{ + UINT8 *FileBuffer; + UINT32 FileBufferSize; + EFI_IMAGE_DOS_HEADER *DosHdr; + EFI_IMAGE_FILE_HEADER *FileHdr; + EFI_IMAGE_OPTIONAL_HEADER32 *Optional32Hdr; + EFI_IMAGE_OPTIONAL_HEADER64 *Optional64Hdr; + EFI_IMAGE_SECTION_HEADER *SectionHeader; + UINT32 FileAlignment; + UINT32 Max; + INTN LastSection; + INTN EmptySection; + UINT32 ActualHeaderSize; + UINT32 StartingPeCoffSize; + UINT32 NewHeaderSize; + CHAR8 *Base; + EFI_IMAGE_RESOURCE_DIRECTORY *ResourceDirectory; + EFI_IMAGE_RESOURCE_DIRECTORY_ENTRY *ResourceDirectoryEntry; + EFI_IMAGE_RESOURCE_DIRECTORY_STRING *ResourceDirectoryString; + EFI_IMAGE_RESOURCE_DATA_ENTRY *ResourceDataEntry; + EFI_IMAGE_DATA_DIRECTORY *DirectoryEntry; + CHAR16 *String; + UINT32 Offset; + UINT32 Index; + + // + // Grow the file in units of FileAlignment + // + DosHdr = (EFI_IMAGE_DOS_HEADER *) *PeCoff; + if (DosHdr->e_magic != EFI_IMAGE_DOS_SIGNATURE) { + // NO DOS header, must start with PE/COFF header + FileHdr = (EFI_IMAGE_FILE_HEADER *)(*PeCoff + sizeof (UINT32)); + } else { + FileHdr = (EFI_IMAGE_FILE_HEADER *)(*PeCoff + DosHdr->e_lfanew + sizeof (UINT32)); + } + + Optional32Hdr = (EFI_IMAGE_OPTIONAL_HEADER32 *) ((UINT8*) FileHdr + sizeof (EFI_IMAGE_FILE_HEADER)); + Optional64Hdr = (EFI_IMAGE_OPTIONAL_HEADER64 *) ((UINT8*) FileHdr + sizeof (EFI_IMAGE_FILE_HEADER)); + if (Optional32Hdr->Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) { + FileAlignment = Optional32Hdr->FileAlignment; + SectionHeader = (EFI_IMAGE_SECTION_HEADER *) ((UINT8 *) Optional32Hdr + FileHdr->SizeOfOptionalHeader); + } else { + FileAlignment = Optional64Hdr->FileAlignment; + SectionHeader = (EFI_IMAGE_SECTION_HEADER *) ((UINT8 *) Optional64Hdr + FileHdr->SizeOfOptionalHeader); + } + + LastSection = -1; + for (Index = 0, Max = 0; Index < FileHdr->NumberOfSections; Index++) { + if (SectionHeader[Index].PointerToRawData > Max) { + Max = SectionHeader[Index].PointerToRawData; + LastSection = Index; + } + } + + EmptySection = -1; + for (Index = 0; Index < FileHdr->NumberOfSections; Index++) { + if ((SectionHeader[Index].Misc.VirtualSize == 0) && (SectionHeader[Index].SizeOfRawData == 0)) { + // + // No Data or Zero Fill so we can repurpose this entry. + // + EmptySection = Index; + break; + } + } + + if (EmptySection == -1) { + ActualHeaderSize = (UINTN)(&SectionHeader[FileHdr->NumberOfSections]) - (UINTN)*PeCoff; + if ((ActualHeaderSize + sizeof (EFI_IMAGE_SECTION_HEADER)) <= Optional32Hdr->SizeOfHeaders) { + // + // There is space to inject a new section. + // + FileHdr->NumberOfSections += 1; + EmptySection = Index; + } + } + + StartingPeCoffSize = SectionHeader[LastSection].PointerToRawData + SectionHeader[LastSection].SizeOfRawData; + if (SectionHeader[LastSection].Misc.VirtualSize > SectionHeader[LastSection].SizeOfRawData) { + StartingPeCoffSize += SectionHeader[LastSection].Misc.VirtualSize - SectionHeader[LastSection].SizeOfRawData; + } + + FileBufferSize = ALIGN_VALUE(StartingPeCoffSize + ResourceDataSize, FileAlignment); + FileBuffer = malloc (FileBufferSize); + if (FileBuffer == NULL) { + return RETURN_OUT_OF_RESOURCES; + } + memset (FileBuffer, 0, FileBufferSize); + + // + // Append the Resource Data to the end of the PE/COFF image. + // + NewHeaderSize = Optional32Hdr->SizeOfHeaders; + CopyMem (FileBuffer, *PeCoff, (StartingPeCoffSize > *PeCoffSize) ? *PeCoffSize: StartingPeCoffSize); + CopyMem (FileBuffer + StartingPeCoffSize, ResourceData, ResourceDataSize); + + free (*PeCoff); + *PeCoff = FileBuffer; + *PeCoffSize = FileBufferSize; + + DosHdr = (EFI_IMAGE_DOS_HEADER *)FileBuffer; + if (DosHdr->e_magic != EFI_IMAGE_DOS_SIGNATURE) { + // NO DOS header, must start with PE/COFF header + FileHdr = (EFI_IMAGE_FILE_HEADER *)(FileBuffer + sizeof (UINT32)); + } else { + FileHdr = (EFI_IMAGE_FILE_HEADER *)(FileBuffer + DosHdr->e_lfanew + sizeof (UINT32)); + } + + // + // Get Resource EntryTable offset, and Section header + // + Optional32Hdr = (EFI_IMAGE_OPTIONAL_HEADER32 *) ((UINT8*) FileHdr + sizeof (EFI_IMAGE_FILE_HEADER)); + Optional64Hdr = (EFI_IMAGE_OPTIONAL_HEADER64 *) ((UINT8*) FileHdr + sizeof (EFI_IMAGE_FILE_HEADER)); + DirectoryEntry = NULL; + if (Optional32Hdr->Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) { + SectionHeader = (EFI_IMAGE_SECTION_HEADER *) ((UINT8 *) Optional32Hdr + FileHdr->SizeOfOptionalHeader); + if (Optional32Hdr->NumberOfRvaAndSizes > EFI_IMAGE_DIRECTORY_ENTRY_RESOURCE) { + DirectoryEntry = &Optional32Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_RESOURCE]; + } + } else { + SectionHeader = (EFI_IMAGE_SECTION_HEADER *) ((UINT8 *) Optional64Hdr + FileHdr->SizeOfOptionalHeader); + if (Optional64Hdr->NumberOfRvaAndSizes > EFI_IMAGE_DIRECTORY_ENTRY_RESOURCE) { + DirectoryEntry = &Optional64Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_RESOURCE]; + } + } + + if (DirectoryEntry == NULL) { + return RETURN_OUT_OF_RESOURCES; + } + + if (EmptySection != -1) { + // + // Create a new section + // + CopyMem (SectionHeader[EmptySection].Name, ".rsrc\0\0\0", EFI_IMAGE_SIZEOF_SHORT_NAME); + SectionHeader[EmptySection].Misc.VirtualSize = ResourceDataSize; + SectionHeader[EmptySection].VirtualAddress = StartingPeCoffSize; + SectionHeader[EmptySection].SizeOfRawData = ALIGN_VALUE(ResourceDataSize, FileAlignment); + SectionHeader[EmptySection].PointerToRawData = StartingPeCoffSize; + + SectionHeader[EmptySection].Characteristics = EFI_IMAGE_SCN_MEM_EXECUTE | EFI_IMAGE_SCN_MEM_READ; + SectionHeader[EmptySection].Characteristics |= EFI_IMAGE_SCN_CNT_INITIALIZED_DATA | EFI_IMAGE_SCN_CNT_CODE; + + DirectoryEntry->VirtualAddress = SectionHeader[EmptySection].VirtualAddress; + } else { + // + // Grow the last section to include the resources. + // For Xcode this is always the .debug section. + // + SectionHeader[LastSection].SizeOfRawData = ALIGN_VALUE(SectionHeader[LastSection].SizeOfRawData + ResourceDataSize, FileAlignment); + + // + // Make sure the Virtual Size uses the file aligned actual size, since we are growing the file. + // + SectionHeader[LastSection].Misc.VirtualSize = SectionHeader[LastSection].SizeOfRawData; + + DirectoryEntry->VirtualAddress = StartingPeCoffSize; + } + DirectoryEntry->Size = ResourceDataSize; + + Optional32Hdr->SizeOfImage = ALIGN_VALUE(*PeCoffSize, Optional32Hdr->SectionAlignment); + if (Type != 0) { + Optional32Hdr->Subsystem = Type; + } + + // + // It looks like the ResourceDataEntry->OffsetToData is relative to the rc file, + // but PeCoffLoaderLoadImage() assumes it is a PE/COFF VirtualAddress so we need + // to fix it up. Walk the ResourceDataEntry just like PeCoffLoaderLoadImage() and + // patch it. + // + Base = (CHAR8 *)(FileBuffer + StartingPeCoffSize); + ResourceDirectory = (EFI_IMAGE_RESOURCE_DIRECTORY *)Base; + ResourceDirectoryEntry = (EFI_IMAGE_RESOURCE_DIRECTORY_ENTRY *)(ResourceDirectory + 1); + + for (Index = 0; Index < ResourceDirectory->NumberOfNamedEntries; Index++) { + if (ResourceDirectoryEntry->u1.s.NameIsString) { + // + // Check the ResourceDirectoryEntry->u1.s.NameOffset before use it. + // + if (ResourceDirectoryEntry->u1.s.NameOffset >= DirectoryEntry->Size) { + return RETURN_UNSUPPORTED; + } + ResourceDirectoryString = (EFI_IMAGE_RESOURCE_DIRECTORY_STRING *) (Base + ResourceDirectoryEntry->u1.s.NameOffset); + String = &ResourceDirectoryString->String[0]; + + if (ResourceDirectoryString->Length == 3 && + String[0] == L'H' && + String[1] == L'I' && + String[2] == L'I') { + // + // Resource Type "HII" found + // + if (ResourceDirectoryEntry->u2.s.DataIsDirectory) { + // + // Move to next level - resource Name + // + if (ResourceDirectoryEntry->u2.s.OffsetToDirectory >= DirectoryEntry->Size) { + return RETURN_UNSUPPORTED; + } + ResourceDirectory = (EFI_IMAGE_RESOURCE_DIRECTORY *) (Base + ResourceDirectoryEntry->u2.s.OffsetToDirectory); + Offset = ResourceDirectoryEntry->u2.s.OffsetToDirectory + sizeof (EFI_IMAGE_RESOURCE_DIRECTORY) + + sizeof (EFI_IMAGE_RESOURCE_DIRECTORY_ENTRY) * (ResourceDirectory->NumberOfNamedEntries + ResourceDirectory->NumberOfIdEntries); + if (Offset > DirectoryEntry->Size) { + return RETURN_UNSUPPORTED; + } + ResourceDirectoryEntry = (EFI_IMAGE_RESOURCE_DIRECTORY_ENTRY *) (ResourceDirectory + 1); + + if (ResourceDirectoryEntry->u2.s.DataIsDirectory) { + // + // Move to next level - resource Language + // + if (ResourceDirectoryEntry->u2.s.OffsetToDirectory >= DirectoryEntry->Size) { + return RETURN_UNSUPPORTED; + } + ResourceDirectory = (EFI_IMAGE_RESOURCE_DIRECTORY *) (Base + ResourceDirectoryEntry->u2.s.OffsetToDirectory); + Offset = ResourceDirectoryEntry->u2.s.OffsetToDirectory + sizeof (EFI_IMAGE_RESOURCE_DIRECTORY) + + sizeof (EFI_IMAGE_RESOURCE_DIRECTORY_ENTRY) * (ResourceDirectory->NumberOfNamedEntries + ResourceDirectory->NumberOfIdEntries); + if (Offset > DirectoryEntry->Size) { + return RETURN_UNSUPPORTED; + } + ResourceDirectoryEntry = (EFI_IMAGE_RESOURCE_DIRECTORY_ENTRY *) (ResourceDirectory + 1); + } + } + + // + // Now it ought to be resource Data + // + if (!ResourceDirectoryEntry->u2.s.DataIsDirectory) { + if (ResourceDirectoryEntry->u2.OffsetToData >= DirectoryEntry->Size) { + return RETURN_UNSUPPORTED; + } + ResourceDataEntry = (EFI_IMAGE_RESOURCE_DATA_ENTRY *) (Base + ResourceDirectoryEntry->u2.OffsetToData); + + // + // Adjust OffsetToData to be a PE/COFF Virtual address in the updated image. + // + ResourceDataEntry->OffsetToData += (UINTN)DirectoryEntry->VirtualAddress; + break; + } + } + } + ResourceDirectoryEntry++; + } + + return EFI_SUCCESS; +} + + STATIC EFI_STATUS ZeroDebugData ( -- 2.24.1 (Apple Git-126) ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] BaseTools/GenFv: Add PE/COFF resource sections injection to GenFw 2020-05-24 21:20 ` [PATCH 1/3] BaseTools/GenFv: Add PE/COFF resource sections injection to GenFw Andrew Fish @ 2020-05-27 3:15 ` Bob Feng 0 siblings, 0 replies; 9+ messages in thread From: Bob Feng @ 2020-05-27 3:15 UTC (permalink / raw) To: Andrew Fish, devel@edk2.groups.io; +Cc: Gao, Liming Hi Andrew, This patch cause building GenFw failure. cl.exe -c /nologo /Zi /c /O2 /MT /W4 /WX /D _CRT_SECURE_NO_DEPRECATE /D _CRT_NONSTDC_NO_DEPRECATE -I . -I D:\Edk2Maintain\edk2head\edk2\BaseTools\Source\C\Include -I D:\Edk2Maintain\edk2head\edk2\BaseTools\Source\C\Include\Ia32 -I D:\Edk2Maintain\edk2head\edk2\BaseTools\Source\C\Common GenFw.c -FoGenFw.obj GenFw.c GenFw.c(3047): error C2220: the following warning is treated as an error GenFw.c(3047): warning C4244: '=': conversion from 'UINT32' to 'UINT16', possible loss of data NMAKE : fatal error U1077: '"C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.25.28610\bin\HostX86\x86\cl.exe"' : return code '0x2' Stop. The possible fix could be change the parameter Type as UINT16 data type. Would you update the patch? + +STATIC +EFI_STATUS +PatchResourceData ( + IN UINT32 Type, + IN UINT8 *ResourceData, + IN UINT32 ResourceDataSize, + IN OUT UINT8 **PeCoff, + IN OUT UINT32 *PeCoffSize + ) +/*++ And a minor comment is about the commit message. BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=557 should change to REF: https://bugzilla.tianocore.org/show_bug.cgi?id=557 Thanks, Bob -----Original Message----- From: Andrew Fish <afish@apple.com> Sent: Monday, May 25, 2020 5:20 AM To: devel@edk2.groups.io Cc: Andrew Fish <afish@apple.com>; Feng, Bob C <bob.c.feng@intel.com>; Gao, Liming <liming.gao@intel.com> Subject: [PATCH 1/3] BaseTools/GenFv: Add PE/COFF resource sections injection to GenFw BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=557 The XCODE toolchain does not suport injecting resource sections via libraries so add --rc to GenFw to inject $(MODULE_NAME)hii.rc into the final PE/COFF image. Since moving exiting code around would break source level debugging we must reuse an existing empty section, or add a new section header. If there is not space to add a new section header append to the last section. The resource entry if found via a directory entry so the PE/COFF loading code does not depend on the section type. Signed-off-by: Andrew Fish <afish@apple.com> Cc: Bob Feng <bob.c.feng@intel.com> Cc: Liming Gao <liming.gao@intel.com> --- BaseTools/Source/C/GenFw/GenFw.c | 370 +++++++++++++++++++++++++++++++ 1 file changed, 370 insertions(+) diff --git a/BaseTools/Source/C/GenFw/GenFw.c b/BaseTools/Source/C/GenFw/GenFw.c index 8cab70ba4d5f..748af5dff259 100644 --- a/BaseTools/Source/C/GenFw/GenFw.c +++ b/BaseTools/Source/C/GenFw/GenFw.c @@ -96,6 +96,16 @@ ZeroDebugData ( BOOLEAN ZeroDebug ); +STATIC+EFI_STATUS+PatchResourceData (+ IN UINT32 Type,+ IN UINT8 *ResourceData,+ IN UINT32 ResourceDataSize,+ IN OUT UINT8 **PeCoff,+ IN OUT UINT32 *PeCoffSize+ );+ STATIC EFI_STATUS SetStamp (@@ -267,6 +277,11 @@ Returns: except for -o option. It is a action option.\n\ If it is combined with other action options, the later\n\ input action option will override the previous one.\n");+ fprintf (stdout, " --rc FlieName Append a Hii resource section to the\n\+ last PE/COFF section. The FileName is the resource section to append\n\+ If FileName does not exist this operation is skipped. This feature is\n\+ only intended for toolchains, like XCODE, that don't suport $(RC).\n\+ This option can only be combined with -e\n"); fprintf (stdout, " --rebase NewAddress Rebase image to new base address. New address \n\ is also set to the first none code section header.\n\ It can't be combined with other action options\n\@@ -1059,10 +1074,12 @@ Returns: CHAR8 **InputFileName; char *OutImageName; char *ModuleType;+ char *RcFileName; CHAR8 *TimeStamp; FILE *fpIn; FILE *fpOut; FILE *fpInOut;+ FILE *fpRc; UINT32 Data; UINT32 *DataPointer; UINT32 *OldDataPointer;@@ -1080,6 +1097,8 @@ Returns: UINT32 OutputFileLength; UINT8 *InputFileBuffer; UINT32 InputFileLength;+ UINT8 *RcFileBuffer;+ UINT32 RcFileLength; RUNTIME_FUNCTION *RuntimeFunction; UNWIND_INFO *UnwindInfo; STATUS Status;@@ -1116,6 +1135,7 @@ Returns: time_t OutputFileTime; struct stat Stat_Buf; BOOLEAN ZeroDebugFlag;+ BOOLEAN InsertRcFile; SetUtilityName (UTILITY_NAME); @@ -1128,6 +1148,7 @@ Returns: mInImageName = NULL; OutImageName = NULL; ModuleType = NULL;+ RcFileName = NULL; Type = 0; Status = STATUS_SUCCESS; FileBuffer = NULL;@@ -1164,6 +1185,7 @@ Returns: InputFileTime = 0; OutputFileTime = 0; ZeroDebugFlag = FALSE;+ InsertRcFile = FALSE; if (argc == 1) { Error (NULL, 0, 1001, "Missing options", "No input options.");@@ -1436,6 +1458,20 @@ Returns: continue; } + if (stricmp (argv[0], "--rc") == 0) {+ RcFileName = argv[1];+ argc -= 2;+ argv += 2;++ if (stat (RcFileName, &Stat_Buf) == 0) {+ //+ // File exists+ //+ InsertRcFile = TRUE;+ }+ continue;+ }+ if (argv[0][0] == '-') { Error (NULL, 0, 1000, "Unknown option", argv[0]); goto Finish;@@ -1568,6 +1604,10 @@ Returns: break; } + if (InsertRcFile) {+ VerboseMsg ("RC input file %s", RcFileName);+ }+ if (ReplaceFlag) { VerboseMsg ("Overwrite the input file with the output content."); }@@ -2052,6 +2092,52 @@ Returns: } } + //+ // Insert Resources into the image.+ //+ if (InsertRcFile) {+ fpRc = fopen (LongFilePath (RcFileName), "rb");+ if (fpRc == NULL) {+ Error (NULL, 0, 0001, "Error opening file", RcFileName);+ goto Finish;+ }++ RcFileLength = _filelength (fileno (fpRc));+ RcFileBuffer = malloc (RcFileLength);+ if (FileBuffer == NULL) {+ Error (NULL, 0, 4001, "Resource", "memory cannot be allocated!");+ fclose (fpRc);+ goto Finish;+ }++ fread (RcFileBuffer, 1, RcFileLength, fpRc);+ fclose (fpRc);++ Status = PatchResourceData (Type, RcFileBuffer, RcFileLength, &FileBuffer, &FileLength);+ if (EFI_ERROR (Status)) {+ Error (NULL, 0, 3000, "Invalid", "RC Patch Data Error status is 0x%x", (int) Status);+ goto Finish;+ }++ fpOut = fopen (LongFilePath (OutImageName), "wb");+ if (fpOut == NULL) {+ Error (NULL, 0, 0001, "Error opening output file", OutImageName);+ goto Finish;+ }++ fwrite (FileBuffer, 1, FileLength, fpOut);++ fclose (fpOut);+ fpOut = NULL;+ VerboseMsg ("the size of output file is %u bytes", (unsigned) FileLength);++ //+ // Write the updated Image+ //+ goto Finish;+ }++ // // Convert ELF image to PeImage //@@ -2761,6 +2847,290 @@ Finish: return GetUtilityStatus (); } +#define ALIGN_VALUE(Value, Alignment) ((Value) + (((Alignment) - (Value)) & ((Alignment) - 1)))++STATIC+EFI_STATUS+PatchResourceData (+ IN UINT32 Type,+ IN UINT8 *ResourceData,+ IN UINT32 ResourceDataSize,+ IN OUT UINT8 **PeCoff,+ IN OUT UINT32 *PeCoffSize+ )+/*++++Routine Description:++ Embed Resource data into a PE/COFF image.++ If there is free space between the header and the image add a new+ .rsrc section to the PE/COFF image. If no space exists then append+ the resource data to the last section.++Arguments:++ Type - If not zero them update PE/COFF header module type.+ ResourceData - *hii.rc data to insert into the image.+ ResourceDataSize - Size of ResourceData in bytes.+ PeCoff - On input existing PE/COFF, on output input PE/COFF with+ ResourceData embedded.+ PeCoffSize - Size of PeCoff in bytes.++Returns:++ EFI_ABORTED - PeImage is invalid.+ EFI_SUCCESS - Zero debug data successfully.++--*/+{+ UINT8 *FileBuffer;+ UINT32 FileBufferSize;+ EFI_IMAGE_DOS_HEADER *DosHdr;+ EFI_IMAGE_FILE_HEADER *FileHdr;+ EFI_IMAGE_OPTIONAL_HEADER32 *Optional32Hdr;+ EFI_IMAGE_OPTIONAL_HEADER64 *Optional64Hdr;+ EFI_IMAGE_SECTION_HEADER *SectionHeader;+ UINT32 FileAlignment;+ UINT32 Max;+ INTN LastSection;+ INTN EmptySection;+ UINT32 ActualHeaderSize;+ UINT32 StartingPeCoffSize;+ UINT32 NewHeaderSize;+ CHAR8 *Base;+ EFI_IMAGE_RESOURCE_DIRECTORY *ResourceDirectory;+ EFI_IMAGE_RESOURCE_DIRECTORY_ENTRY *ResourceDirectoryEntry;+ EFI_IMAGE_RESOURCE_DIRECTORY_STRING *ResourceDirectoryString;+ EFI_IMAGE_RESOURCE_DATA_ENTRY *ResourceDataEntry;+ EFI_IMAGE_DATA_DIRECTORY *DirectoryEntry;+ CHAR16 *String;+ UINT32 Offset;+ UINT32 Index;++ //+ // Grow the file in units of FileAlignment+ //+ DosHdr = (EFI_IMAGE_DOS_HEADER *) *PeCoff;+ if (DosHdr->e_magic != EFI_IMAGE_DOS_SIGNATURE) {+ // NO DOS header, must start with PE/COFF header+ FileHdr = (EFI_IMAGE_FILE_HEADER *)(*PeCoff + sizeof (UINT32));+ } else {+ FileHdr = (EFI_IMAGE_FILE_HEADER *)(*PeCoff + DosHdr->e_lfanew + sizeof (UINT32));+ }++ Optional32Hdr = (EFI_IMAGE_OPTIONAL_HEADER32 *) ((UINT8*) FileHdr + sizeof (EFI_IMAGE_FILE_HEADER));+ Optional64Hdr = (EFI_IMAGE_OPTIONAL_HEADER64 *) ((UINT8*) FileHdr + sizeof (EFI_IMAGE_FILE_HEADER));+ if (Optional32Hdr->Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {+ FileAlignment = Optional32Hdr->FileAlignment;+ SectionHeader = (EFI_IMAGE_SECTION_HEADER *) ((UINT8 *) Optional32Hdr + FileHdr->SizeOfOptionalHeader);+ } else {+ FileAlignment = Optional64Hdr->FileAlignment;+ SectionHeader = (EFI_IMAGE_SECTION_HEADER *) ((UINT8 *) Optional64Hdr + FileHdr->SizeOfOptionalHeader);+ }++ LastSection = -1;+ for (Index = 0, Max = 0; Index < FileHdr->NumberOfSections; Index++) {+ if (SectionHeader[Index].PointerToRawData > Max) {+ Max = SectionHeader[Index].PointerToRawData;+ LastSection = Index;+ }+ }++ EmptySection = -1;+ for (Index = 0; Index < FileHdr->NumberOfSections; Index++) {+ if ((SectionHeader[Index].Misc.VirtualSize == 0) && (SectionHeader[Index].SizeOfRawData == 0)) {+ //+ // No Data or Zero Fill so we can repurpose this entry.+ //+ EmptySection = Index;+ break;+ }+ }++ if (EmptySection == -1) {+ ActualHeaderSize = (UINTN)(&SectionHeader[FileHdr->NumberOfSections]) - (UINTN)*PeCoff;+ if ((ActualHeaderSize + sizeof (EFI_IMAGE_SECTION_HEADER)) <= Optional32Hdr->SizeOfHeaders) {+ //+ // There is space to inject a new section.+ //+ FileHdr->NumberOfSections += 1;+ EmptySection = Index;+ }+ }++ StartingPeCoffSize = SectionHeader[LastSection].PointerToRawData + SectionHeader[LastSection].SizeOfRawData;+ if (SectionHeader[LastSection].Misc.VirtualSize > SectionHeader[LastSection].SizeOfRawData) {+ StartingPeCoffSize += SectionHeader[LastSection].Misc.VirtualSize - SectionHeader[LastSection].SizeOfRawData;+ }++ FileBufferSize = ALIGN_VALUE(StartingPeCoffSize + ResourceDataSize, FileAlignment);+ FileBuffer = malloc (FileBufferSize);+ if (FileBuffer == NULL) {+ return RETURN_OUT_OF_RESOURCES;+ }+ memset (FileBuffer, 0, FileBufferSize);++ //+ // Append the Resource Data to the end of the PE/COFF image.+ //+ NewHeaderSize = Optional32Hdr->SizeOfHeaders;+ CopyMem (FileBuffer, *PeCoff, (StartingPeCoffSize > *PeCoffSize) ? *PeCoffSize: StartingPeCoffSize);+ CopyMem (FileBuffer + StartingPeCoffSize, ResourceData, ResourceDataSize);++ free (*PeCoff);+ *PeCoff = FileBuffer;+ *PeCoffSize = FileBufferSize;++ DosHdr = (EFI_IMAGE_DOS_HEADER *)FileBuffer;+ if (DosHdr->e_magic != EFI_IMAGE_DOS_SIGNATURE) {+ // NO DOS header, must start with PE/COFF header+ FileHdr = (EFI_IMAGE_FILE_HEADER *)(FileBuffer + sizeof (UINT32));+ } else {+ FileHdr = (EFI_IMAGE_FILE_HEADER *)(FileBuffer + DosHdr->e_lfanew + sizeof (UINT32));+ }++ //+ // Get Resource EntryTable offset, and Section header+ //+ Optional32Hdr = (EFI_IMAGE_OPTIONAL_HEADER32 *) ((UINT8*) FileHdr + sizeof (EFI_IMAGE_FILE_HEADER));+ Optional64Hdr = (EFI_IMAGE_OPTIONAL_HEADER64 *) ((UINT8*) FileHdr + sizeof (EFI_IMAGE_FILE_HEADER));+ DirectoryEntry = NULL;+ if (Optional32Hdr->Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {+ SectionHeader = (EFI_IMAGE_SECTION_HEADER *) ((UINT8 *) Optional32Hdr + FileHdr->SizeOfOptionalHeader);+ if (Optional32Hdr->NumberOfRvaAndSizes > EFI_IMAGE_DIRECTORY_ENTRY_RESOURCE) {+ DirectoryEntry = &Optional32Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_RESOURCE];+ }+ } else {+ SectionHeader = (EFI_IMAGE_SECTION_HEADER *) ((UINT8 *) Optional64Hdr + FileHdr->SizeOfOptionalHeader);+ if (Optional64Hdr->NumberOfRvaAndSizes > EFI_IMAGE_DIRECTORY_ENTRY_RESOURCE) {+ DirectoryEntry = &Optional64Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_RESOURCE];+ }+ }++ if (DirectoryEntry == NULL) {+ return RETURN_OUT_OF_RESOURCES;+ }++ if (EmptySection != -1) {+ //+ // Create a new section+ //+ CopyMem (SectionHeader[EmptySection].Name, ".rsrc\0\0\0", EFI_IMAGE_SIZEOF_SHORT_NAME);+ SectionHeader[EmptySection].Misc.VirtualSize = ResourceDataSize;+ SectionHeader[EmptySection].VirtualAddress = StartingPeCoffSize;+ SectionHeader[EmptySection].SizeOfRawData = ALIGN_VALUE(ResourceDataSize, FileAlignment);+ SectionHeader[EmptySection].PointerToRawData = StartingPeCoffSize;++ SectionHeader[EmptySection].Characteristics = EFI_IMAGE_SCN_MEM_EXECUTE | EFI_IMAGE_SCN_MEM_READ;+ SectionHeader[EmptySection].Characteristics |= EFI_IMAGE_SCN_CNT_INITIALIZED_DATA | EFI_IMAGE_SCN_CNT_CODE;++ DirectoryEntry->VirtualAddress = SectionHeader[EmptySection].VirtualAddress;+ } else {+ //+ // Grow the last section to include the resources.+ // For Xcode this is always the .debug section.+ //+ SectionHeader[LastSection].SizeOfRawData = ALIGN_VALUE(SectionHeader[LastSection].SizeOfRawData + ResourceDataSize, FileAlignment);++ //+ // Make sure the Virtual Size uses the file aligned actual size, since we are growing the file.+ //+ SectionHeader[LastSection].Misc.VirtualSize = SectionHeader[LastSection].SizeOfRawData;++ DirectoryEntry->VirtualAddress = StartingPeCoffSize;+ }+ DirectoryEntry->Size = ResourceDataSize;++ Optional32Hdr->SizeOfImage = ALIGN_VALUE(*PeCoffSize, Optional32Hdr->SectionAlignment);+ if (Type != 0) {+ Optional32Hdr->Subsystem = Type;+ }++ //+ // It looks like the ResourceDataEntry->OffsetToData is relative to the rc file,+ // but PeCoffLoaderLoadImage() assumes it is a PE/COFF VirtualAddress so we need+ // to fix it up. Walk the ResourceDataEntry just like PeCoffLoaderLoadImage() and+ // patch it.+ //+ Base = (CHAR8 *)(FileBuffer + StartingPeCoffSize);+ ResourceDirectory = (EFI_IMAGE_RESOURCE_DIRECTORY *)Base;+ ResourceDirectoryEntry = (EFI_IMAGE_RESOURCE_DIRECTORY_ENTRY *)(ResourceDirectory + 1);++ for (Index = 0; Index < ResourceDirectory->NumberOfNamedEntries; Index++) {+ if (ResourceDirectoryEntry->u1.s.NameIsString) {+ //+ // Check the ResourceDirectoryEntry->u1.s.NameOffset before use it.+ //+ if (ResourceDirectoryEntry->u1.s.NameOffset >= DirectoryEntry->Size) {+ return RETURN_UNSUPPORTED;+ }+ ResourceDirectoryString = (EFI_IMAGE_RESOURCE_DIRECTORY_STRING *) (Base + ResourceDirectoryEntry->u1.s.NameOffset);+ String = &ResourceDirectoryString->String[0];++ if (ResourceDirectoryString->Length == 3 &&+ String[0] == L'H' &&+ String[1] == L'I' &&+ String[2] == L'I') {+ //+ // Resource Type "HII" found+ //+ if (ResourceDirectoryEntry->u2.s.DataIsDirectory) {+ //+ // Move to next level - resource Name+ //+ if (ResourceDirectoryEntry->u2.s.OffsetToDirectory >= DirectoryEntry->Size) {+ return RETURN_UNSUPPORTED;+ }+ ResourceDirectory = (EFI_IMAGE_RESOURCE_DIRECTORY *) (Base + ResourceDirectoryEntry->u2.s.OffsetToDirectory);+ Offset = ResourceDirectoryEntry->u2.s.OffsetToDirectory + sizeof (EFI_IMAGE_RESOURCE_DIRECTORY) ++ sizeof (EFI_IMAGE_RESOURCE_DIRECTORY_ENTRY) * (ResourceDirectory->NumberOfNamedEntries + ResourceDirectory->NumberOfIdEntries);+ if (Offset > DirectoryEntry->Size) {+ return RETURN_UNSUPPORTED;+ }+ ResourceDirectoryEntry = (EFI_IMAGE_RESOURCE_DIRECTORY_ENTRY *) (ResourceDirectory + 1);++ if (ResourceDirectoryEntry->u2.s.DataIsDirectory) {+ //+ // Move to next level - resource Language+ //+ if (ResourceDirectoryEntry->u2.s.OffsetToDirectory >= DirectoryEntry->Size) {+ return RETURN_UNSUPPORTED;+ }+ ResourceDirectory = (EFI_IMAGE_RESOURCE_DIRECTORY *) (Base + ResourceDirectoryEntry->u2.s.OffsetToDirectory);+ Offset = ResourceDirectoryEntry->u2.s.OffsetToDirectory + sizeof (EFI_IMAGE_RESOURCE_DIRECTORY) ++ sizeof (EFI_IMAGE_RESOURCE_DIRECTORY_ENTRY) * (ResourceDirectory->NumberOfNamedEntries + ResourceDirectory->NumberOfIdEntries);+ if (Offset > DirectoryEntry->Size) {+ return RETURN_UNSUPPORTED;+ }+ ResourceDirectoryEntry = (EFI_IMAGE_RESOURCE_DIRECTORY_ENTRY *) (ResourceDirectory + 1);+ }+ }++ //+ // Now it ought to be resource Data+ //+ if (!ResourceDirectoryEntry->u2.s.DataIsDirectory) {+ if (ResourceDirectoryEntry->u2.OffsetToData >= DirectoryEntry->Size) {+ return RETURN_UNSUPPORTED;+ }+ ResourceDataEntry = (EFI_IMAGE_RESOURCE_DATA_ENTRY *) (Base + ResourceDirectoryEntry->u2.OffsetToData);++ //+ // Adjust OffsetToData to be a PE/COFF Virtual address in the updated image.+ //+ ResourceDataEntry->OffsetToData += (UINTN)DirectoryEntry->VirtualAddress;+ break;+ }+ }+ }+ ResourceDirectoryEntry++;+ }++ return EFI_SUCCESS;+}++ STATIC EFI_STATUS ZeroDebugData (-- 2.24.1 (Apple Git-126) ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/3] BaseTools: Add PE/COFF resource sections support for XCODE 2020-05-24 21:20 [PATCH 0/3] Add PE/COFF resource sections support for XCODE Andrew Fish 2020-05-24 21:20 ` [PATCH 1/3] BaseTools/GenFv: Add PE/COFF resource sections injection to GenFw Andrew Fish @ 2020-05-24 21:20 ` Andrew Fish 2020-05-24 21:20 ` [PATCH 3/3] OvmfwPkg: Don't exclude XCODE Modules Andrew Fish 2 siblings, 0 replies; 9+ messages in thread From: Andrew Fish @ 2020-05-24 21:20 UTC (permalink / raw) To: devel; +Cc: Andrew Fish, Bob Feng, Liming Gao BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=557 Build a nul lib in [Hii-Binary-Package.UEFI_HII] so the makefile dependencies are satisfied. Add --rc to Genfw in [Dynamic-Library-File] to inject the *hii.rc data if it exists. Signed-off-by: Andrew Fish <afish@apple.com> Cc: Bob Feng <bob.c.feng@intel.com> Cc: Liming Gao <liming.gao@intel.com> Signed-off-by: Andrew Fish <afish@apple.com> --- BaseTools/Conf/build_rule.template | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/BaseTools/Conf/build_rule.template b/BaseTools/Conf/build_rule.template index 0822b681fcd9..00f729a40003 100755 --- a/BaseTools/Conf/build_rule.template +++ b/BaseTools/Conf/build_rule.template @@ -393,7 +393,7 @@ "$(MTOC)" -subsystem $(MODULE_TYPE) $(MTOC_FLAGS) ${src} $(DEBUG_DIR)(+)$(MODULE_NAME).pecoff # create symbol file for GDB debug -$(DSYMUTIL) ${src} - "$(GENFW)" -e $(MODULE_TYPE) -o ${dst} $(DEBUG_DIR)(+)$(MODULE_NAME).pecoff $(GENFW_FLAGS) + "$(GENFW)" -e $(MODULE_TYPE) --rc $(OUTPUT_DIR)(+)$(MODULE_NAME)hii.rc -o ${dst} $(DEBUG_DIR)(+)$(MODULE_NAME).pecoff $(GENFW_FLAGS) $(CP) ${dst} $(DEBUG_DIR) $(CP) ${dst} $(BIN_DIR)(+)$(MODULE_NAME_GUID).efi -$(CP) $(DEBUG_DIR)(+)*.map $(OUTPUT_DIR) @@ -645,10 +645,10 @@ <InputFile> *.hpk - <OutputFile.MSFT, OutputFile.INTEL, OutputFile.GCC, OutputFile.CLANGPDB> + <OutputFile.MSFT, OutputFile.INTEL, OutputFile.GCC, OutputFile.CLANGPDB, OutputFile.XCODE> $(OUTPUT_DIR)(+)$(MODULE_NAME)hii.lib - <OutputFile.XCODE, OutputFile.RVCT> + <OutputFile.RVCT> $(OUTPUT_DIR)(+)$(MODULE_NAME)hii.rc <Command.MSFT, Command.INTEL, Command.CLANGPDB> @@ -659,5 +659,12 @@ "$(GENFW)" -o $(OUTPUT_DIR)(+)$(MODULE_NAME)hii.rc -g $(MODULE_GUID) --hiibinpackage $(HII_BINARY_PACKAGES) $(GENFW_FLAGS) "$(RC)" $(RC_FLAGS) $(OUTPUT_DIR)(+)$(MODULE_NAME)hii.rc ${dst} - <Command.XCODE, Command.RVCT> + <Command.RVCT> GenFw -o $(OUTPUT_DIR)(+)$(MODULE_NAME)hii.rc -g $(MODULE_GUID) --hiibinpackage $(HII_BINARY_PACKAGES) + + <Command.XCODE> + GenFw -o $(OUTPUT_DIR)(+)$(MODULE_NAME)hii.rc -g $(MODULE_GUID) --hiibinpackage $(HII_BINARY_PACKAGES) $(GENFW_FLAGS) + # Fake a lib that is empty for a make dependency like toolchains with a real RC tool. + echo "int aksdlfjlksdfjlksslkdfj;" > $(OUTPUT_DIR)(+)$(MODULE_NAME)hii.hhi.c; + "$(CC)" $(DEPS_FLAGS) $(CC_FLAGS) -o $(OUTPUT_DIR)(+)$(MODULE_NAME)hii.hii.o $(INC) $(OUTPUT_DIR)(+)$(MODULE_NAME)hii.hhi.c + "$(SLINK)" $(SLINK_FLAGS) ${dst} $(OUTPUT_DIR)(+)$(MODULE_NAME)hii.hii.o -- 2.24.1 (Apple Git-126) ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 3/3] OvmfwPkg: Don't exclude XCODE Modules 2020-05-24 21:20 [PATCH 0/3] Add PE/COFF resource sections support for XCODE Andrew Fish 2020-05-24 21:20 ` [PATCH 1/3] BaseTools/GenFv: Add PE/COFF resource sections injection to GenFw Andrew Fish 2020-05-24 21:20 ` [PATCH 2/3] BaseTools: Add PE/COFF resource sections support for XCODE Andrew Fish @ 2020-05-24 21:20 ` Andrew Fish 2020-05-25 19:31 ` [edk2-devel] " Laszlo Ersek 2 siblings, 1 reply; 9+ messages in thread From: Andrew Fish @ 2020-05-24 21:20 UTC (permalink / raw) To: devel Cc: Andrew Fish, Ard Biesheuvel, Jiewen Yao, Jordan Justen, Philippe Mathieu-Daudé With this BZ getting fixed we no longer need to special case XCODE. Cc: Ard Biesheuvel <ard.biesheuvel@arm.com> Cc: Jiewen Yao <jiewen.yao@intel.com> Cc: Jordan Justen <jordan.l.justen@intel.com> Cc: Philippe Mathieu-Daudé <philmd@redhat.com> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=557 Signed-off-by: Andrew Fish <afish@apple.com> Signed-off-by: Andrew Fish <afish@apple.com> --- OvmfPkg/OvmfPkgIa32.dsc | 3 +-- OvmfPkg/OvmfPkgIa32.fdf | 2 -- OvmfPkg/OvmfPkgIa32X64.dsc | 4 ++-- OvmfPkg/OvmfPkgIa32X64.fdf | 2 -- OvmfPkg/OvmfPkgX64.dsc | 3 +-- OvmfPkg/OvmfPkgX64.fdf | 2 -- OvmfPkg/OvmfXen.dsc | 3 +-- OvmfPkg/OvmfXen.fdf | 2 -- 8 files changed, 5 insertions(+), 16 deletions(-) diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc index d0df9cbbfb2b..8166d1588ae5 100644 --- a/OvmfPkg/OvmfPkgIa32.dsc +++ b/OvmfPkg/OvmfPkgIa32.dsc @@ -878,7 +878,6 @@ [Components] OvmfPkg/Csm/Csm16/Csm16.inf !endif -!if $(TOOL_CHAIN_TAG) != "XCODE5" ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf { <PcdsFixedAtBuild> gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE @@ -887,7 +886,7 @@ [Components] <PcdsFixedAtBuild> gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE } -!endif + ShellPkg/Application/Shell/Shell.inf { <LibraryClasses> ShellCommandLib|ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.inf diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf index e2b759aa8d05..cb1a8b1eba60 100644 --- a/OvmfPkg/OvmfPkgIa32.fdf +++ b/OvmfPkg/OvmfPkgIa32.fdf @@ -288,10 +288,8 @@ [FV.DXEFV] INF FatPkg/EnhancedFatDxe/Fat.inf INF MdeModulePkg/Universal/Disk/UdfDxe/UdfDxe.inf -!if $(TOOL_CHAIN_TAG) != "XCODE5" INF ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf INF OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf -!endif INF ShellPkg/Application/Shell/Shell.inf INF MdeModulePkg/Logo/LogoDxe.inf diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc index b3ae62fee92b..e60d25db575e 100644 --- a/OvmfPkg/OvmfPkgIa32X64.dsc +++ b/OvmfPkg/OvmfPkgIa32X64.dsc @@ -892,7 +892,7 @@ [Components.X64] OvmfPkg/Csm/Csm16/Csm16.inf !endif -!if $(TOOL_CHAIN_TAG) != "XCODE5" + ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf { <PcdsFixedAtBuild> gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE @@ -901,7 +901,7 @@ [Components.X64] <PcdsFixedAtBuild> gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE } -!endif + ShellPkg/Application/Shell/Shell.inf { <LibraryClasses> ShellCommandLib|ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.inf diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf index bfca1eff9e83..4cd4dd652a9f 100644 --- a/OvmfPkg/OvmfPkgIa32X64.fdf +++ b/OvmfPkg/OvmfPkgIa32X64.fdf @@ -289,10 +289,8 @@ [FV.DXEFV] INF FatPkg/EnhancedFatDxe/Fat.inf INF MdeModulePkg/Universal/Disk/UdfDxe/UdfDxe.inf -!if $(TOOL_CHAIN_TAG) != "XCODE5" INF ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf INF OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf -!endif INF ShellPkg/Application/Shell/Shell.inf INF MdeModulePkg/Logo/LogoDxe.inf diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc index f7fe75ebf531..3b46bce5a6e0 100644 --- a/OvmfPkg/OvmfPkgX64.dsc +++ b/OvmfPkg/OvmfPkgX64.dsc @@ -888,7 +888,6 @@ [Components] OvmfPkg/Csm/Csm16/Csm16.inf !endif -!if $(TOOL_CHAIN_TAG) != "XCODE5" ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf { <PcdsFixedAtBuild> gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE @@ -897,7 +896,7 @@ [Components] <PcdsFixedAtBuild> gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE } -!endif + ShellPkg/Application/Shell/Shell.inf { <LibraryClasses> ShellCommandLib|ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.inf diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf index bfca1eff9e83..4cd4dd652a9f 100644 --- a/OvmfPkg/OvmfPkgX64.fdf +++ b/OvmfPkg/OvmfPkgX64.fdf @@ -289,10 +289,8 @@ [FV.DXEFV] INF FatPkg/EnhancedFatDxe/Fat.inf INF MdeModulePkg/Universal/Disk/UdfDxe/UdfDxe.inf -!if $(TOOL_CHAIN_TAG) != "XCODE5" INF ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf INF OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf -!endif INF ShellPkg/Application/Shell/Shell.inf INF MdeModulePkg/Logo/LogoDxe.inf diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc index 3af0ee705407..0c11d4732628 100644 --- a/OvmfPkg/OvmfXen.dsc +++ b/OvmfPkg/OvmfXen.dsc @@ -670,7 +670,6 @@ [Components] OvmfPkg/Csm/Csm16/Csm16.inf !endif -!if $(TOOL_CHAIN_TAG) != "XCODE5" ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf { <PcdsFixedAtBuild> gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE @@ -679,7 +678,7 @@ [Components] <PcdsFixedAtBuild> gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE } -!endif + ShellPkg/Application/Shell/Shell.inf { <LibraryClasses> ShellCommandLib|ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.inf diff --git a/OvmfPkg/OvmfXen.fdf b/OvmfPkg/OvmfXen.fdf index d9ee14b484a0..e81689292313 100644 --- a/OvmfPkg/OvmfXen.fdf +++ b/OvmfPkg/OvmfXen.fdf @@ -361,10 +361,8 @@ [FV.DXEFV] INF FatPkg/EnhancedFatDxe/Fat.inf INF MdeModulePkg/Universal/Disk/UdfDxe/UdfDxe.inf -!if $(TOOL_CHAIN_TAG) != "XCODE5" INF ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf INF OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf -!endif INF ShellPkg/Application/Shell/Shell.inf INF MdeModulePkg/Logo/LogoDxe.inf -- 2.24.1 (Apple Git-126) ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH 3/3] OvmfwPkg: Don't exclude XCODE Modules 2020-05-24 21:20 ` [PATCH 3/3] OvmfwPkg: Don't exclude XCODE Modules Andrew Fish @ 2020-05-25 19:31 ` Laszlo Ersek 2020-05-26 4:10 ` Andrew Fish 0 siblings, 1 reply; 9+ messages in thread From: Laszlo Ersek @ 2020-05-25 19:31 UTC (permalink / raw) To: devel, afish Cc: Ard Biesheuvel, Jiewen Yao, Jordan Justen, Philippe Mathieu-Daudé Hi Andrew, On 05/24/20 23:20, Andrew Fish via groups.io wrote: > With this BZ getting fixed we no longer need to special case XCODE. > > Cc: Ard Biesheuvel <ard.biesheuvel@arm.com> > Cc: Jiewen Yao <jiewen.yao@intel.com> > Cc: Jordan Justen <jordan.l.justen@intel.com> > Cc: Philippe Mathieu-Daudé <philmd@redhat.com> > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=557 > Signed-off-by: Andrew Fish <afish@apple.com> > > Signed-off-by: Andrew Fish <afish@apple.com> > --- > OvmfPkg/OvmfPkgIa32.dsc | 3 +-- > OvmfPkg/OvmfPkgIa32.fdf | 2 -- > OvmfPkg/OvmfPkgIa32X64.dsc | 4 ++-- > OvmfPkg/OvmfPkgIa32X64.fdf | 2 -- > OvmfPkg/OvmfPkgX64.dsc | 3 +-- > OvmfPkg/OvmfPkgX64.fdf | 2 -- > OvmfPkg/OvmfXen.dsc | 3 +-- > OvmfPkg/OvmfXen.fdf | 2 -- > 8 files changed, 5 insertions(+), 16 deletions(-) (1) Please run "BaseTools/Scripts/SetupGit.py" in your edk2 clone, because right now, the patch is formatted/posted with too many CR characters. (2) I'm not on the CC list -- is that an oversight, or should I have received the message directly due to some other means? (I've only found this message in my list folder). (3) As the diffstat above indicates it too, the "OvmfPkg/OvmfPkgIa32X64.dsc" update is inconsistent with the other DSC file updates, namely in whitespace. In the other DSC files, only one newline is inserted (just before Shell.inf), but in "OvmfPkgIa32X64.dsc", two newlines are inserted. One before Shell.inf, and another before "TftpDynamicCommand.inf". (4) There's a typo in the subject line of this patch; please replace "OvmfwPkg" with "OvmfPkg". Thank you! Laszlo > diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc > index d0df9cbbfb2b..8166d1588ae5 100644 > --- a/OvmfPkg/OvmfPkgIa32.dsc > +++ b/OvmfPkg/OvmfPkgIa32.dsc > @@ -878,7 +878,6 @@ [Components] > OvmfPkg/Csm/Csm16/Csm16.inf > > !endif > > > > -!if $(TOOL_CHAIN_TAG) != "XCODE5" > > ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf { > > <PcdsFixedAtBuild> > > gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE > > @@ -887,7 +886,7 @@ [Components] > <PcdsFixedAtBuild> > > gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE > > } > > -!endif > > + > > ShellPkg/Application/Shell/Shell.inf { > > <LibraryClasses> > > ShellCommandLib|ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.inf > > diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf > index e2b759aa8d05..cb1a8b1eba60 100644 > --- a/OvmfPkg/OvmfPkgIa32.fdf > +++ b/OvmfPkg/OvmfPkgIa32.fdf > @@ -288,10 +288,8 @@ [FV.DXEFV] > INF FatPkg/EnhancedFatDxe/Fat.inf > > INF MdeModulePkg/Universal/Disk/UdfDxe/UdfDxe.inf > > > > -!if $(TOOL_CHAIN_TAG) != "XCODE5" > > INF ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf > > INF OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf > > -!endif > > INF ShellPkg/Application/Shell/Shell.inf > > > > INF MdeModulePkg/Logo/LogoDxe.inf > > diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc > index b3ae62fee92b..e60d25db575e 100644 > --- a/OvmfPkg/OvmfPkgIa32X64.dsc > +++ b/OvmfPkg/OvmfPkgIa32X64.dsc > @@ -892,7 +892,7 @@ [Components.X64] > OvmfPkg/Csm/Csm16/Csm16.inf > > !endif > > > > -!if $(TOOL_CHAIN_TAG) != "XCODE5" > > + > > ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf { > > <PcdsFixedAtBuild> > > gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE > > @@ -901,7 +901,7 @@ [Components.X64] > <PcdsFixedAtBuild> > > gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE > > } > > -!endif > > + > > ShellPkg/Application/Shell/Shell.inf { > > <LibraryClasses> > > ShellCommandLib|ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.inf > > diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf > index bfca1eff9e83..4cd4dd652a9f 100644 > --- a/OvmfPkg/OvmfPkgIa32X64.fdf > +++ b/OvmfPkg/OvmfPkgIa32X64.fdf > @@ -289,10 +289,8 @@ [FV.DXEFV] > INF FatPkg/EnhancedFatDxe/Fat.inf > > INF MdeModulePkg/Universal/Disk/UdfDxe/UdfDxe.inf > > > > -!if $(TOOL_CHAIN_TAG) != "XCODE5" > > INF ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf > > INF OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf > > -!endif > > INF ShellPkg/Application/Shell/Shell.inf > > > > INF MdeModulePkg/Logo/LogoDxe.inf > > diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc > index f7fe75ebf531..3b46bce5a6e0 100644 > --- a/OvmfPkg/OvmfPkgX64.dsc > +++ b/OvmfPkg/OvmfPkgX64.dsc > @@ -888,7 +888,6 @@ [Components] > OvmfPkg/Csm/Csm16/Csm16.inf > > !endif > > > > -!if $(TOOL_CHAIN_TAG) != "XCODE5" > > ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf { > > <PcdsFixedAtBuild> > > gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE > > @@ -897,7 +896,7 @@ [Components] > <PcdsFixedAtBuild> > > gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE > > } > > -!endif > > + > > ShellPkg/Application/Shell/Shell.inf { > > <LibraryClasses> > > ShellCommandLib|ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.inf > > diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf > index bfca1eff9e83..4cd4dd652a9f 100644 > --- a/OvmfPkg/OvmfPkgX64.fdf > +++ b/OvmfPkg/OvmfPkgX64.fdf > @@ -289,10 +289,8 @@ [FV.DXEFV] > INF FatPkg/EnhancedFatDxe/Fat.inf > > INF MdeModulePkg/Universal/Disk/UdfDxe/UdfDxe.inf > > > > -!if $(TOOL_CHAIN_TAG) != "XCODE5" > > INF ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf > > INF OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf > > -!endif > > INF ShellPkg/Application/Shell/Shell.inf > > > > INF MdeModulePkg/Logo/LogoDxe.inf > > diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc > index 3af0ee705407..0c11d4732628 100644 > --- a/OvmfPkg/OvmfXen.dsc > +++ b/OvmfPkg/OvmfXen.dsc > @@ -670,7 +670,6 @@ [Components] > OvmfPkg/Csm/Csm16/Csm16.inf > > !endif > > > > -!if $(TOOL_CHAIN_TAG) != "XCODE5" > > ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf { > > <PcdsFixedAtBuild> > > gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE > > @@ -679,7 +678,7 @@ [Components] > <PcdsFixedAtBuild> > > gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE > > } > > -!endif > > + > > ShellPkg/Application/Shell/Shell.inf { > > <LibraryClasses> > > ShellCommandLib|ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.inf > > diff --git a/OvmfPkg/OvmfXen.fdf b/OvmfPkg/OvmfXen.fdf > index d9ee14b484a0..e81689292313 100644 > --- a/OvmfPkg/OvmfXen.fdf > +++ b/OvmfPkg/OvmfXen.fdf > @@ -361,10 +361,8 @@ [FV.DXEFV] > INF FatPkg/EnhancedFatDxe/Fat.inf > > INF MdeModulePkg/Universal/Disk/UdfDxe/UdfDxe.inf > > > > -!if $(TOOL_CHAIN_TAG) != "XCODE5" > > INF ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf > > INF OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf > > -!endif > > INF ShellPkg/Application/Shell/Shell.inf > > > > INF MdeModulePkg/Logo/LogoDxe.inf > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH 3/3] OvmfwPkg: Don't exclude XCODE Modules 2020-05-25 19:31 ` [edk2-devel] " Laszlo Ersek @ 2020-05-26 4:10 ` Andrew Fish 2020-05-26 11:45 ` Laszlo Ersek 0 siblings, 1 reply; 9+ messages in thread From: Andrew Fish @ 2020-05-26 4:10 UTC (permalink / raw) To: edk2-devel-groups-io, Laszlo Ersek Cc: Ard Biesheuvel, Jiewen Yao, Jordan Justen, Philippe Mathieu-Daudé [-- Attachment #1: Type: text/plain, Size: 8520 bytes --] > On May 25, 2020, at 12:31 PM, Laszlo Ersek <lersek@redhat.com> wrote: > > Hi Andrew, > > On 05/24/20 23:20, Andrew Fish via groups.io <http://groups.io/> wrote: >> With this BZ getting fixed we no longer need to special case XCODE. >> >> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com> >> Cc: Jiewen Yao <jiewen.yao@intel.com> >> Cc: Jordan Justen <jordan.l.justen@intel.com> >> Cc: Philippe Mathieu-Daudé <philmd@redhat.com> >> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=557 >> Signed-off-by: Andrew Fish <afish@apple.com> >> >> Signed-off-by: Andrew Fish <afish@apple.com> >> --- >> OvmfPkg/OvmfPkgIa32.dsc | 3 +-- >> OvmfPkg/OvmfPkgIa32.fdf | 2 -- >> OvmfPkg/OvmfPkgIa32X64.dsc | 4 ++-- >> OvmfPkg/OvmfPkgIa32X64.fdf | 2 -- >> OvmfPkg/OvmfPkgX64.dsc | 3 +-- >> OvmfPkg/OvmfPkgX64.fdf | 2 -- >> OvmfPkg/OvmfXen.dsc | 3 +-- >> OvmfPkg/OvmfXen.fdf | 2 -- >> 8 files changed, 5 insertions(+), 16 deletions(-) > Laszlo, Thanks for the feedback. Can I ask that you go to https://www.tianocore.org, click on How to Contribute and point me at the chain of links I did not follow, if I missed it tit is likely due to too many links and too much information being vended. When people are starting out we should vend them the instructions that work and let them opt in to learning more. > (1) Please run "BaseTools/Scripts/SetupGit.py" in your edk2 clone, > because right now, the patch is formatted/posted with too many CR > characters. > I filed https://bugzilla.tianocore.org/show_bug.cgi?id=2767 since I passed PatchCheck.py but did not run BaseTools/Scripts/SetupGit.py Thanks, Andrew Fish > (2) I'm not on the CC list -- is that an oversight, or should I have > received the message directly due to some other means? (I've only found > this message in my list folder). > > (3) As the diffstat above indicates it too, the > "OvmfPkg/OvmfPkgIa32X64.dsc" update is inconsistent with the other DSC > file updates, namely in whitespace. > > In the other DSC files, only one newline is inserted (just before > Shell.inf), but in "OvmfPkgIa32X64.dsc", two newlines are inserted. One > before Shell.inf, and another before "TftpDynamicCommand.inf". > > (4) There's a typo in the subject line of this patch; please replace > "OvmfwPkg" with "OvmfPkg". > > Thank you! > Laszlo > >> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc >> index d0df9cbbfb2b..8166d1588ae5 100644 >> --- a/OvmfPkg/OvmfPkgIa32.dsc >> +++ b/OvmfPkg/OvmfPkgIa32.dsc >> @@ -878,7 +878,6 @@ [Components] >> OvmfPkg/Csm/Csm16/Csm16.inf >> >> !endif >> >> >> >> -!if $(TOOL_CHAIN_TAG) != "XCODE5" >> >> ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf { >> >> <PcdsFixedAtBuild> >> >> gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE >> >> @@ -887,7 +886,7 @@ [Components] >> <PcdsFixedAtBuild> >> >> gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE >> >> } >> >> -!endif >> >> + >> >> ShellPkg/Application/Shell/Shell.inf { >> >> <LibraryClasses> >> >> ShellCommandLib|ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.inf >> >> diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf >> index e2b759aa8d05..cb1a8b1eba60 100644 >> --- a/OvmfPkg/OvmfPkgIa32.fdf >> +++ b/OvmfPkg/OvmfPkgIa32.fdf >> @@ -288,10 +288,8 @@ [FV.DXEFV] >> INF FatPkg/EnhancedFatDxe/Fat.inf >> >> INF MdeModulePkg/Universal/Disk/UdfDxe/UdfDxe.inf >> >> >> >> -!if $(TOOL_CHAIN_TAG) != "XCODE5" >> >> INF ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf >> >> INF OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf >> >> -!endif >> >> INF ShellPkg/Application/Shell/Shell.inf >> >> >> >> INF MdeModulePkg/Logo/LogoDxe.inf >> >> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc >> index b3ae62fee92b..e60d25db575e 100644 >> --- a/OvmfPkg/OvmfPkgIa32X64.dsc >> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc >> @@ -892,7 +892,7 @@ [Components.X64] >> OvmfPkg/Csm/Csm16/Csm16.inf >> >> !endif >> >> >> >> -!if $(TOOL_CHAIN_TAG) != "XCODE5" >> >> + >> >> ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf { >> >> <PcdsFixedAtBuild> >> >> gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE >> >> @@ -901,7 +901,7 @@ [Components.X64] >> <PcdsFixedAtBuild> >> >> gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE >> >> } >> >> -!endif >> >> + >> >> ShellPkg/Application/Shell/Shell.inf { >> >> <LibraryClasses> >> >> ShellCommandLib|ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.inf >> >> diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf >> index bfca1eff9e83..4cd4dd652a9f 100644 >> --- a/OvmfPkg/OvmfPkgIa32X64.fdf >> +++ b/OvmfPkg/OvmfPkgIa32X64.fdf >> @@ -289,10 +289,8 @@ [FV.DXEFV] >> INF FatPkg/EnhancedFatDxe/Fat.inf >> >> INF MdeModulePkg/Universal/Disk/UdfDxe/UdfDxe.inf >> >> >> >> -!if $(TOOL_CHAIN_TAG) != "XCODE5" >> >> INF ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf >> >> INF OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf >> >> -!endif >> >> INF ShellPkg/Application/Shell/Shell.inf >> >> >> >> INF MdeModulePkg/Logo/LogoDxe.inf >> >> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc >> index f7fe75ebf531..3b46bce5a6e0 100644 >> --- a/OvmfPkg/OvmfPkgX64.dsc >> +++ b/OvmfPkg/OvmfPkgX64.dsc >> @@ -888,7 +888,6 @@ [Components] >> OvmfPkg/Csm/Csm16/Csm16.inf >> >> !endif >> >> >> >> -!if $(TOOL_CHAIN_TAG) != "XCODE5" >> >> ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf { >> >> <PcdsFixedAtBuild> >> >> gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE >> >> @@ -897,7 +896,7 @@ [Components] >> <PcdsFixedAtBuild> >> >> gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE >> >> } >> >> -!endif >> >> + >> >> ShellPkg/Application/Shell/Shell.inf { >> >> <LibraryClasses> >> >> ShellCommandLib|ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.inf >> >> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf >> index bfca1eff9e83..4cd4dd652a9f 100644 >> --- a/OvmfPkg/OvmfPkgX64.fdf >> +++ b/OvmfPkg/OvmfPkgX64.fdf >> @@ -289,10 +289,8 @@ [FV.DXEFV] >> INF FatPkg/EnhancedFatDxe/Fat.inf >> >> INF MdeModulePkg/Universal/Disk/UdfDxe/UdfDxe.inf >> >> >> >> -!if $(TOOL_CHAIN_TAG) != "XCODE5" >> >> INF ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf >> >> INF OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf >> >> -!endif >> >> INF ShellPkg/Application/Shell/Shell.inf >> >> >> >> INF MdeModulePkg/Logo/LogoDxe.inf >> >> diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc >> index 3af0ee705407..0c11d4732628 100644 >> --- a/OvmfPkg/OvmfXen.dsc >> +++ b/OvmfPkg/OvmfXen.dsc >> @@ -670,7 +670,6 @@ [Components] >> OvmfPkg/Csm/Csm16/Csm16.inf >> >> !endif >> >> >> >> -!if $(TOOL_CHAIN_TAG) != "XCODE5" >> >> ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf { >> >> <PcdsFixedAtBuild> >> >> gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE >> >> @@ -679,7 +678,7 @@ [Components] >> <PcdsFixedAtBuild> >> >> gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE >> >> } >> >> -!endif >> >> + >> >> ShellPkg/Application/Shell/Shell.inf { >> >> <LibraryClasses> >> >> ShellCommandLib|ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.inf >> >> diff --git a/OvmfPkg/OvmfXen.fdf b/OvmfPkg/OvmfXen.fdf >> index d9ee14b484a0..e81689292313 100644 >> --- a/OvmfPkg/OvmfXen.fdf >> +++ b/OvmfPkg/OvmfXen.fdf >> @@ -361,10 +361,8 @@ [FV.DXEFV] >> INF FatPkg/EnhancedFatDxe/Fat.inf >> >> INF MdeModulePkg/Universal/Disk/UdfDxe/UdfDxe.inf >> >> >> >> -!if $(TOOL_CHAIN_TAG) != "XCODE5" >> >> INF ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf >> >> INF OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf >> >> -!endif >> >> INF ShellPkg/Application/Shell/Shell.inf >> >> >> >> INF MdeModulePkg/Logo/LogoDxe.inf >> > > > [-- Attachment #2: Type: text/html, Size: 31266 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH 3/3] OvmfwPkg: Don't exclude XCODE Modules 2020-05-26 4:10 ` Andrew Fish @ 2020-05-26 11:45 ` Laszlo Ersek 2020-05-27 6:05 ` Andrew Fish 0 siblings, 1 reply; 9+ messages in thread From: Laszlo Ersek @ 2020-05-26 11:45 UTC (permalink / raw) To: Andrew Fish, edk2-devel-groups-io Cc: Ard Biesheuvel, Jiewen Yao, Jordan Justen, Philippe Mathieu-Daudé On 05/26/20 06:10, Andrew Fish wrote: > > >> On May 25, 2020, at 12:31 PM, Laszlo Ersek <lersek@redhat.com> wrote: >> >> Hi Andrew, >> >> On 05/24/20 23:20, Andrew Fish via groups.io <http://groups.io/> wrote: >>> With this BZ getting fixed we no longer need to special case XCODE. >>> >>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com> >>> Cc: Jiewen Yao <jiewen.yao@intel.com> >>> Cc: Jordan Justen <jordan.l.justen@intel.com> >>> Cc: Philippe Mathieu-Daudé <philmd@redhat.com> >>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=557 >>> Signed-off-by: Andrew Fish <afish@apple.com> >>> >>> Signed-off-by: Andrew Fish <afish@apple.com> >>> --- >>> OvmfPkg/OvmfPkgIa32.dsc | 3 +-- >>> OvmfPkg/OvmfPkgIa32.fdf | 2 -- >>> OvmfPkg/OvmfPkgIa32X64.dsc | 4 ++-- >>> OvmfPkg/OvmfPkgIa32X64.fdf | 2 -- >>> OvmfPkg/OvmfPkgX64.dsc | 3 +-- >>> OvmfPkg/OvmfPkgX64.fdf | 2 -- >>> OvmfPkg/OvmfXen.dsc | 3 +-- >>> OvmfPkg/OvmfXen.fdf | 2 -- >>> 8 files changed, 5 insertions(+), 16 deletions(-) >> > > Laszlo, > > Thanks for the feedback. > > Can I ask that you go to https://www.tianocore.org, click on How to Contribute and point me at the chain of links I did not follow, if I missed it tit is likely due to too many links and too much information being vended. You didn't miss anything, starting from that page, I think. I don't remember ever clicking "How to Contribute" on that page. :) In fact if I grep the current edk2-wiki project source, at commit de8fae02bbcc ("Add acknowledgements page", 2020-05-21), I find no references to "SetupGit.py". > When people are starting out we should vend them the instructions that work and let them opt in to learning more. "Working instructions" is a moving target. When I wrote the unkempt guide, it was serious work, I had to set aside resources. When we changed the workflow to replace "git-push" (by maintainers) with github PRs (to trigger CI), documenting that was again serious work (for Mike -- Mike updated both the official workflow article and the unkempt guide, as I couldn't volunteer for the latter). The more documentation we add, the larger the burden to update them grows. It's an on-going commitment. Given the constant scarcity of developer (and reviewer) cycles, we can only choose between: - "code, plus more or less up-to-date docs", and - "code, plus no docs". Leif wrote SetupGit.py in the first place to save people the effort of going through some of the unkempt guide steps. If we decide that no SetupGit.py (or similar utilities) should be written without documenting them in the wiki, that won't force contributors to document their workflow-related contributions; it will cause them to not writing the utilities in the first place. Open source development communities teach contributors the workflow by doing. For example, I have contributed to two open source projects that are *exclusively* managed on github.com, using "github native" pull requests and such. (Namely, openssl, and "p11-glue/p11-kit".) I'm the kind of guy that very carefully reads the documentation first, and starts pushing the buttons only second, and I *still* got wrong my initial contributions to both projects. With OpenSSL, I missed details of how review worked and details about the CLA. Maintainers taught me those bits on the PR, while they were reviewing my code. With p11-kit, I had missed that I was expected to write a unit test at once, for the new code. Maintainers pointed that out in my PR. I posit that virtually no up-to-date technical documentation exists unless an organization treats that documentation as a *product* (with its own resource allocations, technical writers, subject matter expert reviewers, project managers, and so on). > >> (1) Please run "BaseTools/Scripts/SetupGit.py" in your edk2 clone, >> because right now, the patch is formatted/posted with too many CR >> characters. >> > > I filed https://bugzilla.tianocore.org/show_bug.cgi?id=2767 since I passed PatchCheck.py but did not run BaseTools/Scripts/SetupGit.py Thanks. For the record: I didn't reject your contribution (I'm very happy you posted a patch series); instead, I asked for an update. In particular, the number of empty lines inserted into the DSC files is not consistent across the OVMF DSC files, and that fact has nothing to do with git configuration -- it would need fixing identically even if the series had been submitted via a github.com PR. Thanks Laszlo ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH 3/3] OvmfwPkg: Don't exclude XCODE Modules 2020-05-26 11:45 ` Laszlo Ersek @ 2020-05-27 6:05 ` Andrew Fish 0 siblings, 0 replies; 9+ messages in thread From: Andrew Fish @ 2020-05-27 6:05 UTC (permalink / raw) To: edk2-devel-groups-io, lersek Cc: Ard Biesheuvel, Jiewen Yao, Jordan Justen, Philippe Mathieu-Daudé > On May 26, 2020, at 4:45 AM, Laszlo Ersek <lersek@redhat.com> wrote: > > On 05/26/20 06:10, Andrew Fish wrote: >> >> >>> On May 25, 2020, at 12:31 PM, Laszlo Ersek <lersek@redhat.com> wrote: >>> >>> Hi Andrew, >>> >>> On 05/24/20 23:20, Andrew Fish via groups.io <http://groups.io/> wrote: >>>> With this BZ getting fixed we no longer need to special case XCODE. >>>> >>>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com> >>>> Cc: Jiewen Yao <jiewen.yao@intel.com> >>>> Cc: Jordan Justen <jordan.l.justen@intel.com> >>>> Cc: Philippe Mathieu-Daudé <philmd@redhat.com> >>>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=557 >>>> Signed-off-by: Andrew Fish <afish@apple.com> >>>> >>>> Signed-off-by: Andrew Fish <afish@apple.com> >>>> --- >>>> OvmfPkg/OvmfPkgIa32.dsc | 3 +-- >>>> OvmfPkg/OvmfPkgIa32.fdf | 2 -- >>>> OvmfPkg/OvmfPkgIa32X64.dsc | 4 ++-- >>>> OvmfPkg/OvmfPkgIa32X64.fdf | 2 -- >>>> OvmfPkg/OvmfPkgX64.dsc | 3 +-- >>>> OvmfPkg/OvmfPkgX64.fdf | 2 -- >>>> OvmfPkg/OvmfXen.dsc | 3 +-- >>>> OvmfPkg/OvmfXen.fdf | 2 -- >>>> 8 files changed, 5 insertions(+), 16 deletions(-) >>> >> >> Laszlo, >> >> Thanks for the feedback. >> >> Can I ask that you go to https://www.tianocore.org, click on How to Contribute and point me at the chain of links I did not follow, if I missed it tit is likely due to too many links and too much information being vended. > > You didn't miss anything, starting from that page, I think. I don't > remember ever clicking "How to Contribute" on that page. :) > > In fact if I grep the current edk2-wiki project source, at commit > de8fae02bbcc ("Add acknowledgements page", 2020-05-21), I find no > references to "SetupGit.py". > >> When people are starting out we should vend them the instructions that work and let them opt in to learning more. > > "Working instructions" is a moving target. When I wrote the unkempt > guide, it was serious work, I had to set aside resources. When we > changed the workflow to replace "git-push" (by maintainers) with github > PRs (to trigger CI), documenting that was again serious work (for Mike > -- Mike updated both the official workflow article and the unkempt > guide, as I couldn't volunteer for the latter). > > The more documentation we add, the larger the burden to update them > grows. It's an on-going commitment. Given the constant scarcity of > developer (and reviewer) cycles, we can only choose between: > > - "code, plus more or less up-to-date docs", and > - "code, plus no docs". > > Leif wrote SetupGit.py in the first place to save people the effort of > going through some of the unkempt guide steps. > > If we decide that no SetupGit.py (or similar utilities) should be > written without documenting them in the wiki, that won't force > contributors to document their workflow-related contributions; it will > cause them to not writing the utilities in the first place. > > Open source development communities teach contributors the workflow by > doing. For example, I have contributed to two open source projects that > are *exclusively* managed on github.com, using "github native" pull > requests and such. (Namely, openssl, and "p11-glue/p11-kit".) I'm the > kind of guy that very carefully reads the documentation first, and > starts pushing the buttons only second, and I *still* got wrong my > initial contributions to both projects. > > With OpenSSL, I missed details of how review worked and details about > the CLA. Maintainers taught me those bits on the PR, while they were > reviewing my code. > > With p11-kit, I had missed that I was expected to write a unit test at > once, for the new code. Maintainers pointed that out in my PR. > > I posit that virtually no up-to-date technical documentation exists > unless an organization treats that documentation as a *product* (with > its own resource allocations, technical writers, subject matter expert > reviewers, project managers, and so on). > >> >>> (1) Please run "BaseTools/Scripts/SetupGit.py" in your edk2 clone, >>> because right now, the patch is formatted/posted with too many CR >>> characters. >>> >> >> I filed https://bugzilla.tianocore.org/show_bug.cgi?id=2767 since I passed PatchCheck.py but did not run BaseTools/Scripts/SetupGit.py > > Thanks. > > For the record: I didn't reject your contribution (I'm very happy you > posted a patch series); instead, I asked for an update. > Lazlo, I understand that is just a tooling issue on my side. Sorry for my delay in fixing up the patches but I have some higher priority work PRs I need to get resolved and that is delaying me fixing up the patch set. At my work we have a tradition of trying to update our documentation when we onboard new people as that new set of eyes helps point out the small things that could have saved people a lot of time. So I'm not so much complaining, but just trying to help. > In particular, the number of empty lines inserted into the DSC files is > not consistent across the OVMF DSC files, and that fact has nothing to > do with git configuration -- it would need fixing identically even if > the series had been submitted via a github.com PR. > My brain is a little dyslexic so I tend to miss symmetry things like this when I review my own work, but that is why we review the code. Thanks, Andrew Fish > Thanks > Laszlo > > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-05-27 6:05 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-05-24 21:20 [PATCH 0/3] Add PE/COFF resource sections support for XCODE Andrew Fish 2020-05-24 21:20 ` [PATCH 1/3] BaseTools/GenFv: Add PE/COFF resource sections injection to GenFw Andrew Fish 2020-05-27 3:15 ` Bob Feng 2020-05-24 21:20 ` [PATCH 2/3] BaseTools: Add PE/COFF resource sections support for XCODE Andrew Fish 2020-05-24 21:20 ` [PATCH 3/3] OvmfwPkg: Don't exclude XCODE Modules Andrew Fish 2020-05-25 19:31 ` [edk2-devel] " Laszlo Ersek 2020-05-26 4:10 ` Andrew Fish 2020-05-26 11:45 ` Laszlo Ersek 2020-05-27 6:05 ` Andrew Fish
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox