[PATCH] D135117: [objdump] Support finding --source via --dsym files

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/test/tools/llvm-objdump/MachO/disassemble-source-dsym.test:1
+# objdump --source w/ explicit .dSYM
+# RUN: dsymutil -f    %p/../../dsymutil/Inputs/basic.macho.x86_64 -o %t1.dSYM         -oso-prepend-path=%p/../../dsymutil
----------------
Optional: newer test/tools tests use `## ` for non-RUN non-CHECK comments. That makes comments stand out and helps possibly future FileCheck's more rigid rule catching unused prefixes.


================
Comment at: llvm/test/tools/llvm-objdump/MachO/disassemble-source-dsym.test:3
+# RUN: dsymutil -f    %p/../../dsymutil/Inputs/basic.macho.x86_64 -o %t1.dSYM         -oso-prepend-path=%p/../../dsymutil
+# RUN: llvm-objdump < %p/../../dsymutil/Inputs/basic.macho.x86_64 - --source  --dsym=%t1.dSYM  --prefix=%p/../../dsymutil| \
+# RUN:   FileCheck --check-prefix=SOURCE %s
----------------
Add a comment if this intentionally tests reading from stdin?


================
Comment at: llvm/test/tools/llvm-objdump/MachO/disassemble-source-dsym.test:9
+# RUN: dsymutil     -oso-prepend-path=%p/../../dsymutil/ %t2
+# RUN: llvm-objdump --source --prefix=%p/../../dsymutil  %t2 | \
+# RUN:   FileCheck --check-prefix=SOURCE %s
----------------
The line isn't long enough that needs wrapping...


================
Comment at: llvm/tools/llvm-objdump/MachODump.cpp:7457
+      }
+    } else if (auto UB = dyn_cast<MachOUniversalBinary>(DSYMBinary.get())) {
+      // this is a Universal Binary, find a Mach-O for this architecture
----------------



================
Comment at: llvm/tools/llvm-objdump/MachODump.cpp:7484
+      WithColor::error(errs(), "llvm-objdump")
+          << DSYMPath << " is not a Mach-O or Universal file type.\n";
+      return nullptr;
----------------
https://llvm.org/docs/CodingStandards.html#error-and-warning-messages

The convention is omit the trailing period.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135117/new/

https://reviews.llvm.org/D135117