[PATCH 1/3] BaseTools/Conf/tools_def: Fix linking using CLANGDWARF_IA32_X64

[PATCH 1/3] BaseTools/Conf/tools_def: Fix linking using CLANGDWARF_IA32_X64

[Patrick Rudolph]

The clang toolchain might default to fPIE/fPIC, which prevents
lld from linking the objects into a binary.

Specify -fno-pie -fno-pic as done on GCC to fix linking.

Test:
Building the Universal Payload using the command
'python UefiPayloadPkg/UniversalPayloadBuild.py -a IA32' actually
works.

Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=4356
---
 BaseTools/Conf/tools_def.template | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template
index 471eb67c0c..9b59bd75c3 100755
--- a/BaseTools/Conf/tools_def.template
+++ b/BaseTools/Conf/tools_def.template
@@ -2888,7 +2888,7 @@ DEFINE CLANGDWARF_X64_DLINK2_FLAGS        = -Wl,--defsym=PECOFF_HEADER_SIZE=0x22
 *_CLANGDWARF_IA32_RC_PATH              = DEF(CLANGDWARF_IA32_PREFIX)llvm-rc
 
 *_CLANGDWARF_IA32_ASLCC_FLAGS          = DEF(GCC_ASLCC_FLAGS) -m32 -fno-lto DEF(CLANG38_IA32_TARGET)
-*_CLANGDWARF_IA32_ASLDLINK_FLAGS       = DEF(CLANGDWARF_IA32_X64_ASLDLINK_FLAGS) -Wl,-m,elf_i386 -fuse-ld=lld
+*_CLANGDWARF_IA32_ASLDLINK_FLAGS       = DEF(CLANGDWARF_IA32_X64_ASLDLINK_FLAGS) -Wl,-m,elf_i386 -fuse-ld=lld -no-pie
 *_CLANGDWARF_IA32_ASM_FLAGS            = DEF(GCC5_ASM_FLAGS) -m32 -march=i386 DEF(CLANG38_IA32_TARGET)
 *_CLANGDWARF_IA32_RC_FLAGS             = DEF(GCC_IA32_RC_FLAGS)
 *_CLANGDWARF_IA32_OBJCOPY_FLAGS        =
@@ -2897,17 +2897,17 @@ DEFINE CLANGDWARF_X64_DLINK2_FLAGS        = -Wl,--defsym=PECOFF_HEADER_SIZE=0x22
 *_CLANGDWARF_IA32_ASLPP_FLAGS          = DEF(GCC_ASLPP_FLAGS) DEF(CLANG38_IA32_TARGET)
 *_CLANGDWARF_IA32_VFRPP_FLAGS          = DEF(GCC_VFRPP_FLAGS) DEF(CLANG38_IA32_TARGET)
 
-DEBUG_CLANGDWARF_IA32_CC_FLAGS         = DEF(CLANG38_ALL_CC_FLAGS) -m32 -Oz -flto -march=i586 DEF(CLANG38_IA32_TARGET) -g -malign-double
+DEBUG_CLANGDWARF_IA32_CC_FLAGS         = DEF(CLANG38_ALL_CC_FLAGS) -fno-pic -fno-pie -m32 -Oz -flto -march=i586 DEF(CLANG38_IA32_TARGET) -g -malign-double
 DEBUG_CLANGDWARF_IA32_DLINK_FLAGS      = DEF(CLANGDWARF_IA32_X64_DLINK_FLAGS) -flto -Wl,-O3 -Wl,-melf_i386 -Wl,--oformat,elf32-i386
-DEBUG_CLANGDWARF_IA32_DLINK2_FLAGS     = DEF(CLANGDWARF_IA32_DLINK2_FLAGS) -O3 -fuse-ld=lld
+DEBUG_CLANGDWARF_IA32_DLINK2_FLAGS     = DEF(CLANGDWARF_IA32_DLINK2_FLAGS) -O3 -fuse-ld=lld -no-pie
 
