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

[Colin LeMahieu via llvm-commits]

colinl marked 4 inline comments as done.
colinl added a comment.

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.


================
Comment at: include/llvm/Object/ELFObjectFile.h:58
@@ -57,2 +57,3 @@
   virtual ErrorOr<int64_t> getRelocationAddend(DataRefImpl Rel) const = 0;
+  std::string NameFromPRCNumber(uint32_t Index) const;
 public:
----------------
rafael wrote:
> Functions should be a verb and start with a lowercase letter. How about getNameFromPRCNumber?
> 
> What is PRC?
It should probably be SHN to match SHN_LOPROC and SHN_HIPROC.

================
Comment at: include/llvm/Object/ELFObjectFile.h:533
@@ +532,3 @@
+ELFObjectFile<ELFT>::getSymbolSectionName(DataRefImpl Symb) const {
+  auto Symbol = getSymbol(Symb);
+  uint32_t Index = Symbol->st_shndx;
----------------
rafael wrote:
> I don't think we use auto in cases like this.
I can change it to ELFFile<ELFT>::Elf_Sym *.  Seems a little unfortunate because shortening typenames like this seems like what auto is for.

================
Comment at: lib/Object/MachOObjectFile.cpp:483
@@ -470,1 +482,3 @@
+  StringRef SegmentName = getSectionFinalSegmentName(DR);
+  return SegmentName.str() + "," + *Result;
 }
----------------
rafael wrote:
> 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.
This was to retain existing llvm-objdump behavior.


Repository:
  rL LLVM

http://reviews.llvm.org/D11839