Adding SymbolRef::getSectionName for getting a textual section name for the symbol. Updating llvm-objdump to use this

[colinl]

Summary

Key features of this patch is separation of file-format specific tweaks to the section name are pushed in to an appropriate class. The MachO specific decorating of section name with the segment name is in MachOObjectFile rather than llvm-objdump.

Another feature is being able to get a section name in cases where a section iterator can't be retrieved e.g. processor specific sections in ELF files.

I'm planning to commit this in two patches. One creating the method and moving the MachO functionality which is an NFC change. A followup patch is the change to ELFObjectFile including a test showing an processor specific section name being printed out.

[• rafael]

include/llvm/Object/ELFObjectFile.h

34

Please use llvm's streams.

209

Use ErrorOr

531

Don't put an anonymous namespace in a header.

[colinl]

Applied ErrorOr instead of by-reference result pattern. Substituted sstream for raw_string_stream. Moved utility function out of header.

[• rafael]

Is there some intended use other than the human friendly dump that llvm-objdump does?

An absolute symbol is not in a section named *ABS*. That is just what llvm-objdump prints in place of the section name for absolute symbols.

For the hexagon case. What does gnu objdump do?

include/llvm/Object/ELFObjectFile.h

58

Functions should be a verb and start with a lowercase letter. How about getNameFromPRCNumber?

What is PRC?

533

I don't think we use auto in cases like this.

lib/Object/MachOObjectFile.cpp

488

Don't use auto in here.

498

This looks a bit odd. The segment is not part of the section name.

It is double odd because this would be the only function adding the segment name.

lib/Object/ObjectFile.cpp

64

Move this earlier, just after the call that produces the error.

You can also fuse it with the if

if (std::error_code EC = ...)

return EC;

[colinl]

I guess the function's purpose is really to service llvm-objdump. I would have preferred to put this code somehow in the llvm-objdump tool though there's no visitor for the related symbol and object file classes so doing so seemed like it would be a mess of dyn_cast chains.

The end goal was for small common symbols to have something like ".scommon.4" printed instead of "*UND*". Stock GNU objdump prints *ABS* though I didn't debug why, that seems likely to be a logic fallthrough.

include/llvm/Object/ELFObjectFile.h

58

It should probably be SHN to match SHN_LOPROC and SHN_HIPROC.

533

I can change it to ELFFile<ELFT>::Elf_Sym *. Seems a little unfortunate because shortening typenames like this seems like what auto is for.

lib/Object/MachOObjectFile.cpp

498

This was to retain existing llvm-objdump behavior.

[ruiu]

include/llvm/Object/ELFObjectFile.h

533

We don't use "auto" if its type is not obvious from the very narrow context. I don't use "auto" unless the real type appears on the right-hand side.

[colinl]

Addressed comments and changed function names to indicate they're for the purpose of objdump. Maybe with new naming it'll fit in to place better?

It still seems unfortunate to inject objdump specific functions in to the Object library. If there are ideas on a cleaner way to generate this name WRT to placement and being able to access required functionality, I'm open to input.

[ruiu]

lib/Object/ELFObjectFile.cpp

60–67

This can be

return Twine("SHN_PROC[0x") + Twine::utohexstr(Index) + "]";

?

[• rafael]

It still seems unfortunate to inject objdump specific functions in to the Object library. If there are ideas on a cleaner way to generate this name WRT to placement and being able to access required functionality, I'm open to input.

Yes, please put all this in llvm-objdump. We only have 3 object
formats, so this shouldn't be too bad. Being in objdump should allow
you to also just pass the stream into these functions instead of
computing a string that is then printed.

Cheers,
Rafael

include/llvm/Object/ELFObjectFile.h

   virtual uint64_t getSectionFlags(DataRefImpl Sec) const = 0;
 
   virtual ErrorOr<int64_t> getRelocationAddend(DataRefImpl Rel) const = 0;