-RELEASE_CLANGDWARF_IA32_CC_FLAGS       = DEF(CLANG38_ALL_CC_FLAGS) -m32 -Oz -flto -march=i586 DEF(CLANG38_IA32_TARGET) -malign-double
+RELEASE_CLANGDWARF_IA32_CC_FLAGS       = DEF(CLANG38_ALL_CC_FLAGS) -fno-pic -fno-pie -m32 -Oz -flto -march=i586 DEF(CLANG38_IA32_TARGET) -malign-double
 RELEASE_CLANGDWARF_IA32_DLINK_FLAGS    = DEF(CLANGDWARF_IA32_X64_DLINK_FLAGS) -flto -Wl,-O3 -Wl,-melf_i386 -Wl,--oformat,elf32-i386
-RELEASE_CLANGDWARF_IA32_DLINK2_FLAGS   = DEF(CLANGDWARF_IA32_DLINK2_FLAGS) -O3 -fuse-ld=lld
+RELEASE_CLANGDWARF_IA32_DLINK2_FLAGS   = DEF(CLANGDWARF_IA32_DLINK2_FLAGS) -O3 -fuse-ld=lld -no-pie
 
-NOOPT_CLANGDWARF_IA32_CC_FLAGS         = DEF(CLANG38_ALL_CC_FLAGS) -m32 -O0 -march=i586 DEF(CLANG38_IA32_TARGET) -g -malign-double
+NOOPT_CLANGDWARF_IA32_CC_FLAGS         = DEF(CLANG38_ALL_CC_FLAGS) -fno-pic -fno-pie -m32 -O0 -march=i586 DEF(CLANG38_IA32_TARGET) -g -malign-double
 NOOPT_CLANGDWARF_IA32_DLINK_FLAGS      = DEF(CLANGDWARF_IA32_X64_DLINK_FLAGS) -Wl,-O0 -Wl,-melf_i386 -Wl,--oformat,elf32-i386
-NOOPT_CLANGDWARF_IA32_DLINK2_FLAGS     = DEF(CLANGDWARF_IA32_DLINK2_FLAGS) -O0 -fuse-ld=lld
+NOOPT_CLANGDWARF_IA32_DLINK2_FLAGS     = DEF(CLANGDWARF_IA32_DLINK2_FLAGS) -O0 -fuse-ld=lld -no-pie
 
 ##########################
 # CLANGDWARF X64 definitions
-- 
2.39.1

[PATCH 2/3] BaseTools/Conf/tools_def: Fix CLANGDWARF_IA32_X64

[Patrick Rudolph]

Drop the "-z max-page-size=0x40" option as it causes the ELF
header to overflow into the .text section, causing undefined
behaviour.

With high optimization level it corrupts essential code and
the binary would crash. It might work with low optimization
level though. As the default is to use Oz and LTO, it always
crashes.

Test:
The ELF generated by
'python UefiPayloadPkg/UniversalPayloadBuild.py -a IA32' boots.

Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=4357
---
 BaseTools/Conf/tools_def.template | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template
index 9b59bd75c3..0c584ab390 100755
--- a/BaseTools/Conf/tools_def.template
+++ b/BaseTools/Conf/tools_def.template
@@ -2866,7 +2866,7 @@ DEFINE CLANGDWARF_X64_PREFIX        = ENV(CLANG_BIN)
 
 # LLVM/CLANG doesn't support -n link option. So, it can't share the same IA32_X64_DLINK_COMMON flag.
 # LLVM/CLANG doesn't support common page size. So, it can't share the same GccBase.lds script.
