[PATCH] D36464: Implemented sane default for llvm-objdump's relocation Value format

[James Y Knight via Phabricator via llvm-commits]

jyknight added inline comments.


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:693
   uint8_t type = RelRef.getType();
-  StringRef res;
-  int64_t addend = 0;
+  Optional<int64_t> addend = 0;
   switch (Sec->sh_type) {
----------------
And after the other changes, I think this can go back to an int64_t.


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:739
     case ELF::R_X86_64_PC32: {
-      std::string fmtbuf;
-      raw_string_ostream fmt(fmtbuf);
-      fmt << Target << (addend < 0 ? "" : "+") << addend << "-P";
-      fmt.flush();
-      Result.append(fmtbuf.begin(), fmtbuf.end());
-    } break;
-    case ELF::R_X86_64_8:
-    case ELF::R_X86_64_16:
-    case ELF::R_X86_64_32:
-    case ELF::R_X86_64_32S:
-    case ELF::R_X86_64_64: {
-      std::string fmtbuf;
-      raw_string_ostream fmt(fmtbuf);
-      fmt << Target << (addend < 0 ? "" : "+") << addend;
-      fmt.flush();
-      Result.append(fmtbuf.begin(), fmtbuf.end());
+      suffix = "-P";
     } break;
----------------
This "-P" suffix here doesn't seem particularly useful, I'd suggest just removing it.

The reloc name is printed, I don't think the fact that PC is subtracted needs to be printed as part of the output here as well. (Perhaps the intent was to print the "full formula", but if so, this list is woefully incomplete, and I don't think that's particularly worthwhile to spend time implementing).


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:745
     break;
-  case ELF::EM_LANAI:
-  case ELF::EM_AVR:
-  case ELF::EM_AARCH64: {
-    std::string fmtbuf;
-    raw_string_ostream fmt(fmtbuf);
-    fmt << Target;
-    if (addend != 0)
-      fmt << (addend < 0 ? "" : "+") << addend;
-    fmt.flush();
-    Result.append(fmtbuf.begin(), fmtbuf.end());
-    break;
-  }
   case ELF::EM_386:
   case ELF::EM_IAMCU:
----------------
I believe this list of case statements which override addend = None is bogus and should go away. Most of these have SHT_REL relocations, which put the addend in the instruction -- and thus don't get printed anyways (see also TODO note above -- although I'd note that GNU objdump also doesn't print SHT_REL addends either). And when they _do_ actually have addends, I see no reason why we would not want them printed.

Other than that, looks good.


https://reviews.llvm.org/D36464