[llvm-objdump] Fix --disassembler-color with --macho flag #163815

rjmansfield · 2025-10-16T16:15:54Z

The --disassembler-color option was being ignored when using the --macho flag because DisassembleMachO() never called setUseColor() on the instruction printer.

llvmbot · 2025-10-16T16:16:27Z

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

Author: Ryan Mansfield (rjmansfield)

Changes

The --disassembler-color option was being ignored when using the --macho flag because DisassembleMachO() never called setUseColor() on the instruction printer.

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

4 Files Affected:

(modified) llvm/test/tools/llvm-objdump/MachO/arm64-disassembly-color.s (+4)
(modified) llvm/tools/llvm-objdump/MachODump.cpp (+26)
(modified) llvm/tools/llvm-objdump/llvm-objdump.cpp (+1-8)
(modified) llvm/tools/llvm-objdump/llvm-objdump.h (+8)

diff --git a/llvm/test/tools/llvm-objdump/MachO/arm64-disassembly-color.s b/llvm/test/tools/llvm-objdump/MachO/arm64-disassembly-color.s
index 50cd24c766763..b612e9a1b3f81 100644
--- a/llvm/test/tools/llvm-objdump/MachO/arm64-disassembly-color.s
+++ b/llvm/test/tools/llvm-objdump/MachO/arm64-disassembly-color.s
@@ -6,6 +6,10 @@
 // RUN: llvm-objdump --disassembler-color=off --disassemble %t | FileCheck %s --check-prefix=NOCOLOR
 // RUN: llvm-objdump --disassembler-color=terminal --disassemble %t | FileCheck %s --check-prefix=NOCOLOR
 
