Re: [edk2-devel] [edk2-staging/RISC-V-V2 PATCH v2 24/29] BaseTools: BaseTools changes for RISC-V platform.

["Abner Chang" <abner.chang@hpe.com>]

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Leif Lindholm
> Sent: Tuesday, October 15, 2019 6:57 PM
> To: devel@edk2.groups.io; Chang, Abner (HPS SW/FW Technologist)
> <abner.chang@hpe.com>
> Subject: Re: [edk2-devel] [edk2-staging/RISC-V-V2 PATCH v2 24/29]
> BaseTools: BaseTools changes for RISC-V platform.
> 
> On Tue, Oct 15, 2019 at 06:18:29AM +0000, Abner Chang wrote:
> > > -----Original Message-----
> > > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf
> > > Of Leif Lindholm
> > > Sent: Friday, September 27, 2019 6:10 AM
> > > To: devel@edk2.groups.io; Chang, Abner (HPS SW/FW Technologist)
> > > <abner.chang@hpe.com>
> > > Subject: Re: [edk2-devel] [edk2-staging/RISC-V-V2 PATCH v2 24/29]
> > > BaseTools: BaseTools changes for RISC-V platform.
> > >
> > > On Mon, Sep 23, 2019 at 08:31:50AM +0800, Abner Chang wrote:
> > > > BaseTools changes for building EDK2 RISC-V platform.
> > > > The changes made to build_rule.template is to avoid build errors
> > > > cause by GCC711RISCV tool chain.
> > >
> > > Thank you, this is much cleaner.
> > > There are however some issues in this patch that prevent building on
> > > any platform. Please ensure to give a local build test before
> > > submitting a 3.
> > >
> > > First of all, this still does not contain the addition to
> > > BaseTools/Source/Python/Common/buildoptions.py that I mentioned in
> > > INVALID URI REMOVED
> > >
> 3A__edk2.groups.io_g_devel_message_47036&d=DwIBAg&c=C5b8zRQO1mi
> > >
> GmBeVZ2LFWg&r=_SN6FZBN4Vgi4Ulkskz6qU3NYRO03nHp9P7Z5q59A3E&m=
> > > YclXVT-
> > >
> dumczX_RwFNv_GDdWAp1gvJXUN0KRfNaGEtw&s=Gp1kHhT9Z6PR93PmPN
> > > ZD-_0h0rPDXLsODbhLWyQs8NA&e=  - meaning that attempting to build
> > > anything for RISCV64 gives an error.
> >
> > I thought you were saying to use ENV(GCC5_RISCV64_PREFIX) to point to
> > build tool binaries, no?
> 
> That is unrelated.
> 
> I am talking about that the build command needs to be aware of the
> existence of the RISCV64 architecture. The version of the build command
> included in v2 cannot be used to build the tree. Just like I commented on,
> and provided the patch for, for v1. In the message linked to above.
> 
> How *have* you tested the build without a working build command?

