[llvm-commits] [PATCH] [lld] patch for ELF Writer to get it to 'Hello world'

[Michael Spencer]

================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:80-86
@@ +79,9 @@
+  /// \brief Describes the type of Chunk
+  enum Kind {
+    ELFHeader, // ELF Header
+    ProgramHeader, // Program Header
+    Segment, // Segment
+    Section, // Section
+    SectionHeader // Section header
+  };
+  Chunk(StringRef name, Kind kind) : _name(name),
----------------
Shankar Kalpathi Easwaran wrote:
> Michael Spencer wrote:
> > This either needs to be an enum class, or the members need to follow the naming convention. Currently this breaks MSVC because it doesn't do two phase name lookup and therefore sees Section and Segment in the derived class (Chunk) instead of the templates Section and Segment.
> Changed the names to add ELF in the front
http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

They should start with K_

================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:121-123
@@ +120,5 @@
+  int64_t            fileOffset() const { return _fileoffset; }
+  void               setFileOffset(int64_t offset) { _fileoffset = offset; }
+  // Output start address of the chunk
+  void               setStart(int64_t start) { _start = start; }
+  int64_t            startAddr() const { return _start; }
----------------
Shankar Kalpathi Easwaran wrote:
> Michael Spencer wrote:
> > What is the difference between these two function? Is one the virtual address?
> yes.
Then the names could be improved and a \brief added.

================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:549
@@ -235,1 +548,3 @@
+  void setOrdinal(int64_t ordinal) {
+    ordinal = ordinal;
   }
----------------
Shankar Kalpathi Easwaran wrote:
> Michael Spencer wrote:
> >   _ordinal = ordinal;
> Nice catch. 
Clang -Wall caught it for me :P

================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1008
@@ +1007,3 @@
+  static inline bool classof(const Chunk<target_endianness, is64Bits> *c) {
+    return c->kind() == Section<target_endianness, is64Bits>::StringTable;
+  }
----------------
Shankar Kalpathi Easwaran wrote:
> Michael Spencer wrote:
> > This is broken. Chunk already has a Kind with the same value.
> Chunk doesnot have such a type called StringTable.
Not named StringTable. But StringTable shares the same underlying value as Chunk::Segment

================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1491-1502
@@ -826,1 +1490,14 @@
+      case DefinedAtom::typeCode:
+          if (name.startswith(".eh_frame_hdr")) 
+            return ORDER_EH_FRAMEHDR;
+          if (name.startswith(".eh_frame")) 
+            return ORDER_EH_FRAME;
+          else if (name.startswith(".init")) 
+            return ORDER_INIT;
+          else if (name.startswith(".fini"))
+            return ORDER_FINI;
+          else if (name.startswith(".hash")) 
+            return ORDER_HASH;
+          else
+            return ORDER_TEXT;
 
----------------
Shankar Kalpathi Easwaran wrote:
> Michael Spencer wrote:
> > llvm::StringSwitch in llvm/ADT/StringSwitch.h
> How do you do startswith with StringSwitch ?
.StartsWith("") instead of .Case("").

https://github.com/Bigcheese/llvm/blob/master/lib/Object/COFFObjectFile.cpp#L214


http://llvm-reviews.chandlerc.com/D222