[Lldb-commits] [PATCH] D56543: DWARF: Add some support for non-native directory separators

[Pavel Labath via Phabricator via lldb-commits]

labath marked 4 inline comments as done.
labath added inline comments.


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:775-787
+void DWARFUnit::ComputePathStyle() {
+  m_path_style = FileSpec::Style::native;
+  const DWARFDebugInfoEntry *die = GetUnitDIEPtrOnly();
+  if (!die)
+    return;
+  llvm::StringRef comp_dir =
+      die->GetAttributeValueAsString(m_dwarf, this, DW_AT_comp_dir, NULL);
----------------
clayborg wrote:
> I would suggest adding a "FileSpec DWARFUnit::GetFileSpec()" and moving code from SymbolFileDWARF::ParseCompileUnit into this function:
> ```
>             FileSpec cu_file_spec(cu_die.GetName());
>             if (cu_file_spec) {
>               // If we have a full path to the compile unit, we don't need to
>               // resolve the file.  This can be expensive e.g. when the source
>               // files are
>               // NFS mounted.
>               if (cu_file_spec.IsRelative()) {
>                 const char *cu_comp_dir{
>                     cu_die.GetAttributeValueAsString(DW_AT_comp_dir, nullptr)};
>                 cu_file_spec.PrependPathComponent(resolveCompDir(cu_comp_dir));
>               }
> ```
> 
> We might even cache the FileSpec object inside DWARFUnit so it doesn't have to be recomputed? 
> 
> Then use this resolved path. DW_AT_comp_dir isn't always there, sometimes the DW_AT_name is a full path only. So we should centralize this in DWARFUnit. We might want to make a function like:
> 
> ```
> void DWARFUnit::ResolveCompileUnitDirectoryRelativeFile(FileSpec &spec);
> ```
> 
> And then have "FileSpec DWARFUnit::GetFileSpec()" call it. I was looking at the other uses of DW_AT_comp_dir in the source, and the line tables tend to need to resolve relative paths. A few other locations. Might be nice to get the DWARFUnit and resolve the path using that? We can add "FileSpec DWARFDie::GetPath()" accessor, could even add a "void DWARFDie::ResolveRelativePath(FileSpec &)"
I think this version mostly implements what you had in mind. I added a `DWARFUnit::GetCompilationDirectory()` which returns the DW_AT_comp_dir attribute (I would expect that `DWARFUnit::GetFileSpec()` would return DW_AT_name, which is not useful for relative path resolution, except as a hint for the path style).

I've also added `FileSpec::MakeAbsolute` to shorten the `if(!spec.relative()) spec.prepend(dir))` pattern we had in some places. I didn't add any special FileSpec accessor to DWARFDie, as it didn't seem particularly useful (at this point). The only place where that's used now is for the computation of the name of the compile unit, and that's pretty concise now anyway.


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFUnit.h:255
 
+  void ComputePathStyle();
+
----------------
zturner wrote:
> aprantl wrote:
> > I would probably call this DetectPathStyle() since it's more a heuristic?
> Maybe even `Guess` since `Compute` implies absolute certainty.
With all the other changes, this is now called `ComputeCompDirAndGuessPathStyle`. Note that this is just a private helper function and the public accessor is still called `GetPathStyle()`


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

https://reviews.llvm.org/D56543