Address other review comments

Author: ostannard

llvm/docs/CommandGuide/llvm-objdump.rst

@@ -291,6 +291,36 @@ OPTIONS
 
   Target triple to disassemble for, see ``--version`` for available targets.
 
+.. option:: --visualize-jumps=<modes>
+
+  Display a control-flow graph which shows the targets of branch instructions to the left of disasembly. ``modes`` is a comma-separated list of options, which configure the character set and used to print the graph.
+
+  If ``modes`` is omitted, the default is ``unicode,auto``
+
+   .. option:: off
+
+    Disable control-flow graph
+
+   .. option:: ascii
+
+    Render control-flow graph using ASCII characters
+
+   .. option:: unicode
+
+    Render control-flow graph using unicode box-drawing characters
+
+   .. option:: nocolor
+
+    Render control-flow graph without using colors
+
+   .. option:: auto
+
+    Render control-flow graph using colors if supported by the terminal
+
+   .. option:: color
+
+    Render control-flow graph using colors
+
 .. option:: -w, --wide
 
   Ignored for compatibility with GNU objdump.

llvm/tools/llvm-objdump/SourcePrinter.cpp

@@ -211,19 +211,19 @@ const char *LiveVariablePrinter::getLineChar(LineChar C) const {
   bool IsASCII = DbgVariables == DVASCII;
   switch (C) {
   case LineChar::RangeStart:
-    return IsASCII ? "^" : (const char *)u8"\u2548";
+    return IsASCII ? "^" : u8"\u2548";
   case LineChar::RangeMid:
-    return IsASCII ? "|" : (const char *)u8"\u2503";
+    return IsASCII ? "|" : u8"\u2503";
   case LineChar::RangeEnd:
-    return IsASCII ? "v" : (const char *)u8"\u253b";
+    return IsASCII ? "v" : u8"\u253b";
   case LineChar::LabelVert:
-    return IsASCII ? "|" : (const char *)u8"\u2502";
+    return IsASCII ? "|" : u8"\u2502";
   case LineChar::LabelCornerNew:
-    return IsASCII ? "/" : (const char *)u8"\u250c";
+    return IsASCII ? "/" : u8"\u250c";
   case LineChar::LabelCornerActive:
-    return IsASCII ? "|" : (const char *)u8"\u2520";
+    return IsASCII ? "|" : u8"\u2520";
   case LineChar::LabelHoriz:
-    return IsASCII ? "-" : (const char *)u8"\u2500";
+    return IsASCII ? "-" : u8"\u2500";
   }
   llvm_unreachable("Unhandled LineChar enum");
 }
@@ -516,6 +516,15 @@ const raw_ostream::Colors LineColors[] = {
   raw_ostream::CYAN,
 };
 
+raw_ostream::Colors ControlFlowPrinter::PickColor() {
+  if (!OutputMode.color_enabled())
+    return raw_ostream::RESET;
+  auto Ret = LineColors[NextColorIdx];
+  NextColorIdx =
+      (NextColorIdx + 1) % (sizeof(LineColors) / sizeof(LineColors[0]));
+  return Ret;
+}
+
 void ControlFlowPrinter::addEdge(uint64_t From, uint64_t To) {
   auto It = Targets.find(To);
   if (It == Targets.end())
@@ -551,32 +560,24 @@ void ControlFlowPrinter::finalise() {
     T->Column = Column;
     MaxColumn = std::max(MaxColumn, Column);
 
-#if 1
-    LLVM_DEBUG(
-      dbgs() << "Target: 0x" << Twine::utohexstr(T->Target) << " (" << T->Length()
-             << ") Column " << Column << ":\n";
-      for (auto Source : T->Sources)
-        dbgs() << "  Source: 0x" << Twine::utohexstr(Source) << "\n";
-    );
-#endif
   }
 
   setControlFlowColumnWidth(MaxColumn * 2 + 4);
 }
 
 const char *ControlFlowPrinter::getLineChar(LineChar C) const {
-  bool IsASCII = OutputMode.Chars == VisualizeJumpsMode::ASCII;
+  bool IsASCII = !OutputMode.unicode_enabled();
   switch (C) {
   case LineChar::Horiz:
-    return IsASCII ? "-" : (const char *)u8"\u2500";
+    return IsASCII ? "-" : u8"\u2500";
   case LineChar::Vert:
-    return IsASCII ? "|" : (const char *)u8"\u2502";
+    return IsASCII ? "|" : u8"\u2502";
   case LineChar::TopCorner:
-    return IsASCII ? "/" : (const char *)u8"\u256d";
+    return IsASCII ? "/" : u8"\u256d";
   case LineChar::BottomCorner:
-    return IsASCII ? "\\" : (const char *)u8"\u2570";
+    return IsASCII ? "\\" : u8"\u2570";
   case LineChar::Tee:
-    return IsASCII ? "+" : (const char *)u8"\u251c";
+    return IsASCII ? "+" : u8"\u251c";
   case LineChar::Arrow:
     return ">";
   }

llvm/tools/llvm-objdump/SourcePrinter.h

@@ -184,6 +184,7 @@ struct VisualizeJumpsMode {
 
   bool enabled() const { return Chars != Off; }
   bool color_enabled() const { return enabled() && Colors != BlackAndWhite; }
+  bool unicode_enabled() const { return Chars == Unicode; }
 
   void ResolveAutoColor(raw_ostream &OS) {
     if (Colors == Auto)
@@ -246,14 +247,7 @@ class ControlFlowPrinter {
   const MCSubtargetInfo &STI;
 
   int NextColorIdx;
-  raw_ostream::Colors PickColor() {
-    if ((OutputMode & VisualizeJumpsMode::ColorMask) ==
-        VisualizeJumpsMode::Off)
-      return raw_ostream::RESET;
-    auto Ret = LineColors[NextColorIdx];
-    NextColorIdx = (NextColorIdx + 1) % (sizeof(LineColors) / sizeof(LineColors[0]));
-    return Ret;
-  }
+  raw_ostream::Colors PickColor();
 
   int getIndentLevel() const { return 10; }
 

llvm/tools/llvm-objdump/llvm-objdump.cpp

@@ -514,9 +514,8 @@ unsigned objdump::GetColumnIndent(MCSubtargetInfo const &STI,
   // Special case for assembly string: the assembly printer uses tabs, so we
   // need to ensure we start the instruction on a tab stop (multiple of 8).
   Indent = alignTo(Indent, 8);
-  if (Col == DisassemblyColumn::Assembly) {
+  if (Col == DisassemblyColumn::Assembly)
     return Indent;
-  }
 
   // Assembly width can be configured with --debug-vars-indent=
   // FIXME this variable name is confusing.
@@ -1376,8 +1375,8 @@ collectLocalBranchTargets(ArrayRef<uint8_t> Bytes, MCInstrAnalysis *MIA,
   End += SectionAddr;
   uint64_t Index = Start;
 
-  std::vector<RelocationRef>::const_iterator RelCur = Relocs.begin();
-  std::vector<RelocationRef>::const_iterator RelEnd = Relocs.end();
+  auto RelCur = Relocs.begin();
+  auto RelEnd = Relocs.end();
 
   while (Index < End) {
     // Disassemble a real instruction and record function-local branch labels.