-DEFINE CLANGDWARF_IA32_X64_DLINK_COMMON   = -nostdlib -Wl,-q,--gc-sections -z max-page-size=0x40
+DEFINE CLANGDWARF_IA32_X64_DLINK_COMMON   = -nostdlib -Wl,-q,--gc-sections
 DEFINE CLANGDWARF_DLINK2_FLAGS_COMMON     = -Wl,--script=$(EDK_TOOLS_PATH)/Scripts/ClangBase.lds
 DEFINE CLANGDWARF_IA32_X64_ASLDLINK_FLAGS = DEF(CLANGDWARF_IA32_X64_DLINK_COMMON) -Wl,--defsym=PECOFF_HEADER_SIZE=0 DEF(CLANGDWARF_DLINK2_FLAGS_COMMON) -Wl,--entry,ReferenceAcpiTable -u ReferenceAcpiTable
 DEFINE CLANGDWARF_IA32_X64_DLINK_FLAGS    = DEF(CLANGDWARF_IA32_X64_DLINK_COMMON) -Wl,--entry,$(IMAGE_ENTRY_POINT) -u $(IMAGE_ENTRY_POINT) -Wl,-Map,$(DEST_DIR_DEBUG)/$(BASE_NAME).map,--whole-archive
-- 
2.39.1

[PATCH 3/3] ShellPkg/TftpDynamicCommand.inf: Add missing DEPEX

[Patrick Rudolph]

Add protocol gEfiHiiPackageListProtocolGuid to DEPEX
to make sure it's present before using it.

Fixes ASSERTION seen on DEBUG build.

Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
---
 ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf b/ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
index b0c8e8f84b..d0d849a1eb 100644
--- a/ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
+++ b/ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
@@ -57,4 +57,4 @@
   gEfiShellDynamicCommandProtocolGuid            ## PRODUCES
 
 [DEPEX]
-  TRUE
+  gEfiHiiPackageListProtocolGuid
-- 
2.39.1

Re: [PATCH 2/3] BaseTools/Conf/tools_def: Fix CLANGDWARF_IA32_X64

[Sean Rhodes]

