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

["Leif Lindholm" <leif.lindholm@linaro.org>]

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?

> > 
> > 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