[PATCH] D74883: Add a llvm-gsymutil tool that can convert object files to GSYM and perform lookups.

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/lib/DebugInfo/GSYM/GsymCreator.cpp:249
+    if (auto Range = ValidTextRanges->getRangeThatContains(
+          Funcs.back().Range.Start)) {
+      Funcs.back().Range.End = Range->End;
----------------
LLVM code tends to delete excess parentheses.


================
Comment at: llvm/lib/DebugInfo/GSYM/ObjectFileTransformer.cpp:90
+    // Function size for MachO files will be 0
+    constexpr bool NoCopy = false;
+    const uint64_t size = IsELF ? ELFSymbolRef(Sym).getSize() : 0;
----------------
NoCopy is only used once.

Deleting it and using `/*NoCopy=*/false` at the call site. will be clearer/simpler.


================
Comment at: llvm/test/tools/llvm-gsymutil/cmdline.test:2
+RUN: llvm-gsymutil -h 2>&1 | FileCheck --check-prefix=HELP %s
+RUN: llvm-gsymutil --help 2>&1 | FileCheck --check-prefix=HELP %s
+HELP: OVERVIEW: A tool for dumping, searching and creating GSYM files.
----------------
See test/tools/llvm-strings/help.test for an example


================
Comment at: llvm/tools/llvm-gsym/llvm-gsymutil.cpp:47
+using namespace llvm;
+using namespace gsym;
+using namespace object;
----------------
`using namespace llvm::gsym;` is preferred.


================
Comment at: llvm/tools/llvm-gsym/llvm-gsymutil.cpp:48
+using namespace gsym;
+using namespace object;
+
----------------
`using namespace llvm::object;` is preferred.


================
Comment at: llvm/tools/llvm-gsym/llvm-gsymutil.cpp:221
+
+/// Determine the virtual address that is considered the base address of mach-o
+/// object file.
----------------
Missing `a`


================
Comment at: llvm/tools/llvm-gsym/llvm-gsymutil.cpp:231
+getImageBaseAddress(const object::MachOObjectFile *MachO) {
+  for (const auto &Command : MachO->load_commands()) {
+    if (Command.C.cmd == MachO::LC_SEGMENT) {
----------------
Write `auto` type explicitly, unless obvious.


================
Comment at: llvm/tools/llvm-gsym/llvm-gsymutil.cpp:328
+  // debug info and also a symbol table entry from the object file.
+  if (auto Err = Gsym.finalize(OS))
+    return Err;
----------------
`auto` -> `Error`




================
Comment at: llvm/tools/llvm-gsym/llvm-gsymutil.cpp:427
+  // Print a stack trace if we signal out.
+  sys::PrintStackTraceOnErrorSignal(argv[0]);
+  PrettyStackTraceProgram X(argc, argv);
----------------
`InitLLVM X(argc, argv);`.

You can find some modern cases in tools/llvm-objcopy and tools/llvm-readobj.


================
Comment at: llvm/tools/llvm-gsym/llvm-gsymutil.cpp:434
+  llvm::InitializeAllTargets();
+  llvm::InitializeAllAsmPrinters();
+
----------------
If AsmPrinter needed?


================
Comment at: llvm/tools/llvm-gsym/llvm-gsymutil.cpp:449
+  if (Help) {
+    PrintHelpMessage(/*Hidden =*/false, /*Categorized =*/true);
+    return 0;
----------------
clang-format prefers `/*Hidden=*/false` (no space).


================
Comment at: llvm/tools/llvm-gsym/llvm-gsymutil.cpp:453
+
+  raw_ostream &OS = outs();
+
----------------
There are other `outs()` references below. The `OS` alias is not useful.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74883