+  std::string getNameFromSHN(uint32_t Index) const;
 public:
 
   typedef iterator_range<elf_symbol_iterator> elf_symbol_iterator_range;

   SymbolRef::Type getSymbolType(DataRefImpl Symb) const override;
   ErrorOr<section_iterator> getSymbolSection(const Elf_Sym *Symb) const;
   ErrorOr<section_iterator> getSymbolSection(DataRefImpl Symb) const override;
+  ErrorOr<std::string>
+  getObjdumpSymbolSectionName(DataRefImpl Symb) const override;
 
   void moveSectionNext(DataRefImpl &Sec) const override;
   std::error_code getSectionName(DataRefImpl Sec,

 }
 
 template <class ELFT>
+ErrorOr<std::string>
+ELFObjectFile<ELFT>::getObjdumpSymbolSectionName(DataRefImpl Symb) const {
+  const typename ELFFile<ELFT>::Elf_Sym *Symbol = getSymbol(Symb);
+  uint32_t Index = Symbol->st_shndx;
+  if(Index < ELF::SHN_LOPROC || Index > ELF::SHN_HIPROC)
+    return ObjectFile::getObjdumpSymbolSectionName(Symb);
+  switch (EF.getHeader()->e_machine) {
+  case ELF::EM_HEXAGON:
+    switch(Index) {
+    case ELF::SHN_HEXAGON_SCOMMON:
+      return std::string(".scommon");
+    case ELF::SHN_HEXAGON_SCOMMON_1:
+      return std::string(".scommon.1");
+    case ELF::SHN_HEXAGON_SCOMMON_2:
+      return std::string(".scommon.2");
+    case ELF::SHN_HEXAGON_SCOMMON_4:
+      return std::string(".scommon.4");
+    case ELF::SHN_HEXAGON_SCOMMON_8:
+      return std::string(".scommon.8");
+    default:
+      return getNameFromSHN(Index);
+    }
+  default:
+    return getNameFromSHN(Index);
+  }
+}
+
+template <class ELFT>
 void ELFObjectFile<ELFT>::moveSectionNext(DataRefImpl &Sec) const {
   const Elf_Shdr *ESec = toELFShdrIter(Sec);
   Sec = toDRI(++ESec);

include/llvm/Object/MachO.h

   SymbolRef::Type getSymbolType(DataRefImpl Symb) const override;
   uint32_t getSymbolFlags(DataRefImpl Symb) const override;
   ErrorOr<section_iterator> getSymbolSection(DataRefImpl Symb) const override;
+  ErrorOr<std::string>
+  getObjdumpSymbolSectionName(DataRefImpl Symb) const override;
+
   unsigned getSymbolSectionID(SymbolRef Symb) const;
   unsigned getSectionID(SectionRef Sec) const;
 

include/llvm/Object/ObjectFile.h

   /// @brief Get section this symbol is defined in reference to. Result is
   /// end_sections() if it is undefined or is an absolute symbol.
   ErrorOr<section_iterator> getSection() const;
+  /// Return the section name for this symbol for display in objdump
+  ErrorOr<std::string> getObjdumpSectionName() const;
 
   const ObjectFile *getObject() const;
 };

   virtual SymbolRef::Type getSymbolType(DataRefImpl Symb) const = 0;
   virtual ErrorOr<section_iterator>
   getSymbolSection(DataRefImpl Symb) const = 0;
+  virtual ErrorOr<std::string>
+  getObjdumpSymbolSectionName(DataRefImpl Symb) const;
 
   // Same as above for SectionRef.
   friend class SectionRef;

 
 inline ErrorOr<section_iterator> SymbolRef::getSection() const {
   return getObject()->getSymbolSection(getRawDataRefImpl());
+}
+
+inline ErrorOr<std::string> SymbolRef::getObjdumpSectionName() const {
+  return getObject()->getObjdumpSymbolSectionName(getRawDataRefImpl());
 }
 
 inline SymbolRef::Type SymbolRef::getType() const {

lib/Object/ELFObjectFile.cpp

 
 #include "llvm/Object/ELFObjectFile.h"
 #include "llvm/Support/MathExtras.h"
+#include "llvm/Support/raw_ostream.h"
 
 namespace llvm {
 using namespace object;

   return std::move(R);
 }
 
+std::string ELFObjectFileBase::getNameFromSHN(uint32_t Index) const {
+  std::string Name;
+  {
+    llvm::raw_string_ostream Stream(Name);
+    Stream << "SHN_PROC[";
+    Stream.write_hex(Index);
+    Stream << "]";
+  }
+  return Name;
+}
+
 } // end namespace llvm

