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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/lib/DebugInfo/GSYM/FunctionInfo.cpp:173
   // gap between functions are after the last function.
-  if (Addr >= LR.FuncRange.End)
+  if (LR.FuncRange.size() > 0 && !LR.FuncRange.contains(Addr))
     return createStringError(std::errc::io_error,
----------------
Use !empty() rather than size() > 0


================
Comment at: llvm/lib/DebugInfo/GSYM/GsymCreator.cpp:247
+  // has no size when doing lookups.
+  if (!Funcs.empty() && Funcs.back().Range.size() == 0 && ValidTextRanges) {
+    if (auto Range = ValidTextRanges->getRangeThatContains(
----------------
Use empty() rather than size() == 0


================
Comment at: llvm/lib/DebugInfo/GSYM/LookupResult.cpp:41
+    OS << " + " << SL.Offset;
+  if (SL.Dir.size() || SL.Base.size()) {
+    OS << " @ ";
----------------
Use early return to reduce indentation here, probably?


================
Comment at: llvm/lib/DebugInfo/GSYM/ObjectFileTransformer.cpp:28-32
+  if (auto *MachO = dyn_cast<object::MachOObjectFile>(&Obj)) {
+    const ArrayRef<uint8_t> MachUUID = MachO->getUuid();
+    if (!MachUUID.empty())
+      UUID.assign(MachUUID.data(), MachUUID.data() + MachUUID.size());
+  } else if (isa<object::ELFObjectFileBase>(&Obj)) {
----------------
Might be worth reducing indentation here:

```
if (MachO)
  ...
  return UUID;
}
if (!ELF)
  return {};

/* all the ELF handling here, not indented */
```


================
Comment at: llvm/lib/DebugInfo/GSYM/ObjectFileTransformer.cpp:30
+    const ArrayRef<uint8_t> MachUUID = MachO->getUuid();
+    if (!MachUUID.empty())
+      UUID.assign(MachUUID.data(), MachUUID.data() + MachUUID.size());
----------------
This test doesn't seem to change the behavior of the program? (ie: copying an empty ArrayRef is a no-op here, so remove the conditional to simplify the code)


================
Comment at: llvm/lib/DebugInfo/GSYM/ObjectFileTransformer.cpp:40
+      }
+      StringRef SectName(*SectNameOrErr);
+      if (SectName != GNUBuildID)
----------------
Use = rather than () to ensure only implicit conversions are in use:

```
StringRef SectName = *SectNameOrErr;
```

(applies in otehr places in this patch too)


================
Comment at: llvm/lib/DebugInfo/GSYM/ObjectFileTransformer.cpp:45-50
+      if (E)
+        BuildIDData = *E;
+      else {
+        consumeError(E.takeError());
+        continue;
+      }
----------------
Invert & unindent this:

```
if (!E) {
  consumeError(E.takeError());
  continue;
}

StringRef BuildIDData = *E;
...
```


================
Comment at: llvm/lib/DebugInfo/GSYM/ObjectFileTransformer.cpp:75
+
+  const bool IsMachO = isa<MachOObjectFile>(&Obj);
+  const bool IsELF = isa<ELFObjectFileBase>(&Obj);
----------------
Please remove const from local variables - this isn't common in LLVM & isn't common/consistent in this patch/code. 


================
Comment at: llvm/tools/llvm-gsym/llvm-gsymutil.cpp:171-174
+  if (MachO.is64Bit())
+    return MachO.getHeader64().cputype;
+  else
+    return MachO.getHeader().cputype;
----------------
Please avoid else-after-return: https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return


================
Comment at: llvm/tools/llvm-gsym/llvm-gsymutil.cpp:259-270
+static llvm::Optional<uint64_t> getImageBaseAddress(object::ObjectFile &Obj) {
+  if (const auto *MachO = dyn_cast<object::MachOObjectFile>(&Obj))
+    return getImageBaseAddress(MachO);
+  else if (const auto *ELFObj = dyn_cast<object::ELF32LEObjectFile>(&Obj))
+    return getImageBaseAddress(ELFObj->getELFFile());
+  else if (const auto *ELFObj = dyn_cast<object::ELF32BEObjectFile>(&Obj))
+    return getImageBaseAddress(ELFObj->getELFFile());
----------------
(more else-after-return)


================
Comment at: llvm/tools/llvm-gsym/llvm-gsymutil.cpp:296
+      continue;
+    const uint64_t Size = Sect.getSize();
+    if (Size == 0)
----------------
LLVM doesn't tend to use "const" on locals (& it isn't applied consistently even in this code, for instance - "ThreadCount", DICtx, Endian are not const). Please remove it. (it can be misleading/confusing - might make a reader think it was meant to be const reference)


================
Comment at: llvm/tools/llvm-gsym/llvm-gsymutil.cpp:502
+  }
+  return EXIT_SUCCESS;
+}
----------------
This is implicit in C++ - do many other LLVM utilities have this line? I'd expect the dominant paradigm is to not have an explicit return 0/EXIT_SUCCESS & rely on C++'s default here -and if that is the dominant paradigm in LLVM, please follow it here & remove this line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74883