[llvm-commits] LLD patch - symbol table and reorganization of sections

[Sean Silva]

-
+
 /// \brief An ELFHeaderChunk represents the Elf[32/64]_Ehdr structure at the
 ///        start of an ELF executable file.
 template<support::endianness target_endianness, bool is64Bits>
@@ -134,7 +177,7 @@
   void e_shnum(uint16_t shnum)         { _eh.e_shnum = shnum; }
   void e_shstrndx(uint16_t shstrndx)   { _eh.e_shstrndx = shstrndx; }

-  uint64_t  size() { return sizeof (Elf_Ehdr); }
+  uint64_t  size()                     { return sizeof (Elf_Ehdr); }

Try to avoid introducing extraneous whitespace or other formatting
changes that distract from the review of the patch.

+template< support::endianness target_endianness, bool is64Bits>
+void ELFSymbolTableChunk<target_endianness, is64Bits>::addSymbol(const Atom *a,
+                                                        uint16_t shndx) {
+ Elf_Sym *symbol = new(_symbolAllocate.Allocate<Elf_Sym>()) Elf_Sym;
+ unsigned char b = 0 , t = 0;
+ const DefinedAtom *da;
+ const AbsoluteAtom *aa;
+
+ symbol->st_name = _stringSection->addString(a->name());
+// In relocatable files, st_value holds a section offset for a defined symbol.
+// st_value is an offset from the beginning of the section that st_shndx
+// identifies. After we assign file offsets we can set this value correctly.
+ symbol->st_size = 0;
+ symbol->st_shndx = shndx;
+ symbol->st_value = 0;
+// FIXME: Need to change and account all STV* when visibilities are supported
+ symbol->st_other = ELF::STV_DEFAULT;
+ if (a->definition() == lld::Atom::definitionRegular){
+      //da = static_cast<const DefinedAtom *>(a);
+      da = llvm::dyn_cast<const DefinedAtom>(a);
+      symbol->st_size = da->size();
+      lld::DefinedAtom::ContentType ct;
+      switch (ct = da->contentType()){
+         case  DefinedAtom::typeCode:
+           t = ELF::STT_FUNC;
+           break;
+          case  DefinedAtom::typeData:
+           t = ELF::STT_OBJECT;
+           break;
+          case  DefinedAtom::typeZeroFill:
+// In relocatable files, st_value holds alignment constraints for a
symbol whose
+// section index is SHN_COMMON
+           if (this->_options.type() == ELF::ET_REL){
+           t = ELF::STT_COMMON;
+           symbol->st_value = 0x1 << (da->alignment()).powerOf2;
+           symbol->st_shndx = ELF::SHN_COMMON;
+           }
+           break;
+          case DefinedAtom::typeFirstInSection:
+           t = ELF::STT_SECTION;
+           break;
+// TODO:: How to find STT_FILE symbols?
+          default:
+           t = ELF::STT_NOTYPE;
+     }
+// TODO: Find out how to incorporate STB_HIOS STB_LOOS
+// STB_HIPROC and STB_LOPROC
+
+     if (da->scope() == DefinedAtom::scopeTranslationUnit)
+             b = ELF::STB_LOCAL;
+     else if (da->merge() == DefinedAtom::mergeAsWeak)
+             b= ELF::STB_WEAK;
+     else
+             b = ELF::STB_GLOBAL;
+    }
+//FIXME: Absolute atoms need more properties to differentiate each other
+// based on binding and type of symbol
+ else if (a->definition() == lld::Atom::definitionAbsolute){
+          aa = llvm::dyn_cast<const AbsoluteAtom>(a);
+          symbol->st_value = aa->value();
+ }
+// We have an undefined atom
+ else {
+      symbol->st_value = 0;
+      t = ELF::STT_NOTYPE;
+      b = ELF::STB_LOCAL;
+ }
+
+// This will set the st_info field in Elf_Sym
+ symbol->setBindingAndType(b, t);
+ _symbolTable.push_back(symbol);
+ this->_size += sizeof(Elf_Sym);
+
+}

Please check your indentation, brace placement, and comment placement
in this section, and throughout the patch.

+
+  std::stable_sort(_chunks.begin(), _chunks.end(),
+  static_cast<bool(*)(const Chunk<target_endianness, is64Bits>*,
+                      const Chunk<target_endianness, is64Bits>*)>([]
+  (const Chunk<target_endianness, is64Bits> *A,
+   const Chunk<target_endianness, is64Bits> *B) -> bool {
+          return (A->ordinal() < B->ordinal());}));
+
+  std::stable_sort(_sectionChunks.begin(), _sectionChunks.end(),
+  static_cast<bool(*)(const SectionChunk<target_endianness, is64Bits>*,
+                      const SectionChunk<target_endianness, is64Bits>*)>([]
+  (const SectionChunk<target_endianness, is64Bits> *A,
+   const SectionChunk<target_endianness, is64Bits> *B) -> bool {
+          return (A->ordinal() < B->ordinal());}));

This is insane. Factor out that lambda at least. Somehow make this
cleaner. You can probably move the comparator into the `Chunk` class
as a comparator struct `LessThanByOrdinal`.

+  class ELFStringSectionChunk<target_endianness, is64Bits> *_stringSection;

Kill the "class".


Also, is there any way to split this patch up into multiple, more
focused patches?

--Sean Silva

On Fri, Sep 28, 2012 at 3:00 PM, Hemant Kulkarni <khemant at codeaurora.org> wrote:
> Purpose of patch:
>
> Add symbol table to writer to emit symbols.
>
> Make all sections – ones made due to atoms and ones made by linker at output
> time to be derived from one class. Have this class all the attributes needed
> to make a section header. Use this class to build section table header and
> set all offsets/addresses.
>
>
>
> Added test for the same. The test needed to be changed to reflect correct
> order sections are now emitted; hence you might see change in existing test
> too.
>
> Please review the patch and send in comments.
>
>
>
> --
>
> Hemant Kulkarni
>
> khemant at codeaurora.org
>
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by
> the Linux Foundation
>
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>