Ah I see.
You were saying -a in build command line. That was fixed in v3.
> 
> > >
> > > Other minor issues reviewed inline:
> > >
> > > > Signed-off-by: Abner Chang <abner.chang@hpe.com>
> > > > ---
> > > >  BaseTools/Conf/build_rule.template                 |  62 ++---
> > > >  BaseTools/Conf/tools_def.template                  |  64 ++++-
> > > >  BaseTools/Source/C/Common/BasePeCoff.c             |  15 +-
> > > >  BaseTools/Source/C/Common/PeCoffLoaderEx.c         |  95 ++++++++
> > > >  BaseTools/Source/C/GenFv/GenFvInternalLib.c        | 128 +++++++++-
> > > >  BaseTools/Source/C/GenFw/Elf32Convert.c            |   5 +-
> > > >  BaseTools/Source/C/GenFw/Elf64Convert.c            | 260
> > > ++++++++++++++++++++-
> > > >  BaseTools/Source/C/GenFw/elf_common.h              |  62 +++++
> > > >  .../Source/C/Include/IndustryStandard/PeImage.h    |   6 +
> > > >  BaseTools/Source/Python/Common/DataType.py         |   7 +-
> > > >  10 files changed, 659 insertions(+), 45 deletions(-)
> > > >
> > > > diff --git a/BaseTools/Conf/build_rule.template
> > > b/BaseTools/Conf/build_rule.template
> > > > index db06d3a..fab3926 100755
> > > > --- a/BaseTools/Conf/build_rule.template
> > > > +++ b/BaseTools/Conf/build_rule.template
> > > > @@ -321,6 +314,21 @@
> > > >          "$(OBJCOPY)" $(OBJCOPY_FLAGS) ${dst}
> > > >
> > > >
> > > > +[Static-Library-File.COMMON.RISCV64, Static-Library-
> > > File.COMMON.RISCV32]
> > > > +    <InputFile>
> > > > +        *.lib
> > > > +
> > > > +    <ExtraDependency>
> > > > +        $(MAKE_FILE)
> > > > +
> > > > +    <OutputFile>
> > > > +        $(DEBUG_DIR)(+)$(MODULE_NAME).dll
> > > > +
> > > > +    <Command.GCC>
> > > > +        "$(DLINK)" -o ${dst} $(DLINK_FLAGS) --start-group
> > > > + $(DLINK_SPATH)
> > > @$(STATIC_LIBRARY_FILES_LIST) --end-group $(DLINK2_FLAGS)
> > >
> > > This line looks to me like the only thing that is actually changed
> > > here, and I am not convinced it is necessary.
> > >           "$(DLINK)" -o ${dst} $(DLINK_FLAGS) -Wl,--start-
> > > group,@$(STATIC_LIBRARY_FILES_LIST),--end-group $(CC_FLAGS)
> > > $(DLINK2_FLAGS)
> > >
> > > On the ARM/AARCH64 side, we use gcc as the DLINK, and pass the
> > > required flags through to the linker with -Wl. Please have a look
> > > and try to rework at that end rather than fundamentally revamping
> > > the basic build rules differently for RISCV than other architectures.
> > >
> > > Basically, please discard all changes to this file, apply the below
> > > diff, and rework the flags to resolve the builds. (Basically, add a
> > > bunch of -Wl,)
> >
> > I got build error when use -Wl with the specific version of RISC-V GCC
> > toolchain (the old and workable one). I will revisit this when I
> > investigate the issue caused by latest RISC-V build tool.
> 
> OK.
> 
> > > > diff --git a/BaseTools/Source/C/GenFw/Elf64Convert.c
> > > b/BaseTools/Source/C/GenFw/Elf64Convert.c
> > > > index 3d6319c..2aa09fd 100644
> > > > --- a/BaseTools/Source/C/GenFw/Elf64Convert.c
> > > > +++ b/BaseTools/Source/C/GenFw/Elf64Convert.c
> > > > @@ -3,6 +3,7 @@ Elf64 convert solution
> > > >
> > > >  Copyright (c) 2010 - 2018, Intel Corporation. All rights
> > > > reserved.<BR>  Portions copyright (c) 2013-2014, ARM Ltd. All
> > > > rights reserved.<BR>
> > > > +Portions Copyright (c) 2016 - 2019 Hewlett Packard Enterprise
> > > Development LP. All rights reserved.<BR>
> > > >
> > > >  SPDX-License-Identifier: BSD-2-Clause-Patent
> > > >
> > > > @@ -31,6 +32,12 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> > > > #include "ElfConvert.h"
> > > >  #include "Elf64Convert.h"
> > > >
> > > > +#define RV_X(x, s, n) (((x) >> (s)) & ((1<<(n))-1)) #define
> > > > +RISCV_IMM_BITS 12 #define RISCV_IMM_REACH
> (1LL<<RISCV_IMM_BITS)
> > > > +#define RISCV_CONST_HIGH_PART(VALUE) \
> > > > +  (((VALUE) + (RISCV_IMM_REACH/2)) & ~(RISCV_IMM_REACH-1))
> > > > +
> > > >  STATIC
> > > >  VOID
> > > >  ScanSections64 (
> > > > @@ -153,8 +160,8 @@ InitializeElf64 (
> > > >      Error (NULL, 0, 3000, "Unsupported", "ELF e_type not ET_EXEC
> > > > or
> > > ET_DYN");
> > > >      return FALSE;
> > > >    }
> > > > -  if (!((mEhdr->e_machine == EM_X86_64) || (mEhdr->e_machine ==
> > > EM_AARCH64))) {
> > > > -    Error (NULL, 0, 3000, "Unsupported", "ELF e_machine not
> EM_X86_64 or
> > > EM_AARCH64");
> > > > +  if (!((mEhdr->e_machine == EM_X86_64) || (mEhdr->e_machine ==
> > > EM_AARCH64) || (mEhdr->e_machine == EM_RISCV64))) {
> > > > +    Error (NULL, 0, 3000, "Unsupported", "ELF e_machine is not
> > > > + Elf64
> > > machine.");
> > > >      return FALSE;
> > > >    }
> > > >    if (mEhdr->e_version != EV_CURRENT) { @@ -481,6 +488,7 @@
> > > > ScanSections64 (
> > > >    switch (mEhdr->e_machine) {
> > > >    case EM_X86_64:
> > > >    case EM_AARCH64:
> > > > +  case EM_RISCV64:
> > > >      mCoffOffset += sizeof (EFI_IMAGE_NT_HEADERS64);
> > > >    break;
> > > >    default:
> > > > @@ -690,6 +698,11 @@ ScanSections64 (
> > > >      NtHdr->Pe32Plus.FileHeader.Machine =
> > > EFI_IMAGE_MACHINE_AARCH64;
> > > >      NtHdr->Pe32Plus.OptionalHeader.Magic =
> > > EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC;
> > > >      break;
> > > > +  case EM_RISCV64:
> > > > +    NtHdr->Pe32Plus.FileHeader.Machine =
> > > EFI_IMAGE_MACHINE_RISCV64;
> > > > +    NtHdr->Pe32Plus.OptionalHeader.Magic =
> > > EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC;
> > > > +    break;
> > > > +
> > > >    default:
> > > >      VerboseMsg ("%s unknown e_machine type. Assume X64",
> > > (UINTN)mEhdr->e_machine);
> > > >      NtHdr->Pe32Plus.FileHeader.Machine = EFI_IMAGE_MACHINE_X64;
> > > > @@ -769,6 +782,11 @@ WriteSections64 (
> > > >    Elf_Shdr    *SecShdr;
> > > >    UINT32      SecOffset;
> > > >    BOOLEAN     (*Filter)(Elf_Shdr *);
> > > > +  UINT32      Value;
> > > > +  UINT32      Value2;
> > > > +  UINT8       *Pass1Targ = NULL;
> > > > +  Elf_Shdr    *Pass1Sym = NULL;
> > > > +  Elf64_Half  Pass1SymSecIndex = 0;
> > > >    Elf64_Addr  GOTEntryRva;
> > > >
> > > >    //
> > > > @@ -893,13 +911,14 @@ WriteSections64 (
> > > >            if (SymName == NULL) {
> > > >              SymName = (const UINT8 *)"<unknown>";
> > > >            }
> > > > +          if (mEhdr->e_machine != EM_RISCV64) {
> > >
> > > This needs a comment explaining why this does not apply to RISCV.
> >
> > >
> > > > +            Error (NULL, 0, 3000, "Invalid",
> > > > +                   "%s: Bad definition for symbol '%s'@%#llx or
> > > > + unsupported
> > > symbol type.  "
> > > > +                   "For example, absolute and undefined symbols
> > > > + are not
> > > supported.",
> > > > +                   mInImageName, SymName, Sym->st_value);
> > > >
> > > > -          Error (NULL, 0, 3000, "Invalid",
> > > > -                 "%s: Bad definition for symbol '%s'@%#llx or unsupported
> symbol
> > > type.  "
> > > > -                 "For example, absolute and undefined symbols are not
> > > supported.",
> > > > -                 mInImageName, SymName, Sym->st_value);
> > > > -
> > > > -          exit(EXIT_FAILURE);
> > > > +            exit(EXIT_FAILURE);
> > > > +          }
> > > >          }
> > > >          SymShdr = GetShdrByIndex(Sym->st_shndx);
> > > >
> > > > @@ -1114,6 +1133,128 @@ WriteSections64 (
> > > >            default:
> > > >              Error (NULL, 0, 3000, "Invalid", "WriteSections64():
> > > > %s unsupported
> > > ELF EM_AARCH64 relocation 0x%x.", mInImageName, (unsigned)
> > > ELF_R_TYPE(Rel->r_info));
> > > >            }
> > > > +        } else if (mEhdr->e_machine == EM_RISCV64) {
> > >
> > > Yeah, this code block is just *waaaay* too big.
> > > Please break it out into its own helper function.
> >
> > Leif, I am not going to address this issue this time. I just follow
> > what other archs done in this function.  I agree with you this
> > function is way too long. I could create a task to refine this
> > function once RISC-V part is reviewed and pushed to the mainstream.
> 
> I don't understand this logic.
> Breaking this out into a helper function would take no more time than you
> spent on typing the above response that you don't intend to do.
> 
> Yes, the code for the other architectures is also bad, and we should revisit
> and fix it. But that doesn't mean we should keep making the file worse.
> 
> I am already giving a pass on how even if you break this hunk out into its own
> helper function, that one is itself way too long and needs to be broken up.
> But at least if we move it out, we've compartmentalised the problem.
> 
> Merging something bad in order to fix it later is never the answer.
> (And not only because in 95% in cases, that later never happens.)
> 
> Best Regards,
> 
> Leif
> 
>