[llvm-objdump] Fix coloring with nested WithMarkup #113834

MaskRay · 2024-10-27T19:54:20Z

WithMarkup objects may nest, resulting in the ) in leaq (%rdx,%rax), %rbx to be green instead of the default color,
mismatching the color of (.

% llvm-mc -triple=x86_64 -mdis <<< '0x48 0x8d 0x1c 0x02'
        .text
        leaq    <mem:(<reg:%rdx>,<reg:%rax>)>, <reg:%rbx>

To ensure that ( and ) get the same color, maintain a color stack
within MCInstPrinter.

Fix #99661

Created using spr 1.3.5-bogner

llvmbot · 2024-10-27T19:54:56Z

@llvm/pr-subscribers-mc

@llvm/pr-subscribers-llvm-binary-utilities

Author: Fangrui Song (MaskRay)

Changes

WithMarkup objects may nest, resulting in the ) in leaq (%rdx,%rax), %rbx to be green instead of the default color,
mismatching the color of (.

% llvm-mc -triple=x86_64 -mdis &lt;&lt;&lt; '0x48 0x8d 0x1c 0x02'
        .text
        leaq    &lt;mem:(&lt;reg:%rdx&gt;,&lt;reg:%rax&gt;)&gt;, &lt;reg:%rbx&gt;

To ensure that ( and ) get the same color, maintain a color stack
within MCInstPrinter.

Full diff: https://github.com/llvm/llvm-project/pull/113834.diff

3 Files Affected:

(modified) llvm/include/llvm/MC/MCInstPrinter.h (+7-3)
(modified) llvm/lib/MC/MCInstPrinter.cpp (+17-10)
(added) llvm/test/tools/llvm-objdump/X86/disassemble-color.s (+21)

diff --git a/llvm/include/llvm/MC/MCInstPrinter.h b/llvm/include/llvm/MC/MCInstPrinter.h
index 60a901e3d0deae..c296a6fe1c045d 100644
--- a/llvm/include/llvm/MC/MCInstPrinter.h
+++ b/llvm/include/llvm/MC/MCInstPrinter.h
@@ -9,8 +9,10 @@
 #ifndef LLVM_MC_MCINSTPRINTER_H
 #define LLVM_MC_MCINSTPRINTER_H
 
+#include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/Format.h"
+#include "llvm/Support/raw_ostream.h"
 #include <cstdint>
 
 namespace llvm {
@@ -24,7 +26,6 @@ class MCRegister;
 class MCRegisterInfo;
 class MCSubtargetInfo;
 class StringRef;
-class raw_ostream;
 
 /// Convert `Bytes' to a hex string and output to `OS'
 void dumpBytes(ArrayRef<uint8_t> Bytes, raw_ostream &OS);
@@ -76,6 +77,8 @@ class MCInstPrinter {
   /// If true, symbolize branch target and memory reference operands.
   bool SymbolizeOperands = false;
 
+  SmallVector<raw_ostream::Colors, 4> ColorStack{raw_ostream::Colors::RESET};
+
   /// Utility function for printing annotations.
   void printAnnotation(raw_ostream &OS, StringRef Annot);
 
@@ -98,8 +101,8 @@ class MCInstPrinter {
 
   class WithMarkup {
   public:
-    LLVM_CTOR_NODISCARD WithMarkup(raw_ostream &OS, Markup M, bool EnableMarkup,
-                                   bool EnableColor);
+    LLVM_CTOR_NODISCARD WithMarkup(MCInstPrinter &IP, raw_ostream &OS, Markup M,
+                                   bool EnableMarkup, bool EnableColor);
     ~WithMarkup();
 
     template <typename T> WithMarkup &operator<<(T &O) {
@@ -113,6 +116,7 @@ class MCInstPrinter {
     }
 
   private:
+    MCInstPrinter &IP;
     raw_ostream &OS;
     bool EnableMarkup;
     bool EnableColor;
diff --git a/llvm/lib/MC/MCInstPrinter.cpp b/llvm/lib/MC/MCInstPrinter.cpp
index e4faeba04a8fd7..a57f4e19c81c6b 100644
--- a/llvm/lib/MC/MCInstPrinter.cpp
+++ b/llvm/lib/MC/MCInstPrinter.cpp
@@ -226,27 +226,32 @@ format_object<uint64_t> MCInstPrinter::formatHex(uint64_t Value) const {
 
 MCInstPrinter::WithMarkup MCInstPrinter::markup(raw_ostream &OS,
                                                 Markup S) const {
-  return WithMarkup(OS, S, getUseMarkup(), getUseColor());
+  return WithMarkup(const_cast<MCInstPrinter &>(*this), OS, S, getUseMarkup(),
+                    getUseColor());
 }
 
-MCInstPrinter::WithMarkup::WithMarkup(raw_ostream &OS, Markup M,
-                                      bool EnableMarkup, bool EnableColor)
-    : OS(OS), EnableMarkup(EnableMarkup), EnableColor(EnableColor) {
+MCInstPrinter::WithMarkup::WithMarkup(MCInstPrinter &IP, raw_ostream &OS,
+                                      Markup M, bool EnableMarkup,
+                                      bool EnableColor)
+    : IP(IP), OS(OS), EnableMarkup(EnableMarkup), EnableColor(EnableColor) {
   if (EnableColor) {
+    raw_ostream::Colors Color = raw_ostream::Colors::RESET;
     switch (M) {
     case Markup::Immediate:
-      OS.changeColor(raw_ostream::RED);
+      Color = raw_ostream::RED;
       break;
     case Markup::Register:
-      OS.changeColor(raw_ostream::CYAN);
+      Color = raw_ostream::CYAN;
       break;
     case Markup::Target:
-      OS.changeColor(raw_ostream::YELLOW);
+      Color = raw_ostream::YELLOW;
       break;
     case Markup::Memory:
-      OS.changeColor(raw_ostream::GREEN);
+      Color = raw_ostream::GREEN;
       break;
     }
+    IP.ColorStack.push_back(Color);
+    OS.changeColor(Color);
   }
 
   if (EnableMarkup) {
@@ -270,6 +275,8 @@ MCInstPrinter::WithMarkup::WithMarkup(raw_ostream &OS, Markup M,
 MCInstPrinter::WithMarkup::~WithMarkup() {
   if (EnableMarkup)
     OS << '>';
-  if (EnableColor)
-    OS.resetColor();
+  if (!EnableColor)
+    return;
+  IP.ColorStack.pop_back();
+  OS << IP.ColorStack.back();
 }
diff --git a/llvm/test/tools/llvm-objdump/X86/disassemble-color.s b/llvm/test/tools/llvm-objdump/X86/disassemble-color.s
new file mode 100644
index 00000000000000..4e1d82562fb546
--- /dev/null
+++ b/llvm/test/tools/llvm-objdump/X86/disassemble-color.s
@@ -0,0 +1,21 @@
+# UNSUPPORTED: system-windows
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o %t
+# RUN: llvm-objdump -d --no-show-raw-insn --disassembler-color=on %t | FileCheck %s --check-prefix=ATT
+# RUN: llvm-objdump -d --no-show-raw-insn --disassembler-color=on -M intel %t | FileCheck %s --check-prefix=INTEL
+
+# ATT:      <.text>:
+# ATT-NEXT:  leaq	�[0;32m(�[0;36m%rdx�[0;32m,�[0;36m%rax�[0;32m,�[0;31m4�[0;32m)�[0m, �[0;36m%rbx�[0m
+# ATT-NEXT:  movq	�[0;32m(,�[0;36m%rax�[0;32m)�[0m, �[0;36m%rbx�[0m
+# ATT-NEXT:  leaq	�[0;32m0x3(�[0;36m%rdx�[0;32m,�[0;36m%rax�[0;32m)�[0m, �[0;36m%rbx�[0m
+# ATT-NEXT:  movq	�[0;31m$0x3�[0m, �[0;36m%rax�[0m
+
+# INTEL:      <.text>:
+# INTEL-NEXT:  lea	�[0;36mrbx�[0m, �[0;32m[�[0;36mrdx�[0;32m + 4*�[0;36mrax�[0;32m]�[0m
+# INTEL-NEXT:  mov	�[0;36mrbx�[0m, qword ptr �[0;32m[1*�[0;36mrax�[0;32m]�[0m
+# INTEL-NEXT:  lea	�[0;36mrbx�[0m, �[0;32m[�[0;36mrdx�[0;32m + �[0;36mrax�[0;32m + �[0;31m0x3�[0;32m]�[0m
+# INTEL-NEXT:  mov	�[0;36mrax�[0m, �[0;31m0x3�[0m
+
+leaq (%rdx,%rax,4), %rbx
+movq (,%rax), %rbx
+leaq 3(%rdx,%rax), %rbx
+movq $3, %rax

jh7370 · 2024-10-28T08:19:30Z

llvm/lib/MC/MCInstPrinter.cpp

@@ -226,27 +226,32 @@ format_object<uint64_t> MCInstPrinter::formatHex(uint64_t Value) const {

 MCInstPrinter::WithMarkup MCInstPrinter::markup(raw_ostream &OS,
                                                Markup S) const {
-  return WithMarkup(OS, S, getUseMarkup(), getUseColor());
+  return WithMarkup(const_cast<MCInstPrinter &>(*this), OS, S, getUseMarkup(),


Overall approach looks fine, but this does feel a bit like an abuse of const. Could const be dropped from the markup method?

Many const methods transitively call the const markup. If we want to drop const_cast here, many target functions printXXX will need be adjusted...

Yuck, okay. I think that's probably still the "right" thing to do (after all you're modifying the state of the class when calling these methods in this manner). But I acknowledge that's probably a bit impractical too, so if you add a TODO comment here instead, I'd be happy with that.

I also considered making the ColorStack member mutable, but that doesn't really feel any better than the const_cast in this context.

Migrated printRegName users. Dropped const_cast now.

jh7370 · 2024-10-29T08:38:09Z

llvm/lib/MC/MCInstPrinter.cpp

@@ -226,27 +226,32 @@ format_object<uint64_t> MCInstPrinter::formatHex(uint64_t Value) const {

 MCInstPrinter::WithMarkup MCInstPrinter::markup(raw_ostream &OS,
                                                Markup S) const {
-  return WithMarkup(OS, S, getUseMarkup(), getUseColor());
+  return WithMarkup(const_cast<MCInstPrinter &>(*this), OS, S, getUseMarkup(),


Yuck, okay. I think that's probably still the "right" thing to do (after all you're modifying the state of the class when calling these methods in this manner). But I acknowledge that's probably a bit impractical too, so if you add a TODO comment here instead, I'd be happy with that.

I also considered making the ColorStack member mutable, but that doesn't really feel any better than the const_cast in this context.

Cr0a3 · 2024-10-29T17:52:57Z

Thank you for fixing this

Similar to printInst. printRegName may change states (e.g. #113834).

Created using spr 1.3.5-bogner

Similar to printInst. printRegName may change states (e.g. llvm#113834).

WithMarkup objects may nest, resulting in the `)` in `leaq (%rdx,%rax), %rbx` to be green instead of the default color, mismatching the color of `(`. ``` % llvm-mc -triple=x86_64 -mdis <<< '0x48 0x8d 0x1c 0x02' .text leaq <mem:(<reg:%rdx>,<reg:%rax>)>, <reg:%rbx> ``` To ensure that `(` and `)` get the same color, maintain a color stack within MCInstPrinter. Fix llvm#99661 Pull Request: llvm#113834

[𝘀𝗽𝗿] initial version

c7e8cde

Created using spr 1.3.5-bogner

llvmbot added mc Machine (object) code llvm:binary-utilities labels Oct 27, 2024

MaskRay requested review from JDevlieghere and jh7370 October 27, 2024 19:54

MaskRay mentioned this pull request Oct 27, 2024

[BUG] Weird coloring in x86 assembly in llvm-objdump #99661

Closed

jh7370 reviewed Oct 28, 2024

View reviewed changes

jh7370 approved these changes Oct 29, 2024

View reviewed changes

MaskRay added a commit that referenced this pull request Oct 30, 2024

[MCInstPrinter] Make printRegName non-const

facdae6

Similar to printInst. printRegName may change states (e.g. #113834).

rebase to remove const_cast

ec8a701

Created using spr 1.3.5-bogner

MaskRay merged commit 29bff4a into main Oct 30, 2024
4 of 7 checks passed

MaskRay deleted the users/MaskRay/spr/llvm-objdump-fix-coloring-with-nested-withmarkup branch October 30, 2024 03:06

NoumanAmir657 pushed a commit to NoumanAmir657/llvm-project that referenced this pull request Nov 4, 2024

[MCInstPrinter] Make printRegName non-const

ef81955

Similar to printInst. printRegName may change states (e.g. llvm#113834).

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[llvm-objdump] Fix coloring with nested WithMarkup #113834

[llvm-objdump] Fix coloring with nested WithMarkup #113834

MaskRay commented Oct 27, 2024 •

edited

Loading

Uh oh!

llvmbot commented Oct 27, 2024 •

edited

Loading

Uh oh!

jh7370 Oct 28, 2024

Uh oh!

MaskRay Oct 28, 2024

Uh oh!

jh7370 Oct 29, 2024

Uh oh!

MaskRay Oct 30, 2024

Uh oh!

jh7370 Oct 29, 2024

Uh oh!

Cr0a3 commented Oct 29, 2024

Uh oh!

Uh oh!

Uh oh!

[llvm-objdump] Fix coloring with nested WithMarkup #113834

[llvm-objdump] Fix coloring with nested WithMarkup #113834

Conversation

MaskRay commented Oct 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Oct 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jh7370 Oct 28, 2024

Choose a reason for hiding this comment

Uh oh!

MaskRay Oct 28, 2024

Choose a reason for hiding this comment

Uh oh!

jh7370 Oct 29, 2024

Choose a reason for hiding this comment

Uh oh!

MaskRay Oct 30, 2024

Choose a reason for hiding this comment

Uh oh!

jh7370 Oct 29, 2024

Choose a reason for hiding this comment

Uh oh!

Cr0a3 commented Oct 29, 2024

Uh oh!

Uh oh!

Uh oh!

MaskRay commented Oct 27, 2024 •

edited

Loading

llvmbot commented Oct 27, 2024 •

edited

Loading