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

[Michael Spencer]

On Mon, Oct 1, 2012 at 10:28 AM, Hemant Kulkarni <khemant at codeaurora.org> wrote:
> Please ignore my previous mail, I posted old patch again. Here is the new patch. Sorry.
>
> --
> Hemant Kulkarni
> khemant at codeaurora.org
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by the Linux Foundation

> Index: lib/ReaderWriter/ELF/WriterELF.cpp
> ===================================================================
> --- lib/ReaderWriter/ELF/WriterELF.cpp	(revision 164787)
> +++ lib/ReaderWriter/ELF/WriterELF.cpp	(working copy)
> @@ -35,6 +35,7 @@
>  #include "llvm/Support/raw_ostream.h"
>  #include "llvm/Support/system_error.h"
>
> +
>  #include <map>
>  #include <tuple>
>  #include <vector>
> @@ -42,11 +43,14 @@
>  using namespace llvm;
>  using namespace llvm::object;
>  namespace lld {
> +
>  namespace elf {
>
>  template<support::endianness target_endianness, bool is64Bits>
>  class ELFWriter;
>
> +
> +

Whitespace shouldn't be added to unrelated changes. Also, it is llvm style to
only have one new line separating things. And no new lines between namespaces.

There are only 136 instances of multiple empty lines out of 87k empty lines
in LLVM.

>  /// \brief A Chunk is a contiguous range of space.
>  template<support::endianness target_endianness, bool is64Bits>
>  class Chunk {

> @@ -71,43 +77,80 @@
>    uint64_t _address;
>    uint64_t _fileOffset;
>    uint64_t _align2;
> +  uint64_t _ordinal;
>  };
>
> +
> +template<support::endianness target_endianness, bool is64Bits>
> +static void swapChunkPositions(Chunk<target_endianness, is64Bits>&a,
> +                             Chunk<target_endianness, is64Bits>&b){

Chunk should line up and { needs a space before it.

> +   uint64_t tempOrdinal;
> +   if (a.ordinal() == b.ordinal()) return;
> +   tempOrdinal = a.ordinal();
> +   a.setOrdinal(b.ordinal());
> +   b.setOrdinal(tempOrdinal);

Two spaces for indentation. This is three.

> +}
> +
>  /// Pair of atom and offset in section.
>  typedef std::tuple<const DefinedAtom*, uint64_t> AtomInfo;
>
> -/// \brief A Section represents a set of Atoms assigned to a specific ELF
> -///        Section.
> +/// \brief A SectionChunk represents ELF sections
>  template<support::endianness target_endianness, bool is64Bits>
>  class SectionChunk : public Chunk<target_endianness, is64Bits> {
>  public:

> +  SectionChunk(StringRef secName, StringRef segName, bool loadable,
> +               uint64_t flags , uint64_t link,  uint64_t info ,
> +               uint64_t type, uint64_t entsz, const WriterOptionsELF &op,
> +              ELFWriter<target_endianness, is64Bits> &writer);

Align with rest of arguments.

> +//
> +/// \brief Represents the symtab section
> +///
> +/// ELFSymbolTableChunk represents the Symbol table as per ELF ABI
> +/// This is a table with Elf[32/64]_Sym entries in it.
> +//

Remove extra //

> +template<support::endianness target_endianness, bool is64Bits>
> +class ELFSymbolTableChunk : public SectionChunk<target_endianness, is64Bits> {
> +public:
> +  typedef object::Elf_Sym_Impl<target_endianness, is64Bits> Elf_Sym;
> +  ELFSymbolTableChunk(const WriterOptionsELF &options,
> +                      ELFWriter<target_endianness, is64Bits> &writer,
> +                      StringRef secName);
> +  virtual StringRef   segmentName() const { return this->_segmentName; }
> +  void                addSymbol(const Atom *a, uint16_t shndx);
> +  void                addSymbol(Elf_Sym* x);

* goes on the right.

> +  const char          *info();
> +  void                setAttributes();
>
> +  virtual void               write(uint8_t *fileBuffer);
>
>

> @@ -239,6 +299,9 @@
>  template<support::endianness target_endianness, bool is64Bits>
>  Chunk<target_endianness, is64Bits>::Chunk()
>   : _size(0), _address(0), _fileOffset(0), _align2(0) {
> +   // 0 and 1 are reserved. 0 for ELF header and 1 for Sectiontable header.
> +   static uint64_t orderNumber = 0;
> +   _ordinal = orderNumber++;

Three spaces again.

> +StockSectionChunk<target_endianness, is64Bits>::
> +  StockSectionChunk(StringRef secName, bool loadable,
> +                    DefinedAtom::ContentType type,
> +                    const WriterOptionsELF &options,
> +                    ELFWriter<target_endianness, is64Bits> &writer)
> + : SectionChunk<target_endianness, is64Bits>(secName, StringRef("PT_NULL"),

Uneeded StringRef.

> +                                             loadable, 0lu, 0lu, 0u, 0lu, 0lu,
> +                                             options, writer)
> +  {

There are still quite a few places where comments are incorrectly indented. They
should be at the same level as the code they describe.

- Michael Spencer