[llvm-objdump] Fix --disassembler-color with --macho flag #163815
+32
−12
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The --disassembler-color option was being ignored when using the --macho flag because DisassembleMachO() never called setUseColor() on the instruction printer.
|
@llvm/pr-subscribers-llvm-binary-utilities Author: Ryan Mansfield (rjmansfield) ChangesThe --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:
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;
|
jh7370
reviewed
Oct 17, 2025
Add missing period. Add setupMachOInstPrinter function.
|
Test failure is unrelated: |
jh7370
approved these changes
Oct 20, 2025
There was a problem hiding this comment.
LGTM (with nit), but maybe worth getting @drodriguez to give it a quick once-over too.
| return DbgObj; | ||
| } | ||
|
|
||
| static void setupMachOInstPrinter(MCInstPrinter *IP) { |
There was a problem hiding this comment.
Nit: setupMachOInstPrinter -> setUpMachOInstPrinter
"setup" == noun
"set up" == verb/adjective
drodriguez
reviewed
Oct 20, 2025
There was a problem hiding this comment.
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
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.
Comment on lines
+7238
to
+7253
| 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; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
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());
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The --disassembler-color option was being ignored when using the --macho flag because DisassembleMachO() never called setUseColor() on the instruction printer.