[PATCH] D72973: using symbol index+symbol name + storage mapping class as label for llvm-objdump -D

Hubert Tong via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Jan 27 07:53:22 PST 2020


hubert.reinterpretcast added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:1205
                                 uint64_t /*Offset*/, uint64_t /*InstSize*/) {
-  using SymbolInfoTy = std::tuple<uint64_t, StringRef, uint8_t>;
+  using SymbolInfoTy = std::tuple<uint64_t, StringRef, uint8_t, int64_t>;
   using SectionSymbolsTy = std::vector<SymbolInfoTy>;
----------------
hubert.reinterpretcast wrote:
> Is this a stray change that got picked up?
If the `SectionSymbolsTy` below needs to match the definition elsewhere, it is concerning that there is no common definition in an interface file that can be included from both places.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:4
+
+namespace llvm {
+
----------------
I suggest to remove this and qualify the function definition explicitly.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:6
+
+#define SymbolTypeMask 0x07
+
----------------
I suggest to use an enum defined inside the function body.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:8
+
+uint8_t getXCoffSymbolCsectSMC(const ObjectFile *Obj, const SymbolRef &Sym) {
+  assert(Obj->isXCOFF() && "Not XCOFF object file.");
----------------
It's getXCOFF a few lines down for the other similarly-named function. Let's keep this consistent.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:8
+
+uint8_t getXCoffSymbolCsectSMC(const ObjectFile *Obj, const SymbolRef &Sym) {
+  assert(Obj->isXCOFF() && "Not XCOFF object file.");
----------------
hubert.reinterpretcast wrote:
> It's getXCOFF a few lines down for the other similarly-named function. Let's keep this consistent.
Use `llvm::` here to help ensure that this function signature matches the one from the header at compile time.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:16
+    assert(XCOFFSymRef.getNumberOfAuxEntries() &&
+           "No Csect Auxiliary Entry is found.");
+    const XCOFFCsectAuxEnt32 *AuxEntPtr = XCOFFSymRef.getXCOFFCsectAuxEnt32();
----------------
s/Csect/CSECT/;


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:21
+  }
+  return XCOFF::XMC_MAX_SMC;
+}
----------------
Should we just return an `int` and use `-1` as a magic number? `XMC_MAX_SMC` does not really capture the semantics. It's the largest value that fits, but it is not currently valid. It is also not guaranteed not to become a valid value in the future. The `-1` magic number is more idiomatic here.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:346
+typedef std::vector<std::tuple<uint64_t, StringRef, uint8_t, int64_t>>
+    SectionSymbolsTy;
 
----------------
Does this belong in a header somewhere?


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.h:143
+
+uint8_t getXCoffSymbolCsectSMC(const llvm::object::ObjectFile *Obj,
+                               const llvm::object::SymbolRef &Sym);
----------------
`object::` is used for `llvm::object::` above.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72973/new/

https://reviews.llvm.org/D72973





More information about the llvm-commits mailing list