[PATCH] Dealing with CPU subtypes (architecture variants)

Hi folks,

The kalimba architecture has a number of variants. The ones of interest to me being 3, 4 and 5. I have added some additional entries to the g_elf_arch_entries to describe these variants. However, up until now, no subtypes (architecture variants), for ELF objectfiles exist in lldb. So I've found it necessary to modify the invocation of ArchSpec.SetArchitecture from ObjectFileELF to deal with this.

Now the kalimba variant specification can be deduced by parsing the e_flags field from the ELF header. So I've written a specific routine which extracts the variant spec from the e_flags. What I'm not too happy about, and would appreciate lldb-dev's opinion on, is if it is ok for the extraction routine (kalimbaVariantFromElfFlags) to remain in ObjectFileELF.cpp? If not, where should the routine go? Furthermore how do I get any new files into the xcode project file, given that I have no xcode system - shall I just change the make and cmake build and rely on an Apple person to fix any xcode build breakage?

Below is a diff of what I have done recognize kalimba variants, basically:

1. add new entries to ArchSpec::enum Core, and g_elf_arch_entries
2. supply routine to extract the kalimba variant from the e_flags
3. if e_machine == EM_CSR_KALIMBA, extract the kalimba variant and pass it to SetArchitecture as the cpu sub type.

--- a/include/lldb/Core/ArchSpec.h 2014-08-18
+++ b/include/lldb/Core/ArchSpec.h 2014-08-18
@@ -103,6 +103,9 @@
          eCore_uknownMach64,

          eCore_kalimba,
+ eCore_kalimba3,
+ eCore_kalimba4,
+ eCore_kalimba5,

          kNumCores,

--- a/source/Core/ArchSpec.cpp 2014-08-05
+++ b/source/Core/ArchSpec.cpp 2014-08-05
@@ -115,7 +115,10 @@
      { eByteOrderLittle, 4, 4, 4 , llvm::Triple::UnknownArch , ArchSpec::eCore_uknownMach32 , "unknown-mach-32" },
      { eByteOrderLittle, 8, 4, 4 , llvm::Triple::UnknownArch , ArchSpec::eCore_uknownMach64 , "unknown-mach-64" },

- { eByteOrderLittle, 4, 1, 1 , llvm::Triple::kalimba , ArchSpec::eCore_kalimba , "kalimba" }
+ { eByteOrderLittle, 4, 1, 1 , llvm::Triple::kalimba , ArchSpec::eCore_kalimba , "kalimba" },
+ { eByteOrderLittle, 4, 1, 1 , llvm::Triple::kalimba , ArchSpec::eCore_kalimba3 , "kalimba3" },
+ { eByteOrderLittle, 4, 1, 1 , llvm::Triple::kalimba , ArchSpec::eCore_kalimba4 , "kalimba4" },
+ { eByteOrderLittle, 4, 1, 1 , llvm::Triple::kalimba , ArchSpec::eCore_kalimba5 , "kalimba5" }
  };

  // Ensure that we have an entry in the g_core_definitions for each core. If you comment out an entry above,
@@ -257,7 +260,10 @@
      { ArchSpec::eCore_x86_64_x86_64 , llvm::elf::EM_X86_64 , LLDB_INVALID_CPUTYPE, 0xFFFFFFFFu, 0xFFFFFFFFu }, // AMD64
      { ArchSpec::eCore_mips64 , llvm::elf::EM_MIPS , LLDB_INVALID_CPUTYPE, 0xFFFFFFFFu, 0xFFFFFFFFu }, // MIPS
      { ArchSpec::eCore_hexagon_generic , llvm::elf::EM_HEXAGON, LLDB_INVALID_CPUTYPE, 0xFFFFFFFFu, 0xFFFFFFFFu }, // HEXAGON
- { ArchSpec::eCore_kalimba , llvm::elf::EM_CSR_KALIMBA, LLDB_INVALID_CPUTYPE, 0xFFFFFFFFu, 0xFFFFFFFFu } // KALIMBA
+ { ArchSpec::eCore_kalimba , llvm::elf::EM_CSR_KALIMBA, LLDB_INVALID_CPUTYPE, 0xFFFFFFFFu, 0xFFFFFFFFu }, // KALIMBA
+ { ArchSpec::eCore_kalimba3 , llvm::elf::EM_CSR_KALIMBA, 3, 0xFFFFFFFFu, 0xFFFFFFFFu }, // KALIMBA
+ { ArchSpec::eCore_kalimba4 , llvm::elf::EM_CSR_KALIMBA, 4, 0xFFFFFFFFu, 0xFFFFFFFFu }, // KALIMBA
+ { ArchSpec::eCore_kalimba5 , llvm::elf::EM_CSR_KALIMBA, 5, 0xFFFFFFFFu, 0xFFFFFFFFu } // KALIMBA

  };

--- a/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp 2014-07-22
+++ b/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp 2014-07-22
@@ -257,6 +257,27 @@
      return true;
  }

+static uint32_t
+kalimbaVariantFromElfFlags(const elf::elf_word e_flags)
+{
+ const uint32_t dsp_rev = e_flags & 0xFF;
+ uint32_t kal_arch_variant = LLDB_INVALID_CPUTYPE;
+ switch(dsp_rev)
+ {
+ // TODO(mg11) Support more variants
+ case 10:
+ kal_arch_variant = 3;
+ break;
+ case 14:
+ kal_arch_variant = 4;
+ break;
+ default:
+ break;
+ }
+ return kal_arch_variant;
+}

Furthermore how do I get any new files into the xcode project file, given that I have no xcode system - shall I just change the make and cmake build and rely on an Apple person to fix any xcode build breakage?

That’s correct. One of the community will fix it up. Enough of us build with enough build systems that I’m sure we’ll catch it relatively quickly. (We ultimately expect build bots to catch these but we are still having to do this “manually” while we get that together).

So I’ve written a specific routine which extracts the variant spec from the e_flags. What I’m not too happy about, and would appreciate lldb-dev’s opinion on, is if it is ok for the extraction routine (kalimbaVariantFromElfFlags) to remain in ObjectFileELF.cpp?

Currently we have Linux/*BSD detection logic (i.e. OS detection code) within ObjectFileELF. Based on that precedent, I think having cpu_arch-specific detection in ObjectFileELF is fine. We could abstract that all and register them somehow to separate them out, but at this point I don’t think the extra complexity is worth it. We can always revisit later.

The patch looks fine and yes it is ok to have Kalimba specific stuff in the ObjectFileELF as it is specific to ELF and you are only checking it if the machine type is set to Kalimba.

So the changes look good, go ahead and check it in, we can fix the Xcode project breakage, but your patch below doesn't seem to have any since it doesn't require an extra file.

Cool. Thanks for these opinions, Todd. I'll submit something in a day or so.
Matt

Todd Fiala wrote:

Greg Clayton wrote:

The patch looks fine and yes it is ok to have Kalimba specific stuff in the ObjectFileELF as it is specific to ELF and you are only checking it if the machine type is set to Kalimba.

So the changes look good, go ahead and check it in, we can fix the Xcode project breakage, but your patch below doesn't seem to have any since it doesn't require an extra file.

Thanks for that. There was no extra file in the diff I floated up, since I was only going to separate the kalimba e_flags extraction stuff to a separate module, if the community reckoned it was necessary.

Matt