[-- Attachment #1: Type: text/plain, Size: 2016 bytes --]

Reviewed-by: Sean Rhodes <sean@starlabs.systems>

On Mon, 6 Mar 2023 at 08:38, Patrick Rudolph <patrick.rudolph@9elements.com>
wrote:

> Drop the "-z max-page-size=0x40" option as it causes the ELF
> header to overflow into the .text section, causing undefined
> behaviour.
>
> With high optimization level it corrupts essential code and
> the binary would crash. It might work with low optimization
> level though. As the default is to use Oz and LTO, it always
> crashes.
>
> Test:
> The ELF generated by
> 'python UefiPayloadPkg/UniversalPayloadBuild.py -a IA32' boots.
>
> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=4357
> ---
>  BaseTools/Conf/tools_def.template | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/BaseTools/Conf/tools_def.template
> b/BaseTools/Conf/tools_def.template
> index 9b59bd75c3..0c584ab390 100755
> --- a/BaseTools/Conf/tools_def.template
> +++ b/BaseTools/Conf/tools_def.template
> @@ -2866,7 +2866,7 @@ DEFINE CLANGDWARF_X64_PREFIX        = ENV(CLANG_BIN)
>
>  # LLVM/CLANG doesn't support -n link option. So, it can't share the same
> IA32_X64_DLINK_COMMON flag.
>  # LLVM/CLANG doesn't support common page size. So, it can't share the
> same GccBase.lds script.
> -DEFINE CLANGDWARF_IA32_X64_DLINK_COMMON   = -nostdlib
> -Wl,-q,--gc-sections -z max-page-size=0x40
> +DEFINE CLANGDWARF_IA32_X64_DLINK_COMMON   = -nostdlib -Wl,-q,--gc-sections
>  DEFINE CLANGDWARF_DLINK2_FLAGS_COMMON     =
> -Wl,--script=$(EDK_TOOLS_PATH)/Scripts/ClangBase.lds
>  DEFINE CLANGDWARF_IA32_X64_ASLDLINK_FLAGS =
> DEF(CLANGDWARF_IA32_X64_DLINK_COMMON) -Wl,--defsym=PECOFF_HEADER_SIZE=0
> DEF(CLANGDWARF_DLINK2_FLAGS_COMMON) -Wl,--entry,ReferenceAcpiTable -u
> ReferenceAcpiTable
>  DEFINE CLANGDWARF_IA32_X64_DLINK_FLAGS    =
> DEF(CLANGDWARF_IA32_X64_DLINK_COMMON) -Wl,--entry,$(IMAGE_ENTRY_POINT) -u
> $(IMAGE_ENTRY_POINT)
> -Wl,-Map,$(DEST_DIR_DEBUG)/$(BASE_NAME).map,--whole-archive
> --
> 2.39.1
>
>

[-- Attachment #2: Type: text/html, Size: 2706 bytes --]

Re: [PATCH 3/3] ShellPkg/TftpDynamicCommand.inf: Add missing DEPEX

[Sean Rhodes]

[-- Attachment #1: Type: text/plain, Size: 976 bytes --]

Reviewed-by: Sean Rhodes <sean@starlabs.systems>

On Mon, 6 Mar 2023 at 08:38, Patrick Rudolph <patrick.rudolph@9elements.com>
wrote:

> Add protocol gEfiHiiPackageListProtocolGuid to DEPEX
> to make sure it's present before using it.
>
> Fixes ASSERTION seen on DEBUG build.
>
> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> ---
>  ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git
> a/ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
> b/ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
> index b0c8e8f84b..d0d849a1eb 100644
> --- a/ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
> +++ b/ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
> @@ -57,4 +57,4 @@
>    gEfiShellDynamicCommandProtocolGuid            ## PRODUCES
>
>  [DEPEX]
> -  TRUE
> +  gEfiHiiPackageListProtocolGuid
> --
> 2.39.1
>
>

[-- Attachment #2: Type: text/html, Size: 1511 bytes --]

Re: [edk2-devel] [PATCH 1/3] BaseTools/Conf/tools_def: Fix linking using CLANGDWARF_IA32_X64

[Sheng Lean Tan]

[-- Attachment #1: Type: text/plain, Size: 56 bytes --]

Hi all,
Can someone please help to review this?
Thanks.

[-- Attachment #2: Type: text/html, Size: 108 bytes --]

Re: [edk2-devel] [PATCH 1/3] BaseTools/Conf/tools_def: Fix linking using CLANGDWARF_IA32_X64

[Sheng Lean Tan]

[-- Attachment #1: Type: text/plain, Size: 228 bytes --]

I think the right reviewer was not added.
Added them.

On Wed, 15 Mar 2023 at 17:46, Sheng Lean Tan via groups.io <sheng.tan=
9elements.com@groups.io> wrote:

> Hi all,
> Can someone please help to review this?
> Thanks.
> 
>
>

[-- Attachment #2: Type: text/html, Size: 798 bytes --]

Re: [edk2-devel] [PATCH 2/3] BaseTools/Conf/tools_def: Fix CLANGDWARF_IA32_X64

[Sheng Lean Tan]

[-- Attachment #1: Type: text/plain, Size: 28 bytes --]

adding the right reviewers.

[-- Attachment #2: Type: text/html, Size: 49 bytes --]

Re: [edk2-devel] [PATCH 3/3] ShellPkg/TftpDynamicCommand.inf: Add missing DEPEX

[Sheng Lean Tan]

[-- Attachment #1: Type: text/plain, Size: 15 bytes --]

Added Zhichao.

[-- Attachment #2: Type: text/html, Size: 47 bytes --]

Re: [edk2-devel] [PATCH 1/3] BaseTools/Conf/tools_def: Fix linking using CLANGDWARF_IA32_X64

[Marvin Häuser]

[-- Attachment #1: Type: text/plain, Size: 143 bytes --]

Hi,

Why does the title mention X64? From what I can see, PIE is unaffected for X64 (and we really want it to be).

Best regards,
Marvin

[-- Attachment #2: Type: text/html, Size: 163 bytes --]

Re: [edk2-devel] [PATCH 1/3] BaseTools/Conf/tools_def: Fix linking using CLANGDWARF_IA32_X64

[Ard Biesheuvel]

On Wed, 15 Mar 2023 at 23:16, Marvin Häuser <mhaeuser@posteo.de> wrote:
>
> Hi,
>
> Why does the title mention X64? From what I can see, PIE is unaffected for X64 (and we really want it to be).
>

Why?

Re: [edk2-devel] [PATCH 1/3] BaseTools/Conf/tools_def: Fix linking using CLANGDWARF_IA32_X64

[Marvin Häuser]

> On 15. Mar 2023, at 23:51, Ard Biesheuvel <ardb@kernel.org> wrote:
> 
> On Wed, 15 Mar 2023 at 23:16, Marvin Häuser <mhaeuser@posteo.de> wrote:
>> 
>> Hi,
>> 
>> Why does the title mention X64? From what I can see, PIE is unaffected for X64 (and we really want it to be).
>> 
> 
> Why?

Why what? By “PIE is unaffected for X64” I meant by the patch (which only changes IA32 macros, no?). I can also see how the last part is a bit ambiguous, but I meant it literally - we really want X64 to be [unaffected] (which it is, by the patch, but the title implies otherwise, doesn’t it?).

Best regards,
Marvin

Re: [edk2-devel] [PATCH 1/3] BaseTools/Conf/tools_def: Fix linking using CLANGDWARF_IA32_X64

[Ard Biesheuvel]

On Wed, 15 Mar 2023 at 23:57, Marvin Häuser <mhaeuser@posteo.de> wrote:
>
>
> > On 15. Mar 2023, at 23:51, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Wed, 15 Mar 2023 at 23:16, Marvin Häuser <mhaeuser@posteo.de> wrote:
> >>
> >> Hi,
> >>
> >> Why does the title mention X64? From what I can see, PIE is unaffected for X64 (and we really want it to be).
> >>
> >
> > Why?
>
> Why what? By “PIE is unaffected for X64” I meant by the patch (which only changes IA32 macros, no?). I can also see how the last part is a bit ambiguous, but I meant it literally - we really want X64 to be [unaffected] (which it is, by the patch, but the title implies otherwise, doesn’t it?).
>

OK, i got confused. Thanks for clearing that up for me :-)

Re: [edk2-devel] [PATCH 3/3] ShellPkg/TftpDynamicCommand.inf: Add missing DEPEX

[Gao, Zhichao]

[-- Attachment #1: Type: text/plain, Size: 940 bytes --]

Did you make sure the change would really fix the issue?
Just add the DEPX would only make sure the protocol is installed but not installed to the image handle. The return value would be still error code.
And the issue is still there.

The HII image related protocol should not be the MUST have for the application. Using the DEBUG ASSERT is not appropriate. I would suggest to remove the ASSERT instead.

Thanks,
Zhichao

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sheng Lean Tan
Sent: Thursday, March 16, 2023 12:54 AM
To: devel@edk2.groups.io; Rudolph, Patrick <patrick.rudolph@9elements.com>; Gao, Zhichao <zhichao.gao@intel.com>
Cc: Dong, Guo <guo.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Rhodes, Sean <sean@starlabs.systems>; Lu, James <james.lu@intel.com>; Guo, Gua <gua.guo@intel.com>
Subject: Re: [edk2-devel] [PATCH 3/3] ShellPkg/TftpDynamicCommand.inf: Add missing DEPEX

Added Zhichao.


[-- Attachment #2: Type: text/html, Size: 3500 bytes --]

[PATCH 2/3] BaseTools/Conf/tools_def: Fix CLANGDWARF_IA32_X64

[Patrick Rudolph]

Drop the "-z max-page-size=0x40" option as it causes the ELF
header to overflow into the .text section, causing undefined
behaviour.

With high optimization level it corrupts essential code and
the binary would crash. It might work with low optimization
level though. As the default is to use Oz and LTO, it always
crashes.

Test:
The ELF generated by
'python UefiPayloadPkg/UniversalPayloadBuild.py -a IA32' boots.

Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=4357
---
 BaseTools/Conf/tools_def.template | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template
index 9b59bd75c3..0c584ab390 100755
--- a/BaseTools/Conf/tools_def.template
+++ b/BaseTools/Conf/tools_def.template
@@ -2866,7 +2866,7 @@ DEFINE CLANGDWARF_X64_PREFIX        = ENV(CLANG_BIN)
 
 # LLVM/CLANG doesn't support -n link option. So, it can't share the same IA32_X64_DLINK_COMMON flag.
 # LLVM/CLANG doesn't support common page size. So, it can't share the same GccBase.lds script.
-DEFINE CLANGDWARF_IA32_X64_DLINK_COMMON   = -nostdlib -Wl,-q,--gc-sections -z max-page-size=0x40
+DEFINE CLANGDWARF_IA32_X64_DLINK_COMMON   = -nostdlib -Wl,-q,--gc-sections
 DEFINE CLANGDWARF_DLINK2_FLAGS_COMMON     = -Wl,--script=$(EDK_TOOLS_PATH)/Scripts/ClangBase.lds
 DEFINE CLANGDWARF_IA32_X64_ASLDLINK_FLAGS = DEF(CLANGDWARF_IA32_X64_DLINK_COMMON) -Wl,--defsym=PECOFF_HEADER_SIZE=0 DEF(CLANGDWARF_DLINK2_FLAGS_COMMON) -Wl,--entry,ReferenceAcpiTable -u ReferenceAcpiTable
 DEFINE CLANGDWARF_IA32_X64_DLINK_FLAGS    = DEF(CLANGDWARF_IA32_X64_DLINK_COMMON) -Wl,--entry,$(IMAGE_ENTRY_POINT) -u $(IMAGE_ENTRY_POINT) -Wl,-Map,$(DEST_DIR_DEBUG)/$(BASE_NAME).map,--whole-archive
-- 
2.39.1

Re: [edk2-devel] [PATCH 1/3] BaseTools/Conf/tools_def: Fix linking using CLANGDWARF_IA32_X64

[Patrick Rudolph]

Fixed the title.
Sorry for the confusion.

On Thu, Mar 16, 2023 at 1:05 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Wed, 15 Mar 2023 at 23:57, Marvin Häuser <mhaeuser@posteo.de> wrote:
> >
> >
> > > On 15. Mar 2023, at 23:51, Ard Biesheuvel <ardb@kernel.org> wrote:
> > >
> > > On Wed, 15 Mar 2023 at 23:16, Marvin Häuser <mhaeuser@posteo.de> wrote:
> > >>
> > >> Hi,
> > >>
> > >> Why does the title mention X64? From what I can see, PIE is unaffected for X64 (and we really want it to be).
> > >>
> > >
> > > Why?
> >
> > Why what? By “PIE is unaffected for X64” I meant by the patch (which only changes IA32 macros, no?). I can also see how the last part is a bit ambiguous, but I meant it literally - we really want X64 to be [unaffected] (which it is, by the patch, but the title implies otherwise, doesn’t it?).
> >
>
> OK, i got confused. Thanks for clearing that up for me :-)

Re: [PATCH 2/3] BaseTools/Conf/tools_def: Fix CLANGDWARF_IA32_X64

[Ni, Ray]

Why ELF header overflows into .text section?

> -----Original Message-----
> From: Patrick Rudolph <patrick.rudolph@9elements.com>
> Sent: Friday, March 17, 2023 10:06 PM
> Cc: devel@edk2.groups.io; Dong, Guo <guo.dong@intel.com>; Guo, Gua
> <gua.guo@intel.com>; Lu, James <james.lu@intel.com>; Ni, Ray
> <ray.ni@intel.com>; mhaeuser@posteo.de; ardb@kernel.org
> Subject: [PATCH 2/3] BaseTools/Conf/tools_def: Fix
> CLANGDWARF_IA32_X64
> 
> Drop the "-z max-page-size=0x40" option as it causes the ELF
> header to overflow into the .text section, causing undefined
> behaviour.
> 
> With high optimization level it corrupts essential code and
> the binary would crash. It might work with low optimization
> level though. As the default is to use Oz and LTO, it always
> crashes.
> 
> Test:
> The ELF generated by
> 'python UefiPayloadPkg/UniversalPayloadBuild.py -a IA32' boots.
> 
> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=4357
> ---
>  BaseTools/Conf/tools_def.template | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/BaseTools/Conf/tools_def.template
> b/BaseTools/Conf/tools_def.template
> index 9b59bd75c3..0c584ab390 100755
> --- a/BaseTools/Conf/tools_def.template
> +++ b/BaseTools/Conf/tools_def.template
> @@ -2866,7 +2866,7 @@ DEFINE CLANGDWARF_X64_PREFIX        =
> ENV(CLANG_BIN)
> 
> 
>  # LLVM/CLANG doesn't support -n link option. So, it can't share the same
> IA32_X64_DLINK_COMMON flag.
> 
>  # LLVM/CLANG doesn't support common page size. So, it can't share the
> same GccBase.lds script.
> 
> -DEFINE CLANGDWARF_IA32_X64_DLINK_COMMON   = -nostdlib -Wl,-q,--gc-
> sections -z max-page-size=0x40
> 
> +DEFINE CLANGDWARF_IA32_X64_DLINK_COMMON   = -nostdlib -Wl,-q,--
> gc-sections
> 
>  DEFINE CLANGDWARF_DLINK2_FLAGS_COMMON     = -Wl,--
> script=$(EDK_TOOLS_PATH)/Scripts/ClangBase.lds
> 
>  DEFINE CLANGDWARF_IA32_X64_ASLDLINK_FLAGS =
> DEF(CLANGDWARF_IA32_X64_DLINK_COMMON) -Wl,--
> defsym=PECOFF_HEADER_SIZE=0
> DEF(CLANGDWARF_DLINK2_FLAGS_COMMON) -Wl,--
> entry,ReferenceAcpiTable -u ReferenceAcpiTable
> 
>  DEFINE CLANGDWARF_IA32_X64_DLINK_FLAGS    =
> DEF(CLANGDWARF_IA32_X64_DLINK_COMMON) -Wl,--
> entry,$(IMAGE_ENTRY_POINT) -u $(IMAGE_ENTRY_POINT) -Wl,-
> Map,$(DEST_DIR_DEBUG)/$(BASE_NAME).map,--whole-archive
> 
> --
> 2.39.1

Re: [PATCH 2/3] BaseTools/Conf/tools_def: Fix CLANGDWARF_IA32_X64

[Marvin Häuser]

[-- Attachment #1: Type: text/plain, Size: 4217 bytes --]


> On 31. Mar 2023, at 16:41, Ni, Ray <ray.ni@intel.com> wrote:
> 
> Why ELF header overflows into .text section?

That's a good question, isn't it? :)

From what I can see, these binaries don't pass post-processing like GenFw or such. GCC (and I think thus CLANGDWARF?) gets an extra objcopy step as part of linking [2], but the arguments are empty [3] and thus should be no-op (I hope?).

I suppose potential candidates are:

1) A bug in the LLD linker used by CLANGDWARF for IA32 and X64. That would be very surprising to me, especially as no other platform reported issues and LLD is well-established. But who knows, generally ELFs will have large alignment values compared to the 64 Bytes used by edk2.

2) A bug in llvm-objcopy used by UniversalPayloadBuild.py [1]. I'm honestly unfamiliar with objcopy variants and their quality/reliability.

3) A bug in the llvm-objcopy or CLANGDWARF tools_def commands on the edk2 side of things.

Some may disagree, but I would reduce 3) to either 1) or 2). I think even if the commands malformed and this causes the overflow, I believe LLD or objcopy should issue a warning regardless.

As I have no way to reproduce the issue, I cannot really help further, sorry.

Best regards,
Marvin

[1]
https://github.com/tianocore/edk2/blob/b08a19eae28e76fb5a296a604c27d06fab29b08a/UefiPayloadPkg/UniversalPayloadBuild.py#L163-L183

[2]
https://github.com/tianocore/edk2/blob/b08a19eae28e76fb5a296a604c27d06fab29b08a/BaseTools/Conf/build_rule.template#L298

[3]
https://github.com/tianocore/edk2/blob/b08a19eae28e76fb5a296a604c27d06fab29b08a/BaseTools/Conf/tools_def.template#L2895
https://github.com/tianocore/edk2/blob/b08a19eae28e76fb5a296a604c27d06fab29b08a/BaseTools/Conf/tools_def.template#L2931

> 
>> -----Original Message-----
>> From: Patrick Rudolph <patrick.rudolph@9elements.com>
>> Sent: Friday, March 17, 2023 10:06 PM
>> Cc: devel@edk2.groups.io; Dong, Guo <guo.dong@intel.com>; Guo, Gua
>> <gua.guo@intel.com>; Lu, James <james.lu@intel.com>; Ni, Ray
>> <ray.ni@intel.com>; mhaeuser@posteo.de; ardb@kernel.org
>> Subject: [PATCH 2/3] BaseTools/Conf/tools_def: Fix
>> CLANGDWARF_IA32_X64
>> 
>> Drop the "-z max-page-size=0x40" option as it causes the ELF
>> header to overflow into the .text section, causing undefined
>> behaviour.
>> 
>> With high optimization level it corrupts essential code and
>> the binary would crash. It might work with low optimization
>> level though. As the default is to use Oz and LTO, it always
>> crashes.
>> 
>> Test:
>> The ELF generated by
>> 'python UefiPayloadPkg/UniversalPayloadBuild.py -a IA32' boots.
>> 
>> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=4357
>> ---
>> BaseTools/Conf/tools_def.template | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/BaseTools/Conf/tools_def.template
>> b/BaseTools/Conf/tools_def.template
>> index 9b59bd75c3..0c584ab390 100755
>> --- a/BaseTools/Conf/tools_def.template
>> +++ b/BaseTools/Conf/tools_def.template
>> @@ -2866,7 +2866,7 @@ DEFINE CLANGDWARF_X64_PREFIX        =
>> ENV(CLANG_BIN)
>> 
>> 
>> # LLVM/CLANG doesn't support -n link option. So, it can't share the same
>> IA32_X64_DLINK_COMMON flag.
>> 
>> # LLVM/CLANG doesn't support common page size. So, it can't share the
>> same GccBase.lds script.
>> 
>> -DEFINE CLANGDWARF_IA32_X64_DLINK_COMMON   = -nostdlib -Wl,-q,--gc-
>> sections -z max-page-size=0x40
>> 
>> +DEFINE CLANGDWARF_IA32_X64_DLINK_COMMON   = -nostdlib -Wl,-q,--
>> gc-sections
>> 
>> DEFINE CLANGDWARF_DLINK2_FLAGS_COMMON     = -Wl,--
>> script=$(EDK_TOOLS_PATH)/Scripts/ClangBase.lds
>> 
>> DEFINE CLANGDWARF_IA32_X64_ASLDLINK_FLAGS =
>> DEF(CLANGDWARF_IA32_X64_DLINK_COMMON) -Wl,--
>> defsym=PECOFF_HEADER_SIZE=0
>> DEF(CLANGDWARF_DLINK2_FLAGS_COMMON) -Wl,--
>> entry,ReferenceAcpiTable -u ReferenceAcpiTable
>> 
>> DEFINE CLANGDWARF_IA32_X64_DLINK_FLAGS    =
>> DEF(CLANGDWARF_IA32_X64_DLINK_COMMON) -Wl,--
>> entry,$(IMAGE_ENTRY_POINT) -u $(IMAGE_ENTRY_POINT) -Wl,-
>> Map,$(DEST_DIR_DEBUG)/$(BASE_NAME).map,--whole-archive
>> 
>> --
>> 2.39.1
> 


[-- Attachment #2: Type: text/html, Size: 5560 bytes --]