[lldb] Refactor SBFileSpec::GetDirectory

[bulbazord]

Summary

There's no reason to create an entire new filespec to mutate and grab
data from when we can just grab the data directly.

[jingham]

The old code had the side-effect of NOT resolving the path of the SBFileSpec in order to get its directory. I am not sure whether that was on purpose or not, however.

[bulbazord]

In D149625#4312838, @jingham wrote:

The old code had the side-effect of NOT resolving the path of the SBFileSpec in order to get its directory. I am not sure whether that was on purpose or not, however.

To be more precise, the old code had the side-effect of guaranteeing that whatever path was given to you was from an unresolved FileSpec. Right now, if the SBFileSpec is unresolved, this will do the exact same thing as before. With this change, if an SBFileSpec is resolved, the path you're getting is from a resolved FileSpec. I'm pretty sure that this shouldn't actually be different than what was there before since we're making a copy from the underlying FileSpec though, unless I'm missing something?

[omjavaid]

This introduced test failures on windows lldb-aarch64-windows buildbot.

lldb-api :: functionalities/process_save_core/TestProcessSaveCore.py
lldb-api :: python_api/symbol-context/TestSymbolContext.py

https://lab.llvm.org/buildbot/#/builders/219/builds/2485

I am reverting it, kindly revisit it.

[bulbazord]

AssertionError: 'C:\\Users\\tcwg\\llvm-worker\\lldb-aarch64-windows\\build\\lldb-test-build.noindex\\functionalities\\process_save_core\\TestProcessSaveCore.test_save_windows_mini_dump_dwarf\\a.out' not found in ['C:/Users/tcwg/llvm-worker/lldb-aarch64-windows/build/lldb-test-build.noindex/functionalities/process_save_core/TestProcessSaveCore.test_save_windows_mini_dump_dwarf\\a.out', 'C:/Windows/System32\\ntdll.dll', 'C:/Windows/System32\\kernel32.dll', 'C:/Windows/System32\\KernelBase.dll']

Looks like a path normalization issue... GetPathAsConstString() has a bool parameter that defaults to true for normalizing paths. The right thing for this patch to do is actually return m_opaque_up->GetPathAsConstString().GetCString() then instead of invoking GetDirectory() directly.

[bulbazord]

Ok, after looking at this more closely, it's a little clearer to me why SBFileSpec::GetDirectory was written this way to begin with. The method itself requires its return value to be denormalized (something not explicitly documented in the header nor the SWIG docstrings for this class/method). The only way to get the FileSpec's path in a denormalized form is by using FileSpec::GetPath and its variants... That's why we create a new FileSpec, remove its filename, and then get the path as a ConstString. I won't be addressing that in this patch because I view this as a flaw of the existing FileSpec implementation.

Thank you for finding and reverting this.

lldb/source/API/SBFileSpec.cpp

const char *SBFileSpec::GetFilename() const {
   LLDB_INSTRUMENT_VA(this);
 
   return m_opaque_up->GetFilename().AsCString();
 }
 
 const char *SBFileSpec::GetDirectory() const {
   LLDB_INSTRUMENT_VA(this);
 
-  return m_opaque_up->GetDirectory().GetCString();
+  FileSpec directory{*m_opaque_up};
+  directory.ClearFilename();
+  return directory.GetPathAsConstString().GetCString();
 }
 
 void SBFileSpec::SetFilename(const char *filename) {
   LLDB_INSTRUMENT_VA(this, filename);
 
   if (filename && filename[0])
     m_opaque_up->SetFilename(filename);
   else