[PATCH] D53379: GSYM symbolication format

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere added a comment.

Hi Greg, I did a high level pass over your patch. Personally I'm okay with keeping the format in sync with what you have at Facebook as long as the style issues are addressed before we land this.



================
Comment at: include/llvm/DebugInfo/GSYM/DwarfTransformer.h:34
+
+  std::error_code loadDwarf(const object::ObjectFile &Obj);
+  std::error_code loadDwarf(StringRef filename) {
----------------
Have you considered using `llvm::Error`? You can use it as a wrapper for error codes and has an assert that the error is dealt with. At some point you'll probably have to convert it to an error anyway so might as well do it at the source (imho). 


================
Comment at: include/llvm/DebugInfo/GSYM/DwarfTransformer.h:36
+  std::error_code loadDwarf(StringRef filename) {
+    if (auto binary = getObjectFile(filename)) {
+      return loadDwarf(*binary.getValue().getBinary());
----------------
No braces around single statement block according to the LLVM style guide.


================
Comment at: include/llvm/DebugInfo/GSYM/DwarfTransformer.h:40
+    return std::make_error_code(std::errc::invalid_argument);
+;
+  }
----------------
Spurious `;`


================
Comment at: include/llvm/DebugInfo/GSYM/FileTableCreator.h:26
+class FileTableCreator {
+  //std::unordered_map<llvm::gsym::FileEntry, uint32_t> EntryToIndex;
+  DenseMap<llvm::gsym::FileEntry, uint32_t> EntryToIndex;
----------------
Commented out code.


================
Comment at: include/llvm/DebugInfo/GSYM/GsymCreator.h:47
+  uint32_t insertString(StringRef S) {
+    std::lock_guard<std::mutex> guard(Mutex);
+    return StrTab.insert(S.str());
----------------
s/guard/Guard/


================
Comment at: include/llvm/DebugInfo/GSYM/GsymReader.h:56
+  static size_t getByteSize() { return sizeof(Header); }
+  std::string getError() const;
+  void dump(llvm::raw_ostream &OS) const;
----------------
llvm::Error?


================
Comment at: lib/DebugInfo/GSYM/Breakpad.cpp:49
+
+  BreakpadLineType GetLineType() {
+    static StringRef BPAD_MODULE("MODULE ");
----------------
lowerCamelCase for methods.


================
Comment at: lib/DebugInfo/GSYM/DwarfTransformer.cpp:161
+  StringRef ShortName(Die.getName(DINameKind::ShortName));
+  if (ShortName.empty()) {
+    return "";
----------------
No braces. Same for the single-line if-blocks below.


================
Comment at: lib/DebugInfo/GSYM/DwarfTransformer.cpp:216
+  }
+  if (checkChildren) {
+    for (DWARFDie child : Die.children()) {
----------------
You could make this an early return and save some indentation.


================
Comment at: lib/DebugInfo/GSYM/DwarfTransformer.cpp:246
+    }
+    if (ii.Ranges.empty()) {
+      return;
----------------
No braces, same for the rest of the file.


================
Comment at: lib/DebugInfo/GSYM/DwarfTransformer.cpp:262
+    parent.Children.emplace_back(std::move(ii));
+  } else if (tag == dwarf::DW_TAG_subprogram ||
+             tag == dwarf::DW_TAG_lexical_block) {
----------------
Make this an early return. 


================
Comment at: lib/DebugInfo/GSYM/DwarfTransformer.cpp:494
+
+
+  for (const object::SymbolRef &s : Obj.symbols()) {
----------------
Can you run clang-format over you patch (again)? It should've removed these double newlines.


================
Comment at: lib/DebugInfo/GSYM/GsymReader.cpp:212
+  }
+  return DataExtractor(StringRef(), true, 8);
+}
----------------
Since there's no dependency for the fall-through case I'd turn this into an early return as well, and duplicate this line. 


================
Comment at: lib/DebugInfo/GSYM/GsymReader.cpp:215
+
+const char *GsymReader::getInfoTypeAsString(InfoType IT) {
+  switch (IT) {
----------------
StringRef


================
Comment at: lib/DebugInfo/GSYM/GsymReader.cpp:227
+
+// void GsymReader::FileTable::dump(llvm::raw_ostream &OS,
+//                                  const StringTable &StrTab) const {
----------------
Remove commented-out code.


================
Comment at: lib/DebugInfo/GSYM/GsymReader.cpp:406
+  case 2: {
+    auto First = reinterpret_cast<const uint16_t *>(AddrOffsets.data());
+    auto Last = First + GSYMHeader->NumAddresses;
----------------
Looks like this code is always the same except the cast, I'd factor this out.


================
Comment at: lib/DebugInfo/GSYM/LineTable.cpp:24
+struct DeltaInfo {
+  int64_t delta;
+  uint32_t count;
----------------
UperCamelCase.


================
Comment at: lib/DebugInfo/GSYM/LineTable.cpp:33
+
+static bool encode_special(int64_t min_line_delta, int64_t max_line_delta,
+                           int64_t line_delta, uint64_t addr_delta,
----------------
MinLineDelta, etc


================
Comment at: lib/DebugInfo/GSYM/LineTable.cpp:54
+    std::function<bool(const LineEntry &row)> const &row_callback) {
+  uint32_t offset = 0;
+  int64_t min_delta = Data.getSLEB128(&offset);
----------------
s/offset/Offset, same for the other variable names in this file.


https://reviews.llvm.org/D53379