[llvm-commits] [PATCH] Object File Library

[Chris Lattner]

On Dec 18, 2010, at 4:58 PM, Michael Spencer wrote:

> On Sat, Dec 18, 2010 at 4:55 PM, Chris Lattner <clattner at apple.com> wrote:
>> 
>> On Dec 15, 2010, at 6:48 PM, Michael Spencer wrote:
>> 
>>> On Mon, Nov 22, 2010 at 8:00 PM, Michael Spencer <bigcheesegs at gmail.com> wrote:
>>>> On Thu, Nov 11, 2010 at 9:23 PM, Michael Spencer <bigcheesegs at gmail.com> wrote:
>>>>> Attached are updated patches to be reviewed. I've split them up into
>>>>> the generic API, the COFF and ELF implementations, tool changes, and
>>>>> tests.
>> Hi Michael,
>> 
>> Can you resend the latest version of the patches?
>> 
>> -Chris
> 
> Attached.

Sorry for the delay, here are some general comments.  I don't feel that I can do a good job reviewing the design (because I don't really understand all the implications), but you clearly have a plan. :)

Patch #1 (coff stuff):

+++ b/lib/Object/COFFObjectFile.cpp
@@ -0,0 +1,369 @@
+//===- COFFObjectFile.h - COFF object file implementation -------*- C++ -*-===//

File header wrong again...

+namespace {

Please make the anonymous namespace minimally wrap the types in it.  I'd prefer to have a new anon namespace for each type, but that's up to you.
http://llvm.org/docs/CodingStandards.html#micro_anonns

+const coff_section *COFFObjectFile::getSection(std::size_t index) const {
+  if (index > 0 && index <= Header->NumberOfSections)
+    return SectionTable + (index - 1);
+  else
+    return 0;

Very minor, but there is no need for else after the return (likewise elsewhere).

+// static_assert(sizeof(coff_file_header) == 20,
+//   "coff_file_header must be a packed pod of 20 bytes!");

You can do this with an extern array:

extern char coff_layout_static_assert[sizeof(foo) == 20 ? 1 : -1];


Patch #2 (elf stuff):

+++ b/lib/Object/ELFObjectFile.cpp
@@ -0,0 +1,702 @@
+//===- ELFObjectFile.h - ELF object file implementation ---------*- C++ -*-===//

Comment wrong again :)

Same comment about anon namespaces.


+template<support::endianness target_endianness>
+struct Elf_Shdr_Base<target_endianness, false> {
+  LLVM_ELF_IMPORT_TYPES(target_endianness, false)
+  Elf_Word sh_name;     // Section name (index into string table)
+  Elf_Word sh_type;     // Section type (SHT_*)
+  Elf_Word sh_flags;    // Section flags (SHF_*)
+  Elf_Addr sh_addr;     // Address where section is to be loaded
...
+template<support::endianness target_endianness>
+struct Elf_Shdr_Base<target_endianness, true> {
+  LLVM_ELF_IMPORT_TYPES(target_endianness, true)
+  Elf_Word  sh_name;     // Section name (index into string table)
+  Elf_Word  sh_type;     // Section type (SHT_*)
+  Elf_Xword sh_flags;    // Section flags (SHF_*)
+  Elf_Addr  sh_addr;     // Address where section is to be loaded


Instead of doing this with explicit specializations like this, I'd suggest defining a helper type like this:

template<bool is64Bits>
struct ElfArchSpecificTypes;

template<> struct ElfArchSpecificTypes<false> {
  typedef Elf_Word PtrSize_Word;
};
template<> struct ElfArchSpecificTypes<true> {
  typedef Elf_Xword PtrSize_Word;
};

THis allows you to define Elf_Shdr_Base in one copy, making the fields that change something like:

template<support::endianness target_endianness, bool is64Bits>
struct Elf_Shdr_Base {
  LLVM_ELF_IMPORT_TYPES(target_endianness, is64Bit)
  Elf_Word sh_name;     // Section name (index into string table)
  Elf_Word sh_type;     // Section type (SHT_*)
  typename ElfArchSpecificTypes<is64Bit>::PtrSize_Word sh_flags;    // Section flags (SHF_*)
  Elf_Addr sh_addr;     // Address where section is to be loaded


This also allows you to eliminate the icky LLVM_ELF_IMPORT_TYPES macro.  In fact it looks like you've partially done this with ELFDataTypeTypedefHelper, why not move the "pointer sized word" concept into it?

Another way to make eliminate the LLVM_ELF_IMPORT_TYPES macro is to just have Elf_Shdr_Base (for example) inherit from ELFDataTypeTypedefHelper.

+    report_fatal_error("Sybol name offset outside of string table!");

typo "Symbol".

+    if (Ident.first == ELF::ELFCLASS32 && Ident.second == ELF::ELFDATA2LSB)
+      return new ELFObjectFile<support::little, false>(Object);
+    else if (Ident.first == ELF::ELFCLASS32 && Ident.second == ELF::ELFDATA2MSB)
+      return new ELFObjectFile<support::big, false>(Object);
+    else if (Ident.first == ELF::ELFCLASS64 && Ident.second == ELF::ELFDATA2LSB)
+      return new ELFObjectFile<support::little, true>(Object);
+    else if (Ident.first == ELF::ELFCLASS64 && Ident.second == ELF::ELFDATA2MSB)
+      return new ELFObjectFile<support::big, true>(Object);
+    else
+      // FIXME: Proper error handling.
+      report_fatal_error("Not an ELF object file!");

No "else" needed after return.


Patch #3: NM Stuff

Looks reasonable to me!

Patch #4: ObjDump

+++ b/tools/llvm-objdump/Makefile
@@ -0,0 +1,17 @@
+##===- tools/llvm-nm/Makefile ------------------------------*- Makefile -*-===##

Please update file headers.

+++ b/tools/llvm-objdump/llvm-objdump.cpp
@@ -0,0 +1,257 @@
+//===-- llvm-nm.cpp - Symbol table dumping utility for llvm ---------------===//

Please update file headers :)


+  const unsigned OutputSize = (15 * 3) + 1;
+  char output[OutputSize];

I'm pretty sure that this is a VLA in C++, please avoid this for portability.  Maybe make OutputSize an enum?


+  for (ObjectFile::section_iterator i = Obj->begin_sections(),
+                                    e = Obj->end_sections();
+                                    i != e; ++i) {
+    if (i->isText()) {


Please use: "if (!i->isText()) continue;" to reduce indentation of the loop body.


Patch #5: testcases

Looks good!

Please make the changes above and commit whenever you'd like to Michael.  This looks like great work!

-Chris