lib/Object/MachOObjectFile.cpp

   MachO::nlist_base Entry =
       getSymbolTableEntryBase(this, Sym.getRawDataRefImpl());
   return Entry.n_sect - 1;
+}
+
+ErrorOr<std::string>
+MachOObjectFile::getObjdumpSymbolSectionName(DataRefImpl Symb) const {
+  ErrorOr<std::string> Result = ObjectFile::getObjdumpSymbolSectionName(Symb);
+  if(!Result)
+    return Result;
+  section_iterator Section = section_end();
+  auto Error = getSymbolSection(Symb, Section);
+  if(Error || Section == section_end())
+    // Section doesn't existing in file e.g. proc-specific
+    return Result;
+  DataRefImpl DR = Section->getRawDataRefImpl();
+  StringRef SegmentName = getSectionFinalSegmentName(DR);
+  return SegmentName.str() + "," + *Result;
 }
 
 void MachOObjectFile::moveSectionNext(DataRefImpl &Sec) const {

lib/Object/ObjectFile.cpp

   return std::error_code();
 }
 
-uint32_t ObjectFile::getSymbolAlignment(DataRefImpl DRI) const { return 0; }
+uint32_t ObjectFile::getSymbolAlignment(DataRefImpl DRI) const { return 0; }
+
+ErrorOr<std::string>
+ObjectFile::getObjdumpSymbolSectionName(DataRefImpl Symb) const {
+  section_iterator Section = section_end();
+  if(std::error_code Error = getSymbolSection(Symb, Section))
+    return Error;
+  uint32_t Flags = getSymbolFlags(Symb);
+  bool Absolute = Flags & SymbolRef::SF_Absolute;
+  bool Common = Flags & SymbolRef::SF_Common;
+  if (Absolute)
+    return std::string("*ABS*");
+  else if (Common)
+    return std::string("*COM*");
+  else if(Section != section_end()) {
+    StringRef Name;
+    auto Error = Section->getName(Name);
+    if(Error)
+      return Error;
+    return Name.str();
+  }
+  else
+    return std::string("*UND*");
+}
 
 section_iterator ObjectFile::getRelocatedSection(DataRefImpl Sec) const {
   return section_iterator(SectionRef(Sec, this));

tools/llvm-objdump/llvm-objdump.cpp

       error(NameOrErr.getError());
       Name = *NameOrErr;
     }
+    ErrorOr<std::string> SectionName = Symbol.getObjdumpSectionName();
+    error(SectionName.getError());
 
     bool Global = Flags & SymbolRef::SF_Global;
     bool Weak = Flags & SymbolRef::SF_Weak;
-    bool Absolute = Flags & SymbolRef::SF_Absolute;
     bool Common = Flags & SymbolRef::SF_Common;
     bool Hidden = Flags & SymbolRef::SF_Hidden;
 

            << Debug // Debugging (d) or dynamic (D) symbol.
            << FileFunc // Name of function (F), file (f) or object (O).
            << ' ';
-    if (Absolute) {
-      outs() << "*ABS*";
-    } else if (Common) {
-      outs() << "*COM*";
-    } else if (Section == o->section_end()) {
-      outs() << "*UND*";
-    } else {
-      if (const MachOObjectFile *MachO =
-          dyn_cast<const MachOObjectFile>(o)) {
-        DataRefImpl DR = Section->getRawDataRefImpl();
-        StringRef SegmentName = MachO->getSectionFinalSegmentName(DR);
-        outs() << SegmentName << ",";
-      }
-      StringRef SectionName;
-      error(Section->getName(SectionName));
-      outs() << SectionName;
-    }
-
+    outs() << *SectionName;
     outs() << '\t';
     if (Common || isa<ELFObjectFileBase>(o)) {
       uint64_t Val =