[PATCH] D105985: Support GSYM in llvm-symbolizer.

Wed Jul 14 12:56:21 PDT 2021

clayborg added inline comments.

================
Comment at: llvm/lib/DebugInfo/GSYM/GsymDIContext.cpp:25
+                                     DILineInfo &LineInfo) {
+  LineInfo.FunctionName = static_cast<std::string>(Location.Name);
+
----------------
If Specifier.FNKind is set to DINameKind::ShortName, we want to demangle the name into LineInfo.FunctionName. If it is set to DINameKind::LinkageName or DINameKind::None, set to to the current name. Demangling can be tricky, as there are many name manglings. Not sure if there is a one stop shop to demangle all sorts of names in LLVM.

================
Comment at: llvm/lib/DebugInfo/GSYM/GsymDIContext.cpp:29-31
+    SmallString<128> P(Location.Dir);
+    sys::path::append(P, Location.Base);
+    LineInfo.FileName = static_cast<std::string>(P);
----------------
We should watch out for an empty Location.Dir and also for an empty Location.Base.

================
Comment at: llvm/lib/DebugInfo/GSYM/GsymDIContext.cpp:36
+  case DILineInfoSpecifier::FileLineInfoKind::BaseNameOnly:
+  case DILineInfoSpecifier::FileLineInfoKind::RawValue:
+    LineInfo.FileName = static_cast<std::string>(Location.Base);
----------------
Move DILineInfoSpecifier::FileLineInfoKind::RawValue up to the DILineInfoSpecifier::FileLineInfoKind::AbsoluteFilePath case. RawValue is described in comments as:
```
    // RawValue is whatever the compiler stored in the filename table.  Could be
    // a full path, could be something else.
```
So we should emit this as a full path of what was found in the GSYM

================
Comment at: llvm/lib/DebugInfo/GSYM/GsymDIContext.cpp:42-43
+  default:
+    // TODO We have no information to determine the relative path. Should we
+    // fail? Or fall back to something else?
+    return false;
----------------
 The header file describes DILineInfoSpecifier::FileLineInfoKind::RelativeFilePath using:
```
   // Relative to the compilation directory.
```
We don't have the compilation directory here, so I would just emit a full path, so move "case DILineInfoSpecifier::FileLineInfoKind::RelativeFilePath:" up to the case for "DILineInfoSpecifier::FileLineInfoKind::AbsoluteFilePath".

================
Comment at: llvm/lib/DebugInfo/GSYM/GsymDIContext.cpp:49
+  // TODO What's Source? The actual source code? We don't have that.
+  // LineInfo.Source = {};
+
----------------
We don't have the source. I would avoid attempting to read it from disk as well because you  might be symbolicating something from another machine or architecture.

================
Comment at: llvm/lib/DebugInfo/GSYM/GsymDIContext.cpp:52
+  // We don't have column info in GSYM.
+  // LineInfo.Column = 0;
+
----------------
Yes, set to zero 

================
Comment at: llvm/lib/DebugInfo/GSYM/GsymDIContext.cpp:54-56
+  // TODO I don't think we have this information available
+  // LineInfo.StartFileName = {};
+  // LineInfo.StartLine = 0;
----------------
yeah, we don't have this info

================
Comment at: llvm/lib/DebugInfo/GSYM/GsymDIContext.cpp:58
+
+  // TODO can we do something for the requested Specifier.FNKind?
+
----------------
We can. If it is set to DINameKind::ShortName, we want to demangle the name into LineInfo.FunctionName. If it is set to DINameKind::LinkageName or DINameKind::None, set to to the current name.

================
Comment at: llvm/lib/DebugInfo/GSYM/GsymDIContext.cpp:66
+                                     DILineInfoSpecifier Specifier) {
+  auto ResultOrErr = Reader->lookup(Address.Address);
+
----------------
Check for a Address for a valid section index:
```
if (Address.SectionIndex != llvm::object::SectionedAddress::UndefSection)
  return {};
```

================
Comment at: llvm/lib/DebugInfo/GSYM/GsymDIContext.cpp:68-69
+
+  if (!ResultOrErr)
+    return {};
+
----------------
You need to consume the error, or this will crash.

================
Comment at: llvm/lib/DebugInfo/GSYM/GsymDIContext.cpp:75-76
+
+  // TODO Or should we set LineInfo.FunctionName = Result.FuncName? Does that
+  // make a difference?
+  if (!Result.Locations.empty()) {
----------------
So the "Result.FuncName" is the name of the concrete function or symbol if there was no debug info for a symbol in the symbol table. So you only want to use this if "Result.Locations" is empty. If "Result.locations" is not empty, you don't want to use that because the "Result.Locations.front()" might point to a inlined function. The name in the SourceLocation will be the inlined function name. 

The main question is what other symbolizers do in this case. Lets say we are in the concrete function "main" and the "Result.Locations.front()" points to something line "std::vector<int>::empty()" at "/.../vector:123", do other symbolizers create a name the includes the inlined function + the concrete function? I would find an address in a DWARF file that points to an inlined function and symbolize it using the DWARF and see what gets output for the DWARF. 

The other issue here is that GSYM can return multiple locations for a single address since GSYM will unwind the inline call stack. The LookupResult contains an array of locations. Do we not want to convey this back? Or does the LLVM symbolizer always want the deepest inline function for a given address?

But seeing as below we get the inlining information in GsymDIContext::getInliningInfoForAddress(...), maybe we should always be returning the Result.FuncName? It really depends on what the other symbolizers do. I would make a small DWARF file, convert it to GSYM, then do lookups using the GSYM and the DWARF and making sure the DI classes match.

================
Comment at: llvm/lib/DebugInfo/GSYM/GsymDIContext.cpp:77-81
+  if (!Result.Locations.empty()) {
+    if (!fillLineInfoFromLocation(Result.Locations.front(), Specifier,
+                                  LineInfo))
+      return {};
+  }
----------------

================
Comment at: llvm/lib/DebugInfo/GSYM/GsymDIContext.cpp:97
+
+  // FIXME Implement this as well. It's not used by Symbolize though.
+
----------------
If this isn't used, is there a reason we are converting it?

================
Comment at: llvm/lib/DebugInfo/GSYM/GsymDIContext.cpp:99
+
+  // auto LineNumbers = Session->findLineNumbersByAddress(Address.Address,
+  // Size); if (!LineNumbers || LineNumbers->getChildCount() == 0)
----------------
If you wanted to do this you would get the full gsym::FunctionInfo from the GsymReader and convert any addresses that fall into this range.

```
if (Address.SectionIndex != llvm::object::SectionedAddress::UndefSection)
  return DILineInfoTable();

if (auto LineTableOrErr = Reader->getFunctionInfo(Address.Address)) {
  // Iterate over line table entries and take the ones that fall in the range.
} else {
  consumeError(LineTableOrErr.takeError());
  return DILineInfoTable();
}
```

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105985