+// Test with --macho flag to ensure DisassembleMachO respects color settings
+// RUN: llvm-objdump --disassembler-color=on --macho --disassemble %t | FileCheck %s --check-prefix=COLOR
+// RUN: llvm-objdump --disassembler-color=off --macho --disassemble %t | FileCheck %s --check-prefix=NOCOLOR
+
 sub	sp, sp, #16
 str	w0, [sp, #12]
 ldr	w8, [sp, #12]
diff --git a/llvm/tools/llvm-objdump/MachODump.cpp b/llvm/tools/llvm-objdump/MachODump.cpp
index 8e9c91fde544d..b707617928f95 100644
--- a/llvm/tools/llvm-objdump/MachODump.cpp
+++ b/llvm/tools/llvm-objdump/MachODump.cpp
@@ -7321,6 +7321,19 @@ static void DisassembleMachO(StringRef Filename, MachOObjectFile *MachOOF,
   CHECK_TARGET_INFO_CREATION(IP);
   // Set the display preference for hex vs. decimal immediates.
   IP->setPrintImmHex(PrintImmHex);
+  // Set up disassembler color output.
+  switch (DisassemblyColor) {
+  case ColorOutput::Enable:
+    IP->setUseColor(true);
+    break;
+  case ColorOutput::Auto:
+    IP->setUseColor(outs().has_colors());
+    break;
+  case ColorOutput::Disable:
+  case ColorOutput::Invalid:
+    IP->setUseColor(false);
+    break;
+  }
   // Comment stream and backing vector.
   SmallString<128> CommentsToEmit;
   raw_svector_ostream CommentStream(CommentsToEmit);
@@ -7374,6 +7387,19 @@ static void DisassembleMachO(StringRef Filename, MachOObjectFile *MachOOF,
     CHECK_THUMB_TARGET_INFO_CREATION(ThumbIP);
     // Set the display preference for hex vs. decimal immediates.
     ThumbIP->setPrintImmHex(PrintImmHex);
+    // Set up disassembler color output.
+    switch (DisassemblyColor) {
+    case ColorOutput::Enable:
+      ThumbIP->setUseColor(true);
+      break;
+    case ColorOutput::Auto:
+      ThumbIP->setUseColor(outs().has_colors());
+      break;
+    case ColorOutput::Disable:
+    case ColorOutput::Invalid:
+      ThumbIP->setUseColor(false);
+      break;
+    }
   }
 
 #undef CHECK_TARGET_INFO_CREATION
diff --git a/llvm/tools/llvm-objdump/llvm-objdump.cpp b/llvm/tools/llvm-objdump/llvm-objdump.cpp
index 3ec644a472bfc..aa3106f268397 100644
--- a/llvm/tools/llvm-objdump/llvm-objdump.cpp
+++ b/llvm/tools/llvm-objdump/llvm-objdump.cpp
@@ -291,13 +291,6 @@ class BBAddrMapInfo {
 
 #define DEBUG_TYPE "objdump"
 
-enum class ColorOutput {
-  Auto,
-  Enable,
-  Disable,
-  Invalid,
-};
-
 static uint64_t AdjustVMA;
 static bool AllHeaders;
 static std::string ArchName;
@@ -310,7 +303,7 @@ bool objdump::SymbolDescription;
 bool objdump::TracebackTable;
 static std::vector<std::string> DisassembleSymbols;
 static bool DisassembleZeroes;
-static ColorOutput DisassemblyColor;
+ColorOutput objdump::DisassemblyColor;
 DIDumpType objdump::DwarfDumpType;
 static bool DynamicRelocations;
 static bool FaultMapSection;
diff --git a/llvm/tools/llvm-objdump/llvm-objdump.h b/llvm/tools/llvm-objdump/llvm-objdump.h
index 3525be9a5314a..ed0918c93c28e 100644
--- a/llvm/tools/llvm-objdump/llvm-objdump.h
+++ b/llvm/tools/llvm-objdump/llvm-objdump.h
@@ -43,6 +43,13 @@ namespace objdump {
 
 enum DebugFormat { DFASCII, DFDisabled, DFInvalid, DFLimitsOnly, DFUnicode };
 
+enum class ColorOutput {
+  Auto,
+  Enable,
+  Disable,
+  Invalid,
+};
+
 extern bool ArchiveHeaders;
 extern int DbgIndent;
 extern DebugFormat DbgVariables;
@@ -51,6 +58,7 @@ extern bool Demangle;
 extern bool Disassemble;
 extern bool DisassembleAll;
 extern std::vector<std::string> DisassemblerOptions;
+extern ColorOutput DisassemblyColor;
 extern DIDumpType DwarfDumpType;
 extern std::vector<std::string> FilterSections;
 extern bool LeadingAddr;

llvm/test/tools/llvm-objdump/MachO/arm64-disassembly-color.s

llvm/tools/llvm-objdump/MachODump.cpp

Add missing period. Add setupMachOInstPrinter function.

rjmansfield · 2025-10-17T15:46:58Z

Test failure is unrelated:

# | Input was:
# | <<<<<<
# |              1: opt: /home/gha/actions-runner/_work/llvm-project/llvm-project/polly/lib/Support/RegisterPasses.cpp:482: llvm::Expected<bool> polly::parseCGPipeline(StringRef, llvm::CGSCCPassManager &, PassInstrumentationCallbacks *, ArrayRef<PassBuilder::PipelineElement>): Assertion `Pipeline.empty()' failed. 
# | check:205'0     X~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: no match found
# |              2: PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace and instructions to reproduce the bug. 
# | check:205'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# |              3: Stack dump: 
# | check:205'0     ~~~~~~~~~~~~
# |              4: 0. Program arguments: /home/gha/actions-runner/_work/llvm-project/llvm-project/build/bin/opt -disable-output -debug-pass-manager -passes=no-op-cgscc(no-op-cgscc,whatever) /home/gha/actions-runner/_work/llvm-project/llvm-project/llvm/test/Other/pass-pipeline-parsing.ll 
# | check:205'0

jh7370

LGTM (with nit), but maybe worth getting @drodriguez to give it a quick once-over too.

jh7370 · 2025-10-20T08:08:45Z

llvm/tools/llvm-objdump/MachODump.cpp

  return DbgObj;
 }

+static void setupMachOInstPrinter(MCInstPrinter *IP) {


Nit: setupMachOInstPrinter -> setUpMachOInstPrinter

"setup" == noun
"set up" == verb/adjective

drodriguez

As with #163810 I am not sure if it is worth it to replicate everything that works without --macho in the code that that --macho seems to diverge to. Maybe the code in

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

Lines 3476 to 3479 in 35b9f20

    
           if (MachOOpt) { 
        
             parseInputMachO(file); 
        
             return; 
        
           }

and around other usages of MachOOpt needs to be adjusted better to avoid going into special code for Mach-O if the normal llvm-objdump code will work anyway.
Otherwise I think we are going to replicate a lot of code that already works for Mach-O when not passing --macho because passing --macho takes a completely different path because 14 years ago it made sense to have specific code for Mach-O.
Just my 2 cents.

drodriguez · 2025-10-20T21:34:13Z

llvm/tools/llvm-objdump/MachODump.cpp

+static void setUpMachOInstPrinter(MCInstPrinter *IP) {
+  IP->setPrintImmHex(PrintImmHex);
+  switch (DisassemblyColor) {
+  case ColorOutput::Enable:
+    IP->setUseColor(true);
+    break;
+  case ColorOutput::Auto:
+    IP->setUseColor(outs().has_colors());
+    break;
+  case ColorOutput::Disable:
+  case ColorOutput::Invalid:
+    IP->setUseColor(false);
+    break;
+  }
+}
+


I am a little preoccupied about having a std::unique_ptr and having to break the idea of single ownership with .get() to pass into this helper function. I think the usage of std::unique_ptr<>& as the argument type would have the same semantics and I think it also should be avoided.

If setUseColor just accepts a boolean, this helper can be just to calculate the boolean, and the usages below can be just a call to setUseColor.

static bool shouldInstPrinterUseColor() { switch (DisassemblyColor) { case ColorOutput::Enable: return true; case ColorOutput::Auto: return outs().has_colors(); case ColorOutput::Disable: case ColorOutput::Invalid: return false; } return false; // default to no color } ... IP->setPrintImmHex(PrintImmHex); IP->setUseColor(shouldInstPrinterUseColor());

[llvm-objdump] Fix --disassembler-color with --macho flag

f433720

The --disassembler-color option was being ignored when using the --macho flag because DisassembleMachO() never called setUseColor() on the instruction printer.

llvmbot added the llvm:binary-utilities label Oct 16, 2025

jh7370 reviewed Oct 17, 2025

View reviewed changes

llvm/test/tools/llvm-objdump/MachO/arm64-disassembly-color.s Outdated Show resolved Hide resolved

llvm/tools/llvm-objdump/MachODump.cpp Outdated Show resolved Hide resolved

Address reviewer feedback.

57f1c98

Add missing period. Add setupMachOInstPrinter function.

jh7370 approved these changes Oct 20, 2025

View reviewed changes

Rename setupMachOInstPrinter to setUpMachOInstPrinter.

c2f893e

drodriguez reviewed Oct 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[llvm-objdump] Fix --disassembler-color with --macho flag #163815

[llvm-objdump] Fix --disassembler-color with --macho flag #163815

Uh oh!

rjmansfield commented Oct 16, 2025

Uh oh!

llvmbot commented Oct 16, 2025

Uh oh!

Uh oh!

Uh oh!

rjmansfield commented Oct 17, 2025

Uh oh!

jh7370 left a comment

Uh oh!

jh7370 Oct 20, 2025

Uh oh!

drodriguez left a comment

Uh oh!

drodriguez Oct 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

[llvm-objdump] Fix --disassembler-color with --macho flag #163815

Are you sure you want to change the base?

[llvm-objdump] Fix --disassembler-color with --macho flag #163815

Uh oh!

Conversation

rjmansfield commented Oct 16, 2025

Uh oh!

llvmbot commented Oct 16, 2025

Uh oh!

Uh oh!

Uh oh!

rjmansfield commented Oct 17, 2025

Uh oh!

jh7370 left a comment

Choose a reason for hiding this comment

Uh oh!

jh7370 Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

drodriguez left a comment

Choose a reason for hiding this comment

Uh oh!

drodriguez Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants