Always normalize FileSpec paths.

[clayborg]

Summary

Always normalizing lldb_private::FileSpec paths will help us get a consistent results from comparisons when setting breakpoints and when looking for source files. This also removes a lot of complexity from the comparison routines. Modified the DWARF line table parser to use the normalized compile unit directory if needed.

[clayborg]

Remove unwanted file changes.

[aprantl]

lldb.xcodeproj/xcshareddata/xcschemes/darwin-debug.xcscheme

29 ↗

(On Diff #143605)

Is this an accidental change or is it relevant to the patch?

lldb.xcodeproj/xcshareddata/xcschemes/desktop.xcscheme

68 ↗

(On Diff #143605)

This looks like an accidental commit?

[clayborg]

Remove commented out code.

[aprantl]

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

189 ↗

(On Diff #143608)

// slashes and other path ano*m*alies before we use it for path prepending*.*

source/Utility/FileSpec.cpp

105 ↗

(On Diff #143608)

Are we sure that we need this? Could we use llvm::sys::path::has_relative_path & friends instead? http://llvm.org/doxygen/namespacellvm_1_1sys_1_1path.html

107 ↗

(On Diff #143608)

shouldn't this be a more general path separator (we probably have something like this in Support/Path.h)

[clayborg]

source/Utility/FileSpec.cpp

105 ↗

(On Diff #143609)

I only want to scan the string one time. This will return true if this contains relative redundancies or extra slashes, not if the path is relative. "./foo" doesn't need normalization and this will return false, but "foo/." would need to be normalized to "foo", as well as "foo/./bar" and "foo/../bar". So this function is doing something completely different than the LLVM counterparts

107 ↗

(On Diff #143609)

no as the only caller to this function switches the slashes to '/' already..

[clayborg]

Fix comment typo

[aprantl]

source/Utility/FileSpec.cpp

107 ↗

(On Diff #143609)

If we change that to also replace double-/ with singles, we could replace this function with a call to llvm::sys::path::remove_dots()

http://llvm.org/doxygen/namespacellvm_1_1sys_1_1path.html#a35c103b5fb70a66a1cb5da3b56f588a1

115 ↗

(On Diff #143643)

Does this also turn "//WORKGROUP/Foo" into "/WORKGROUP/Foo/"?

[aprantl]

One general question: why is this form of normalization preferred over calling realpath?

[clayborg]

source/Utility/FileSpec.cpp

107 ↗

(On Diff #143643)

I am fine switching to using the llvm functions for removing, this is only detecting if any normalization needs to happen. If there is an equivalent LLVM function that will only run through the string one time to detect all needs for normalization (no multiple passes looking for "..", then for "." etc like we used to have), I would be happy to use it, but there doesn't seem to be. We are trying to avoid having to create a "SmallVectorImpl< char >" for all paths if they don't need fixing here. This function is just iterating through an llvm::StringRef just to see if normalization needs to happen. If it does, then we make a SmallVectorImpl< char > and we fix the path. We can easily use llvm functions for the fixing part.

115 ↗

(On Diff #143643)

No it does not,. Notice the "if (i > 0)" below. We need to keep leading "//"

[clayborg]

In D45977#1076048, @aprantl wrote:

One general question: why is this form of normalization preferred over calling realpath?

Normalization is everything we can do to fix up a path without knowing anything about the current working directory or any symlinks. So we can remove redundant references to the current directory (".") or the parent directory (".."), but only if they are not at the start of the path. Since our path may have been created on a different machine with an unknown symlinks, realpath really can't do anything for us. What is a path like "./foo.txt" relative to when stored in the debug info with no compilation directory? what if we have "/tmp/mysymlink" mean on another machine? We can't realpath it because we don't know the actual root or any of the symlinks.

[aprantl]

source/Utility/FileSpec.cpp

107 ↗

(On Diff #143643)

I see, you want to avoid copying the string when it isn't necessary to do so. IMHO the best way to do this is to add a bool hasDots(const Twine &) function to llvm::sys::path and add a bool remove_dot_dot parameter to removeDots(). Since you have already written the unittests, adding the function to LLVM shouldn't be any extra work (I'll help reviewing), and we'll have 100 LoC less to maintain.

This way we can avoid having two functions that perform path normalization that superficially look like they might do the same thing, but a year from now nobody can really tell whether they behave exactly the same, and whether a bug found in one implementation should also be fixed in the other one, etc... .

[clayborg]

source/Utility/FileSpec.cpp

107 ↗

(On Diff #143643)

I want to know if there are redundant slashes as well. I want something like "bool llvm::sys::has(const Twine &, bool dots, bool dot_dots, bool redundant_slashes)". But I don't see that getting accepted? I just want a "bool llvm:sys::path::contains_redundant_stuff(const Twine &)". Not sure on the name though. needs_normalization? can_be_shortened? Everything in LLVM right now assumes that dots, dot dots, slashes and realpath stuff happen in the same function. Not sure how to break that up without ruining the performance of the one and only loop that should be happening. I like the current approach since it doesn't require chopping up the string into an array and iterating through the array.

[clayborg]

source/Utility/FileSpec.cpp

107 ↗

(On Diff #143643)

Also not sure if the LLVM stuff will try to substitute in the current working directory for "."?

[aprantl]

source/Utility/FileSpec.cpp

107 ↗

(On Diff #143643)

I just read the source code of remove_dots() (http://llvm.org/doxygen/Path_8cpp_source.html#l00699) and if I'm not mistaken, it actually also removes double-separators as a side-effect, since it iterates over the path components of the string as it constructs the copy. It also seems to avoid the copy operation if it isn't necessary.

Could you take another look? Perhaps I'm missing something, (or perhaps we can just turn this into a small addition to remove_dots).

Everything in LLVM right now assumes that dots, dot dots, slashes and realpath stuff happen in the same function.

I'm not sure I understand.

[aprantl]

Also not sure if the LLVM stuff will try to substitute in the current working directory for "."?

No it won't, remove_dots() has no side effects. There's a separate make_absolute() function. That one will cause another copy. Personally, if I have a choice between a redundant string copy and maintaining more code, I'd pick the string copy unless we know that it really matters.

[aprantl]

I'll take that back: make_absolute only copies when the path isn't already absolute.

[labath]

source/Breakpoint/BreakpointResolverFileLine.cpp

130–131 ↗

(On Diff #143643)

What about paths like .foo/bar.c and .../bar.c. These don't contain . or .. components. so you'll want to avoid the consuming here. (You can use *llvm::sys::path::begin(path) to get the first path component and check that).

Also, I'm not sure what is the behavior we want for paths like ../../foo.cpp. The present behavior of matching the path as it was /../foo.cpp does not seem entirely useful.

source/Utility/FileSpec.cpp

107 ↗

(On Diff #143643)

FWIW, this could be implemented in a much simpler way, while still making sure we run through the string only once. I'm thinking of something like:

bool first = true;
for(it = llvm::sys::path::begin(path, style), end = llvm::sys::path::end(path); it != end; ++it) {
  if (!first && (*it == "." || *it == "..")) return true;
  first = false;
}
return false;

184–187 ↗

(On Diff #143643)

This looks wrong, because then for a path like ./../whatever, you will end up with both . and .. in the "normalized" path.

401–406 ↗

(On Diff #143643)

This is an interesting edge case, but I'm not sure if we actually want to be doing this. I think we should make an effort to formally define what do we expect from the normalized path. After the lldb-dev discussion on ./foo.cpp I have formed a tentative definition in my head, and this does not seem consistent with that.

I'll try to propose one definition:

Def: Two FileSpecs are equivalent iff:

• they resolve to the same file in a virtual filesystem, where all referenced path components exist and are directories
• the current working directory is deep enough so that each .. component resolves to a different directory than its predecessor.
• both of them have at least two path components or both of them have just one path component.

Def: The normalized form of a FileSpec is the equivalent FileSpec with the least number path components. In case of ties, we choose the one with the least number of .. components.

Some explanations about how I arrived at this:
a) the "all paths exist and are directories" part is there so we can consider foo/../bar.cpp and ./bar.cpp equivalent. (Because, in general, this will not be true if foo does not exists, or is a regular file, or a symlink, etc.)
b) the "cwd is deep enough" part is there to avoid collapsing excessive leading ".." components, because / and /.. are the same file
c) the "at least two path components" is there to avoid collapsing ./foo.cpp into foo.cpp as we want to preserve the information that the file contained some kind of directory specification.
d) in the equivalence definition, the "least number of components" is there to make sure we remove all components that are removable and (hopefully) arrive at a single canonical representation.
e) the "tie" case is necessary to disambiguate between foo/.. and ./. as otherwise these two would be incomparable.

Now the (c) part would imply that foo/.. should normalize to ./. and not ., which is a bit odd, but it is consistent with our stated intention of preserving directory information. If we do not want to have ./. here, then we need to come up with a different definition of what it means to be "normalized".

857 ↗

(On Diff #143643)

Why is this necessary? result can't be empty because we have just appended something to it (and we've checked that components[i] is non-empty).

unittests/Utility/FileSpecTest.cpp

150–153 ↗

(On Diff #143643)

You've removed the remove_backup_dots argument from the FileSpec::Equal, which removes two of the combinations we need to test here. However, I think the other other two combination still deserve some testing. As it stands now, this completely removes the test coverage of the Equal function.

[clayborg]

I am trying to switch to using llvm::sys::path::remove_dots() and we will see where we end up. By switching to this it means:

"./foo.c" --> "foo.c"
"./foo/bar.c" --> "foo/bar.c"

This makes is easier to see if a relative path matches another FileSpec since we don't have to remove the "./" from the beginning of the path.

source/Breakpoint/BreakpointResolverFileLine.cpp

131 ↗

(On Diff #143643)

I will be removing this after I switch to using llvm::sys::path::remove_dots() instead of the Normalize() I converted.

source/Utility/FileSpec.cpp

107 ↗

(On Diff #143643)

This will all go away, I am just going to call llvm::sys::path::remove_dots()...

187 ↗

(On Diff #143643)

Yes we would need to remove this. Again, I will switch to using llvm::sys::path::remove_dots and we can deal with the fallout there,.

406 ↗

(On Diff #143643)

they resolve to the same file in a virtual filesystem, where all referenced path components exist and are directories

Normalizing happens after resolving and if we don't resolve a path, we have no way to know what is a directory and what isn't. We will be setting breakpoints for remote targets quite a bit in LLDB and ww can't assume or stat anything in a path. So I would say FileSpec's are equivalent if the relative paths match all components.

Now the (c) part would imply that foo/.. should normalize to ./. and not ., which is a bit odd, but it is consistent with our stated intention of preserving directory information. If we do not want to have ./. here, then we need to come up with a different definition of what it means to be "normalized".

I guess we could make the m_directory contain "." and m_filename contain nothing for the "./." case. It doesn 't make sense to have "." in both places.

857 ↗

(On Diff #143643)

Because if you don't check this joining {"/', "foo.txt"} will result in "//foo.txt" which is wrong,.

[labath]

source/Utility/FileSpec.cpp

406 ↗

(On Diff #143643)

they resolve to the same file in a virtual filesystem, where all referenced path components exist and are directories

Normalizing happens after resolving and if we don't resolve a path, we have no way to know what is a directory and what isn't. We will be setting breakpoints for remote targets quite a bit in LLDB and ww can't assume or stat anything in a path.

Yes, I am aware of that. I am not saying this is how we should actually implement the normalization algorithm. I am trying define what a "normalization" is in the first place, so that we can then judge whether a particular normalization algorithm is good or not. I think defining normalization in terms of an actual filesystem makes sense, since at the end of the day, our algorithm should somehow approximate what happens in real file systems. I am not saying the algorithm should be doing any stats, but for the verification (either in our heads or in the tests) we can use certainly use stats or actual file systems.

So I would say FileSpec's are equivalent if the relative paths match all components.

This is too vague to be useful. I have no idea how I would apply this definition to determine if e.g. "/foo/../bar.txt" and "./bar.txt" are equivalent. And you didn't say anything about how to derive the normal form for a FileSpec.

Now the (c) part would imply that foo/.. should normalize to ./. and not ., which is a bit odd, but it is consistent with our stated intention of preserving directory information. If we do not want to have ./. here, then we need to come up with a different definition of what it means to be "normalized".

I guess we could make the m_directory contain "." and m_filename contain nothing for the "./." case. It doesn 't make sense to have "." in both places.

I don't think that is very useful, as then this would be the only special case where normalization would produce a FileSpec without a filename component.

857 ↗

(On Diff #143643)

Yes, but didn't the original condition guard against that already?

We know that components[i] is non-empty, and we have just appended it to result two lines above. So, unless I am missing something, result.back() should be the same as components[i].back() and this additional check does not buy us anything.

[clayborg]

source/Utility/FileSpec.cpp

406 ↗

(On Diff #143643)

So I would say FileSpec's are equivalent if the relative paths match all components.

This is too vague to be useful. I have no idea how I would apply this definition to determine if e.g. "/foo/../bar.txt" and "./bar.txt" are equivalent. And you didn't say anything about how to derive the normal form for a FileSpec.

"/foo/../bar.txt" would be normalized to "/bar.txt" and "./bar.txt" will, after switching to llvm's remove_dots, be normalized to "bar.txt". So those could be though of as equivalent since one is only a basename and would only need to match the filename. If you have "/foo/bar/baz.txt", it could be equivalent to "bar/baz.txt" by making sure the filename's match, and if either or both path is relative, then matching as many directories as are specified.

I guess we could make the m_directory contain "." and m_filename contain nothing for the "./." case. It doesn 't make sense to have "." in both places.

I don't think that is very useful, as then this would be the only special case where normalization would produce a FileSpec without a filename component.

Ok, then leave it as is with "." in filename, not directory. We don't need it in both places IMHO

857 ↗

(On Diff #143643)

I did not. During testing I found this case and "/" + "main.c" was producing "//main.c"

[labath]

source/Utility/FileSpec.cpp

406 ↗

(On Diff #143643)

Ok, if we start using llvm's remove_dots as our normalization algorithm, then both of these issues will become moot (and I believe I have a fairly good understanding of how remove_dots works).

857 ↗

(On Diff #143643)

I've just tried calling join_path_components(ePathSyntaxPosix, {"/", "main.c"}), and it produced /main.c even without your modifications.

[clayborg]

Switch over to using llvm::sys::path::remove_dots(), remove the ::Normalize() function and fix a few issue discovered during testing.

[aprantl]

source/Utility/FileSpec.cpp

241 ↗

(On Diff #143777)

// Normalize the path by removing './' and other redundant components.

242 ↗

(On Diff #143777)

Thanks!

[clayborg]

Added comment before llvm::sys::path_remove_dots(...)

[labath]

This code itself looks fine, I have just two minor comments.

However, I do have a question about performance. I remember us being very worried about performance in the past, so we ended up putting in this like r298876. This removes the normalization step during FileSpec comparison, but it introduces mandatory normalization upon every FileSpec creation, so it's not obvious to me what will this do to performance. Should we try to benchmark this somehow?

Jim, you seem to have encountered a case when the normalization was a bottleneck (r298876). Do you remember what situation was that in?

source/Utility/FileSpec.cpp

245 ↗

(On Diff #143801)

It looks like this is unused.

269–270 ↗

(On Diff #143801)

remove_dots should never produce a path like this, so we should be able to revert this now.

[clayborg]

In D45977#1077719, @labath wrote:

This code itself looks fine, I have just two minor comments.

However, I do have a question about performance. I remember us being very worried about performance in the past, so we ended up putting in this like r298876. This removes the normalization step during FileSpec comparison, but it introduces mandatory normalization upon every FileSpec creation, so it's not obvious to me what will this do to performance. Should we try to benchmark this somehow?

Ok, so I ran a benchmark and we are about 7% slower on a completely cold file cache, and 9% slower on a warm file cache. If I add the needsNormalization() function back in, we are 7% faster for cold, and 10% faster for warm. So I will add the needsNormalization() back in.

[clayborg]

After doing performance tests, the code was 7 to 10 % slower if we didn't check if a path needs normalization due to the llvm code making arrays of StringRef objects and appending a path together. Restored and even improved performance after adding back the needsNormalization() function that quickly checks if a path needs normalization and avoids the splitting of the path and reconstruction of the path if it isn't needed.

[labath]

Looks fine to me. Normalization, at least as it is implemented now in remove_dots, is a fairly heavy operation, so it makes sense to avoid it when possible. And the extra speedup is great.

source/Utility/FileSpec.cpp

203 ↗

(On Diff #144021)

these can be just static functions. No need for a namespace.

unittests/Utility/FileSpecTest.cpp

236–237 ↗

(On Diff #144021)

Neither the present behavior, nor c:\ is correct here. c:..\.. is a path relative to the current directory on the C drive, C:\ is the root of the C drive, and c:\..\.. is equivalent to c:\. However, it seems nobody cares about corner cases like this, as there are bunch of them in the llvm path handling.

lldb/trunk/include/lldb/Core/FileSpecList.h

public:
   ///     An index into the file list.
   ///
   /// @param[in] file
   ///     The file specification to search for.
   ///
   /// @param[in] full
   ///     Should FileSpec::Equal be called with "full" true or false.
   ///
-  /// @param[in] remove_backup_dots
-  ///     Should FileSpec::Equal be called with "remove_backup_dots" true or
-  ///     false.
-  ///
   /// @return
   ///     The index of the file that matches \a file if it is found,
   ///     else UINT32_MAX is returned.
   //------------------------------------------------------------------
-  size_t FindFileIndex(size_t idx, const FileSpec &file, bool full,
-                       bool remove_backup_dots = false) const;
+  size_t FindFileIndex(size_t idx, const FileSpec &file, bool full) const;
 
   //------------------------------------------------------------------
   /// Get file at index.
   ///
   /// Gets a file from the file list. If \a idx is not a valid index,
   /// an empty FileSpec object will be returned. The file objects
   /// that are returned can be tested using
   /// FileSpec::operator void*().

lldb/trunk/include/lldb/Utility/FileSpec.h

public:
   ///
   /// @return
   ///     @li -1 if \a lhs is less than \a rhs
   ///     @li 0 if \a lhs is equal to \a rhs
   ///     @li 1 if \a lhs is greater than \a rhs
   //------------------------------------------------------------------
   static int Compare(const FileSpec &lhs, const FileSpec &rhs, bool full);
 
-  static bool Equal(const FileSpec &a, const FileSpec &b, bool full,
-                    bool remove_backups = false);
+  static bool Equal(const FileSpec &a, const FileSpec &b, bool full);
 
   //------------------------------------------------------------------
   /// Case sensitivity of path.
   ///
   /// @return
   ///     \b true if the file path is case sensitive (POSIX), false
   ///   if case insensitive (Windows).
   //------------------------------------------------------------------
public:
   /// @return
   ///     The number of bytes that this object occupies in memory.
   ///
   /// @see ConstString::StaticMemorySize ()
   //------------------------------------------------------------------
   size_t MemorySize() const;
 
   //------------------------------------------------------------------
-  /// Normalize a pathname by collapsing redundant separators and
-  /// up-level references.
-  //------------------------------------------------------------------
-  FileSpec GetNormalizedPath() const;
-
-  //------------------------------------------------------------------
   /// Change the file specified with a new path.
   ///
   /// Update the contents of this object with a new path. The path will
   /// be split up into a directory and filename and stored as uniqued
   /// string values for quick comparison and efficient memory usage.
   ///
   /// @param[in] path
   ///     A full, partial, or relative path to a file.

lldb/trunk/source/Breakpoint/BreakpointResolverFileLine.cpp

 // inlined into.
 void BreakpointResolverFileLine::FilterContexts(SymbolContextList &sc_list,
                                                 bool is_relative) {
   if (m_exact_match)
     return; // Nothing to do. Contexts are precise.
 
   llvm::StringRef relative_path;
   if (is_relative)
-    relative_path = m_file_spec.GetNormalizedPath().GetDirectory().GetStringRef();
+    relative_path = m_file_spec.GetDirectory().GetStringRef();
 
   Log * log = GetLogIfAllCategoriesSet(LIBLLDB_LOG_BREAKPOINTS);
   for(uint32_t i = 0; i < sc_list.GetSize(); ++i) {
     SymbolContext sc;
     sc_list.GetContextAtIndex(i, sc);
     if (is_relative) {
       // If the path was relative, make sure any matches match as long as the
       // relative parts of the path match the path from support files

lldb/trunk/source/Core/FileSpecList.cpp

 //------------------------------------------------------------------
 // Find the index of the file in the file spec list that matches
 // "file_spec" starting "start_idx" entries into the file spec list.
 //
 // Returns the valid index of the file that matches "file_spec" if
 // it is found, else std::numeric_limits<uint32_t>::max() is returned.
 //------------------------------------------------------------------
 size_t FileSpecList::FindFileIndex(size_t start_idx, const FileSpec &file_spec,
-                                   bool full, bool remove_dots) const {
+                                   bool full) const {
   const size_t num_files = m_files.size();
 
   // When looking for files, we will compare only the filename if the
   // FILE_SPEC argument is empty
   bool compare_filename_only = file_spec.GetDirectory().IsEmpty();
 
   for (size_t idx = start_idx; idx < num_files; ++idx) {
     if (compare_filename_only) {
       if (ConstString::Equals(
               m_files[idx].GetFilename(), file_spec.GetFilename(),
               file_spec.IsCaseSensitive() || m_files[idx].IsCaseSensitive()))
         return idx;
     } else {
-      if (FileSpec::Equal(m_files[idx], file_spec, full, remove_dots))
+      if (FileSpec::Equal(m_files[idx], file_spec, full))
         return idx;
     }
   }
 
   // We didn't find the file, return an invalid index
   return UINT32_MAX;
 }
 

lldb/trunk/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp

if (!rpath_paths.empty()) {
 
       for (const auto &rpath_relative_path : rpath_relative_paths) {
         for (const auto &rpath : rpath_paths) {
           std::string path = rpath;
           path += rpath_relative_path;
           // It is OK to resolve this path because we must find a file on
           // disk for us to accept it anyway if it is rpath relative.
           FileSpec file_spec(path, true);
-          // Remove any redundant parts of the path (like "../foo") since
-          // LC_RPATH values often contain "..".
-          file_spec = file_spec.GetNormalizedPath();
           if (file_spec.Exists() && files.AppendIfUnique(file_spec)) {
             count++;
             break;
           }
         }
       }
     }
 
     // We may have @executable_paths but no RPATHS.  Figure those out here.
     // Only do this if this object file is the executable.  We have no way to
     // get back to the actual executable otherwise, so we won't get the right
     // path.
     if (!at_exec_relative_paths.empty() && CalculateType() == eTypeExecutable) {
       FileSpec exec_dir = this_file_spec.CopyByRemovingLastPathComponent();
       for (const auto &at_exec_relative_path : at_exec_relative_paths) {
         FileSpec file_spec = 
             exec_dir.CopyByAppendingPathComponent(at_exec_relative_path);
-        file_spec = file_spec.GetNormalizedPath();
         if (file_spec.Exists() && files.AppendIfUnique(file_spec))
           count++;
       }
     }
   }
   return count;
 }
 

lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFDebugLine.h

struct Prologue {
     void Clear() {
       total_length = version = prologue_length = min_inst_length = line_base =
           line_range = opcode_base = 0;
       line_base = 0;
       standard_opcode_lengths.clear();
       include_directories.clear();
       file_names.clear();
     }
-    bool GetFile(uint32_t file_idx, const char *comp_dir,
+    bool GetFile(uint32_t file_idx, const lldb_private::FileSpec &cu_comp_dir,
                  lldb_private::FileSpec &file) const;
   };
 
   // Standard .debug_line state machine structure
   struct Row {
     typedef std::vector<Row> collection;
     typedef collection::iterator iterator;
     typedef collection::const_iterator const_iterator;
static bool DumpOpcodes(
       uint32_t dump_flags = 0); // If line_offset is invalid, dump everything
   static bool DumpLineTableRows(
       lldb_private::Log *log, SymbolFileDWARF *dwarf2Data,
       dw_offset_t line_offset =
           DW_INVALID_OFFSET); // If line_offset is invalid, dump everything
   static bool
   ParseSupportFiles(const lldb::ModuleSP &module_sp,
                     const lldb_private::DWARFDataExtractor &debug_line_data,
-                    const char *cu_comp_dir, dw_offset_t stmt_list,
+                    const lldb_private::FileSpec &cu_comp_dir,
+                    dw_offset_t stmt_list,
                     lldb_private::FileSpecList &support_files);
   static bool
   ParsePrologue(const lldb_private::DWARFDataExtractor &debug_line_data,
                 lldb::offset_t *offset_ptr, Prologue *prologue);
   static bool
   ParseStatementTable(const lldb_private::DWARFDataExtractor &debug_line_data,
                       lldb::offset_t *offset_ptr, State::Callback callback,
                       void *userData);

lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFDebugLine.cpp

Host::SystemLog(Host::eSystemLogWarning,
                     " but it ended at 0x%8.8" PRIx64 "\n",
                     prologue_offset, end_prologue_offset, *offset_ptr);
   }
   return end_prologue_offset;
 }
 
 bool DWARFDebugLine::ParseSupportFiles(
     const lldb::ModuleSP &module_sp, const DWARFDataExtractor &debug_line_data,
-    const char *cu_comp_dir, dw_offset_t stmt_list,
+    const lldb_private::FileSpec &cu_comp_dir, dw_offset_t stmt_list,
     FileSpecList &support_files) {
   lldb::offset_t offset = stmt_list;
 
   Prologue prologue;
   if (!ParsePrologue(debug_line_data, &offset, &prologue)) {
     Host::SystemLog(Host::eSystemLogError, "error: parsing line table prologue "
                                            "at 0x%8.8x (parsing ended around "
                                            "0x%8.8" PRIx64 "\n",

 //      buff.AppendCStr(file_names[i].name.c_str());
 //      buff.Append32_as_ULEB128(file_names[i].dir_idx);
 //      buff.Append32_as_ULEB128(file_names[i].mod_time);
 //      buff.Append32_as_ULEB128(file_names[i].length);
 //  }
 //  buff.Append8(0);    // Terminate the file names section with empty string
 //}
 
-bool DWARFDebugLine::Prologue::GetFile(uint32_t file_idx, const char *comp_dir,
-                                       FileSpec &file) const {
+bool DWARFDebugLine::Prologue::GetFile(uint32_t file_idx,
+    const lldb_private::FileSpec &comp_dir, FileSpec &file) const {
   uint32_t idx = file_idx - 1; // File indexes are 1 based...
   if (idx < file_names.size()) {
     file.SetFile(file_names[idx].name, false);
     if (file.IsRelative()) {
       if (file_names[idx].dir_idx > 0) {
         const uint32_t dir_idx = file_names[idx].dir_idx - 1;
         if (dir_idx < include_directories.size()) {
           file.PrependPathComponent(include_directories[dir_idx]);
           if (!file.IsRelative())
             return true;
         }
       }
 
-      if (comp_dir && comp_dir[0])
+      if (comp_dir)
         file.PrependPathComponent(comp_dir);
     }
     return true;
   }
   return false;
 }
 
 //----------------------------------------------------------------------

lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

static const char *removeHostnameFromPathname(const char *path_from_dwarf) {
   if (colon_pos == path_from_dwarf + 1 && isalpha(*path_from_dwarf) &&
       strlen(path_from_dwarf) > 2 && '\\' == path_from_dwarf[2]) {
     return path_from_dwarf;
   }
 
   return colon_pos + 1;
 }
 
-static const char *resolveCompDir(const char *path_from_dwarf) {
+static FileSpec resolveCompDir(const char *path_from_dwarf) {
   if (!path_from_dwarf)
-    return nullptr;
+    return FileSpec();
 
   // DWARF2/3 suggests the form hostname:pathname for compilation directory.
   // Remove the host part if present.
   const char *local_path = removeHostnameFromPathname(path_from_dwarf);
   if (!local_path)
-    return nullptr;
+    return FileSpec();
 
   bool is_symlink = false;
-  FileSpec local_path_spec(local_path, false);
+  // Always normalize our compile unit directory to get rid of redundant
+  // slashes and other path anomalies before we use it for path prepending
+  FileSpec local_spec(local_path, false);
   const auto &file_specs = GetGlobalPluginProperties()->GetSymLinkPaths();
   for (size_t i = 0; i < file_specs.GetSize() && !is_symlink; ++i)
     is_symlink = FileSpec::Equal(file_specs.GetFileSpecAtIndex(i),
-                                 local_path_spec, true);
+                                 local_spec, true);
 
   if (!is_symlink)
-    return local_path;
+    return local_spec;
 
   namespace fs = llvm::sys::fs;
-  if (fs::get_file_type(local_path_spec.GetPath(), false) !=
+  if (fs::get_file_type(local_spec.GetPath(), false) !=
       fs::file_type::symlink_file)
-    return local_path;
+    return local_spec;
 
-  FileSpec resolved_local_path_spec;
-  const auto error =
-      FileSystem::Readlink(local_path_spec, resolved_local_path_spec);
+  FileSpec resolved_symlink;
+  const auto error = FileSystem::Readlink(local_spec, resolved_symlink);
   if (error.Success())
-    return resolved_local_path_spec.GetCString();
+    return resolved_symlink;
 
-  return nullptr;
+  return local_spec;
 }
 
 DWARFUnit *SymbolFileDWARF::GetBaseCompileUnit() {
   return nullptr;
 }
 
 void SymbolFileDWARF::Initialize() {
   LogChannelDWARF::Initialize();

 bool SymbolFileDWARF::ParseCompileUnitSupportFiles(
     const SymbolContext &sc, FileSpecList &support_files) {
   assert(sc.comp_unit);
   DWARFUnit *dwarf_cu = GetDWARFCompileUnit(sc.comp_unit);
   if (dwarf_cu) {
     const DWARFDIE cu_die = dwarf_cu->GetUnitDIEOnly();
 
     if (cu_die) {
-      const char *cu_comp_dir = resolveCompDir(
+      FileSpec cu_comp_dir = resolveCompDir(
           cu_die.GetAttributeValueAsString(DW_AT_comp_dir, nullptr));
       const dw_offset_t stmt_list = cu_die.GetAttributeValueAsUnsigned(
           DW_AT_stmt_list, DW_INVALID_OFFSET);
       if (stmt_list != DW_INVALID_OFFSET) {
         // All file indexes in DWARF are one based and a file of index zero is
         // supposed to be the compile unit itself.
         support_files.Append(*sc.comp_unit);
         return DWARFDebugLine::ParseSupportFiles(

lldb/trunk/source/Symbol/CompileUnit.cpp

uint32_t CompileUnit::ResolveSymbolContext(const FileSpec &file_spec,
                                            uint32_t line, bool check_inlines,
                                            bool exact, uint32_t resolve_scope,
                                            SymbolContextList &sc_list) {
   // First find all of the file indexes that match our "file_spec". If
   // "file_spec" has an empty directory, then only compare the basenames
   // when finding file indexes
   std::vector<uint32_t> file_indexes;
   const bool full_match = (bool)file_spec.GetDirectory();
-  const bool remove_backup_dots = true;
   bool file_spec_matches_cu_file_spec =
-      FileSpec::Equal(file_spec, *this, full_match, remove_backup_dots);
+      FileSpec::Equal(file_spec, *this, full_match);
 
   // If we are not looking for inlined functions and our file spec doesn't
   // match then we are done...
   if (file_spec_matches_cu_file_spec == false && check_inlines == false)
     return 0;
 
   uint32_t file_idx =
-      GetSupportFiles().FindFileIndex(1, file_spec, true, remove_backup_dots);
+      GetSupportFiles().FindFileIndex(1, file_spec, true);
   while (file_idx != UINT32_MAX) {
     file_indexes.push_back(file_idx);
-    file_idx = GetSupportFiles().FindFileIndex(file_idx + 1, file_spec, true,
-                                               remove_backup_dots);
+    file_idx = GetSupportFiles().FindFileIndex(file_idx + 1, file_spec, true);
   }
 
   const size_t num_file_indexes = file_indexes.size();
   if (num_file_indexes == 0)
     return 0;
 
   const uint32_t prev_size = sc_list.GetSize();
 

lldb/trunk/source/Symbol/Declaration.cpp

 
 bool lldb_private::operator==(const Declaration &lhs, const Declaration &rhs) {
 #ifdef LLDB_ENABLE_DECLARATION_COLUMNS
   if (lhs.GetColumn() == rhs.GetColumn())
     if (lhs.GetLine() == rhs.GetLine())
       return lhs.GetFile() == rhs.GetFile();
 #else
   if (lhs.GetLine() == rhs.GetLine())
-    return FileSpec::Equal(lhs.GetFile(), rhs.GetFile(), true, true);
+    return FileSpec::Equal(lhs.GetFile(), rhs.GetFile(), true);
 #endif
   return false;
 }

lldb/trunk/source/Utility/FileSpec.cpp

 char GetPreferredPathSeparator(FileSpec::PathSyntax syntax) {
   return GetPathSeparators(syntax)[0];
 }
 
 bool IsPathSeparator(char value, FileSpec::PathSyntax syntax) {
   return value == '/' || (!PathSyntaxIsPosix(syntax) && value == '\\');
 }
 
-void Normalize(llvm::SmallVectorImpl<char> &path, FileSpec::PathSyntax syntax) {
-  if (PathSyntaxIsPosix(syntax))
-    return;
-
-  std::replace(path.begin(), path.end(), '\\', '/');
-  // Windows path can have \\ slashes which can be changed by replace
-  // call above to //. Here we remove the duplicate.
-  auto iter = std::unique(path.begin(), path.end(), [](char &c1, char &c2) {
-    return (c1 == '/' && c2 == '/');
-  });
-  path.erase(iter, path.end());
+inline llvm::sys::path::Style
+LLVMPathSyntax(FileSpec::PathSyntax lldb_syntax) {
+  switch (lldb_syntax) {
+    case FileSpec::ePathSyntaxPosix:
+      return llvm::sys::path::Style::posix;
+    case FileSpec::ePathSyntaxWindows:
+      return llvm::sys::path::Style::windows;
+    default:
+    case FileSpec::ePathSyntaxHostNative:
+      return llvm::sys::path::Style::native;
+  };
+  return llvm::sys::path::Style::native;
 }
 
 void Denormalize(llvm::SmallVectorImpl<char> &path,
                  FileSpec::PathSyntax syntax) {
   if (PathSyntaxIsPosix(syntax))
     return;
 
   std::replace(path.begin(), path.end(), '/', '\\');
if (rhs)
     *this = *rhs;
 }
 
 //------------------------------------------------------------------
 // Virtual destructor in case anyone inherits from this class.
 //------------------------------------------------------------------
 FileSpec::~FileSpec() {}
 
+namespace {
+//------------------------------------------------------------------
+/// Safely get a character at the specified index.
+///
+/// @param[in] path
+///     A full, partial, or relative path to a file.
+///
+/// @param[in] i
+///     An index into path which may or may not be valid.
+///
+/// @return
+///   The character at index \a i if the index is valid, or 0 if
+///   the index is not valid.
+//------------------------------------------------------------------
+inline char safeCharAtIndex(const llvm::StringRef &path, size_t i) {
+  if (i < path.size())
+    return path[i];
+  return 0;
+}
+
+//------------------------------------------------------------------
+/// Check if a path needs to be normalized.
+///
+/// Check if a path needs to be normalized. We currently consider a
+/// path to need normalization if any of the following are true
+///  - path contains "/./"
+///  - path contains "/../"
+///  - path contains "//"
+///  - path ends with "/"
+/// Paths that start with "./" or with "../" are not considered to
+/// need normalization since we aren't trying to resolve the path,
+/// we are just trying to remove redundant things from the path.
+///
+/// @param[in] path
+///     A full, partial, or relative path to a file.
+///
+/// @param[in] syntax
+///     The syntax enumeration for the path in \a path.
+///
+/// @return
+///   Returns \b true if the path needs to be normalized.
+//------------------------------------------------------------------
+bool needsNormalization(const llvm::StringRef &path,
+                        FileSpec::PathSyntax syntax) {
+  const auto separator = GetPreferredPathSeparator(syntax);
+  for (auto i = path.find(separator); i != llvm::StringRef::npos;
+       i = path.find(separator, i + 1)) {
+    const auto next = safeCharAtIndex(path, i+1);
+    switch (next) {
+      case 0:
+        // path separator char at the end of the string which should be
+        // stripped unless it is the one and only character
+        return i > 0;
+      case '/':
+      case '\\':
+        // two path separator chars in the middle of a path needs to be
+        // normalized
+        if (next == separator && i > 0)
+          return true;
+        ++i;
+        break;
+
+      case '.': {
+          const auto next_next = safeCharAtIndex(path, i+2);
+          switch (next_next) {
+            default: break;
+            case 0: return true; // ends with "/."
+            case '/':
+            case '\\':
+              if (next_next == separator)
+                return true; // contains "/./"
+              break;
+            case '.': {
+              const auto next_next_next = safeCharAtIndex(path, i+3);
+              switch (next_next_next) {
+                default: break;
+                case 0: return true; // ends with "/.."
+                case '/':
+                case '\\':
+                  if (next_next_next == separator)
+                    return true; // contains "/../"
+                  break;
+              }
+              break;
+            }
+          }
+        }
+        break;
+
+      default:
+        break;
+    }
+  }
+  return false;
+}
+
+  
+}
 //------------------------------------------------------------------
 // Assignment operator.
 //------------------------------------------------------------------
 const FileSpec &FileSpec::operator=(const FileSpec &rhs) {
   if (this != &rhs) {
     m_directory = rhs.m_directory;
     m_filename = rhs.m_filename;
     m_is_resolved = rhs.m_is_resolved;
void FileSpec::SetFile(llvm::StringRef pathname, bool resolve,
 
   llvm::SmallString<64> resolved(pathname);
 
   if (resolve) {
     FileSpec::Resolve(resolved);
     m_is_resolved = true;
   }
 
-  Normalize(resolved, m_syntax);
+  // Normalize the path by removing ".", ".." and other redundant components.
+  if (needsNormalization(llvm::StringRef(resolved.data(), resolved.size()),
+                         syntax))
+    llvm::sys::path::remove_dots(resolved, true, LLVMPathSyntax(syntax));
+
+  // Normalize back slashes to forward slashes
+  if (syntax == FileSpec::ePathSyntaxWindows)
+    std::replace(resolved.begin(), resolved.end(), '\\', '/');
 
   llvm::StringRef resolve_path_ref(resolved.c_str());
   size_t dir_end = ParentPathEnd(resolve_path_ref, m_syntax);
   if (dir_end == 0) {
     m_filename.SetString(resolve_path_ref);
     return;
   }
 
int FileSpec::Compare(const FileSpec &a, const FileSpec &b, bool full) {
   if (full || (a.m_directory && b.m_directory)) {
     result = ConstString::Compare(a.m_directory, b.m_directory, case_sensitive);
     if (result)
       return result;
   }
   return ConstString::Compare(a.m_filename, b.m_filename, case_sensitive);
 }
 
-bool FileSpec::Equal(const FileSpec &a, const FileSpec &b, bool full,
-                     bool remove_backups) {
-  static ConstString g_dot_string(".");
-  static ConstString g_dot_dot_string("..");
+bool FileSpec::Equal(const FileSpec &a, const FileSpec &b, bool full) {
 
   // case sensitivity of equality test
   const bool case_sensitive = a.IsCaseSensitive() || b.IsCaseSensitive();
   
-  bool filenames_equal = ConstString::Equals(a.m_filename, 
-                                             b.m_filename, 
+  const bool filenames_equal = ConstString::Equals(a.m_filename,
+                                                   b.m_filename,
                                                    case_sensitive);
 
-  // The only way two FileSpecs can be equal if their filenames are
-  // unequal is if we are removing backups and one or the other filename
-  // is a backup string:
-
-  if (!filenames_equal && !remove_backups)
-      return false;
-
-  bool last_component_is_dot = ConstString::Equals(a.m_filename, g_dot_string) 
-                               || ConstString::Equals(a.m_filename, 
-                                                      g_dot_dot_string)
-                               || ConstString::Equals(b.m_filename, 
-                                                      g_dot_string)
-                               || ConstString::Equals(b.m_filename, 
-                                                      g_dot_dot_string);
-
-  if (!filenames_equal && !last_component_is_dot)
+  if (!filenames_equal)
       return false;
 
   if (!full && (a.GetDirectory().IsEmpty() || b.GetDirectory().IsEmpty()))
     return filenames_equal;
 
-  if (remove_backups == false)
-    return a == b;
-
-  if (a == b)
-    return true;
-
-  return Equal(a.GetNormalizedPath(), b.GetNormalizedPath(), full, false);
-}
-
-FileSpec FileSpec::GetNormalizedPath() const {
-  // Fast path. Do nothing if the path is not interesting.
-  if (!m_directory.GetStringRef().contains(".") &&
-      !m_directory.GetStringRef().contains("//") &&
-      m_filename.GetStringRef() != ".." && m_filename.GetStringRef() != ".")
-    return *this;
-
-  llvm::SmallString<64> path, result;
-  const bool normalize = false;
-  GetPath(path, normalize);
-  llvm::StringRef rest(path);
-
-  // We will not go below root dir.
-  size_t root_dir_start = RootDirStart(path, m_syntax);
-  const bool absolute = root_dir_start != llvm::StringRef::npos;
-  if (absolute) {
-    result += rest.take_front(root_dir_start + 1);
-    rest = rest.drop_front(root_dir_start + 1);
-  } else {
-    if (m_syntax == ePathSyntaxWindows && path.size() > 2 && path[1] == ':') {
-      result += rest.take_front(2);
-      rest = rest.drop_front(2);
-    }
-  }
-
-  bool anything_added = false;
-  llvm::SmallVector<llvm::StringRef, 0> components, processed;
-  rest.split(components, '/', -1, false);
-  processed.reserve(components.size());
-  for (auto component : components) {
-    if (component == ".")
-      continue; // Skip these.
-    if (component != "..") {
-      processed.push_back(component);
-      continue; // Regular file name.
-    }
-    if (!processed.empty()) {
-      processed.pop_back();
-      continue; // Dots. Go one level up if we can.
-    }
-    if (absolute)
-      continue; // We're at the top level. Cannot go higher than that. Skip.
-
-    result += component; // We're a relative path. We need to keep these.
-    result += '/';
-    anything_added = true;
-  }
-  for (auto component : processed) {
-    result += component;
-    result += '/';
-    anything_added = true;
-  }
-  if (anything_added)
-    result.pop_back(); // Pop last '/'.
-  else if (result.empty())
-    result = ".";
-
-  return FileSpec(result, false, m_syntax);
 }
 
 //------------------------------------------------------------------
 // Dump the object to the supplied stream. If the object contains
 // a valid directory name, it will be displayed followed by a
 // directory delimiter, and the filename.
 //------------------------------------------------------------------
 void FileSpec::Dump(Stream *s) const {

 const char *FileSpec::GetCString(bool denormalize) const {
   return ConstString{GetPath(denormalize)}.AsCString(NULL);
 }
 
 void FileSpec::GetPath(llvm::SmallVectorImpl<char> &path,
                        bool denormalize) const {
   path.append(m_directory.GetStringRef().begin(),
               m_directory.GetStringRef().end());
-  if (m_directory && m_filename &&
-      !IsPathSeparator(m_directory.GetStringRef().back(), m_syntax))
-    path.insert(path.end(), GetPreferredPathSeparator(m_syntax));
+  // Since the path was normalized and all paths use '/' when stored in these
+  // objects, we don't need to look for the actual syntax specific path
+  // separator, we just look for and insert '/'.
+  if (m_directory && m_filename && m_directory.GetStringRef().back() != '/' &&
+      m_filename.GetStringRef().back() != '/')
+    path.insert(path.end(), '/');
   path.append(m_filename.GetStringRef().begin(),
               m_filename.GetStringRef().end());
-  Normalize(path, m_syntax);
   if (denormalize && !path.empty())
     Denormalize(path, m_syntax);
 }
 
 ConstString FileSpec::GetFileNameExtension() const {
   if (m_filename) {
     const char *filename = m_filename.GetCString();
     const char *dot_pos = strrchr(filename, '.');

lldb/trunk/unittests/Utility/FileSpecTest.cpp

TEST(FileSpecTest, FileAndDirectoryComponents) {
   FileSpec fs_windows_long("F:\\bar\\baz", false, FileSpec::ePathSyntaxWindows);
   EXPECT_STREQ("F:\\bar\\baz", fs_windows_long.GetCString());
   // EXPECT_STREQ("F:\\bar", fs_windows_long.GetDirectory().GetCString()); // It
   // returns "F:/bar"
   EXPECT_STREQ("baz", fs_windows_long.GetFilename().GetCString());
 
   FileSpec fs_posix_trailing_slash("/foo/bar/", false,
                                    FileSpec::ePathSyntaxPosix);
-  EXPECT_STREQ("/foo/bar/.", fs_posix_trailing_slash.GetCString());
-  EXPECT_STREQ("/foo/bar", fs_posix_trailing_slash.GetDirectory().GetCString());
-  EXPECT_STREQ(".", fs_posix_trailing_slash.GetFilename().GetCString());
+  EXPECT_STREQ("/foo/bar", fs_posix_trailing_slash.GetCString());
+  EXPECT_STREQ("/foo", fs_posix_trailing_slash.GetDirectory().GetCString());
+  EXPECT_STREQ("bar", fs_posix_trailing_slash.GetFilename().GetCString());
 
   FileSpec fs_windows_trailing_slash("F:\\bar\\", false,
                                      FileSpec::ePathSyntaxWindows);
-  EXPECT_STREQ("F:\\bar\\.", fs_windows_trailing_slash.GetCString());
-  // EXPECT_STREQ("F:\\bar",
-  // fs_windows_trailing_slash.GetDirectory().GetCString()); // It returns
-  // "F:/bar"
-  EXPECT_STREQ(".", fs_windows_trailing_slash.GetFilename().GetCString());
+  EXPECT_STREQ("F:\\bar", fs_windows_trailing_slash.GetCString());
+  EXPECT_STREQ("bar", fs_windows_trailing_slash.GetFilename().GetCString());
 }
 
 TEST(FileSpecTest, AppendPathComponent) {
   FileSpec fs_posix("/foo", false, FileSpec::ePathSyntaxPosix);
   fs_posix.AppendPathComponent("bar");
   EXPECT_STREQ("/foo/bar", fs_posix.GetCString());
   EXPECT_STREQ("/foo", fs_posix.GetDirectory().GetCString());
   EXPECT_STREQ("bar", fs_posix.GetFilename().GetCString());
TEST(FileSpecTest, PrependPathComponent) {
   fs_posix_root.PrependPathComponent("/");
   EXPECT_STREQ("/bar", fs_posix_root.GetCString());
 
   FileSpec fs_windows_root("bar", false, FileSpec::ePathSyntaxWindows);
   fs_windows_root.PrependPathComponent("F:\\");
   EXPECT_STREQ("F:\\bar", fs_windows_root.GetCString());
 }
 
-static void Compare(const FileSpec &one, const FileSpec &two, bool full_match,
-                    bool remove_backup_dots, bool result) {
-  EXPECT_EQ(result, FileSpec::Equal(one, two, full_match, remove_backup_dots))
-      << "File one: " << one.GetCString() << "\nFile two: " << two.GetCString()
-      << "\nFull match: " << full_match
-      << "\nRemove backup dots: " << remove_backup_dots;
-}
-
 TEST(FileSpecTest, EqualSeparator) {
   FileSpec backward("C:\\foo\\bar", false, FileSpec::ePathSyntaxWindows);
   FileSpec forward("C:/foo/bar", false, FileSpec::ePathSyntaxWindows);
   EXPECT_EQ(forward, backward);
-
-  const bool full_match = true;
-  const bool remove_backup_dots = true;
-  const bool match = true;
-  Compare(forward, backward, full_match, remove_backup_dots, match);
-  Compare(forward, backward, full_match, !remove_backup_dots, match);
-  Compare(forward, backward, !full_match, remove_backup_dots, match);
-  Compare(forward, backward, !full_match, !remove_backup_dots, match);
 }
 
 TEST(FileSpecTest, EqualDotsWindows) {
-  const bool full_match = true;
-  const bool remove_backup_dots = true;
-  const bool match = true;
   std::pair<const char *, const char *> tests[] = {
       {R"(C:\foo\bar\baz)", R"(C:\foo\foo\..\bar\baz)"},
       {R"(C:\bar\baz)", R"(C:\foo\..\bar\baz)"},
       {R"(C:\bar\baz)", R"(C:/foo/../bar/baz)"},
       {R"(C:/bar/baz)", R"(C:\foo\..\bar\baz)"},
       {R"(C:\bar)", R"(C:\foo\..\bar)"},
       {R"(C:\foo\bar)", R"(C:\foo\.\bar)"},
       {R"(C:\foo\bar)", R"(C:\foo\bar\.)"},
   };
 
   for (const auto &test : tests) {
     FileSpec one(test.first, false, FileSpec::ePathSyntaxWindows);
     FileSpec two(test.second, false, FileSpec::ePathSyntaxWindows);
-    EXPECT_NE(one, two);
-    Compare(one, two, full_match, remove_backup_dots, match);
-    Compare(one, two, full_match, !remove_backup_dots, !match);
-    Compare(one, two, !full_match, remove_backup_dots, match);
-    Compare(one, two, !full_match, !remove_backup_dots, !match);
+    EXPECT_EQ(one, two);
   }
 }
 
 TEST(FileSpecTest, EqualDotsPosix) {
-  const bool full_match = true;
-  const bool remove_backup_dots = true;
-  const bool match = true;
   std::pair<const char *, const char *> tests[] = {
       {R"(/foo/bar/baz)", R"(/foo/foo/../bar/baz)"},
       {R"(/bar/baz)", R"(/foo/../bar/baz)"},
       {R"(/bar)", R"(/foo/../bar)"},
       {R"(/foo/bar)", R"(/foo/./bar)"},
       {R"(/foo/bar)", R"(/foo/bar/.)"},
   };
 
   for (const auto &test : tests) {
     FileSpec one(test.first, false, FileSpec::ePathSyntaxPosix);
     FileSpec two(test.second, false, FileSpec::ePathSyntaxPosix);
-    EXPECT_NE(one, two);
-    Compare(one, two, full_match, remove_backup_dots, match);
-    Compare(one, two, full_match, !remove_backup_dots, !match);
-    Compare(one, two, !full_match, remove_backup_dots, match);
-    Compare(one, two, !full_match, !remove_backup_dots, !match);
+    EXPECT_EQ(one, two);
   }
 }
 
 TEST(FileSpecTest, EqualDotsPosixRoot) {
-  const bool full_match = true;
-  const bool remove_backup_dots = true;
-  const bool match = true;
   std::pair<const char *, const char *> tests[] = {
       {R"(/)", R"(/..)"},
       {R"(/)", R"(/.)"},
       {R"(/)", R"(/foo/..)"},
   };
 
   for (const auto &test : tests) {
     FileSpec one(test.first, false, FileSpec::ePathSyntaxPosix);
     FileSpec two(test.second, false, FileSpec::ePathSyntaxPosix);
-    EXPECT_NE(one, two);
-    Compare(one, two, full_match, remove_backup_dots, match);
-    Compare(one, two, full_match, !remove_backup_dots, !match);
-    Compare(one, two, !full_match, remove_backup_dots, !match);
-    Compare(one, two, !full_match, !remove_backup_dots, !match);
+    EXPECT_EQ(one, two);
   }
 }
 
 TEST(FileSpecTest, GetNormalizedPath) {
   std::pair<const char *, const char *> posix_tests[] = {
       {"/foo/.././bar", "/bar"},
       {"/foo/./../bar", "/bar"},
       {"/foo/../bar", "/bar"},
       {"/foo/./bar", "/foo/bar"},
       {"/foo/..", "/"},
       {"/foo/.", "/foo"},
       {"/foo//bar", "/foo/bar"},
       {"/foo//bar/baz", "/foo/bar/baz"},
       {"/foo//bar/./baz", "/foo/bar/baz"},
       {"/./foo", "/foo"},
       {"/", "/"},
-      {"//", "//"},
+      // TODO: fix llvm::sys::path::remove_dots() to return "//" below.
+      //{"//", "//"},
       {"//net", "//net"},
       {"/..", "/"},
       {"/.", "/"},
       {"..", ".."},
-      {".", "."},
+      {".", ""},
       {"../..", "../.."},
-      {"foo/..", "."},
+      {"foo/..", ""},
       {"foo/../bar", "bar"},
       {"../foo/..", ".."},
       {"./foo", "foo"},
+      {"././foo", "foo"},
+      {"../foo", "../foo"},
+      {"../../foo", "../../foo"},
   };
   for (auto test : posix_tests) {
     EXPECT_EQ(test.second,
               FileSpec(test.first, false, FileSpec::ePathSyntaxPosix)
-                  .GetNormalizedPath()
                   .GetPath());
   }
 
   std::pair<const char *, const char *> windows_tests[] = {
       {R"(c:\bar\..\bar)", R"(c:\bar)"},
       {R"(c:\bar\.\bar)", R"(c:\bar\bar)"},
       {R"(c:\bar\..)", R"(c:\)"},
       {R"(c:\bar\.)", R"(c:\bar)"},
       {R"(c:\.\bar)", R"(c:\bar)"},
       {R"(\)", R"(\)"},
       //      {R"(\\)", R"(\\)"},
       //      {R"(\\net)", R"(\\net)"},
       {R"(c:\..)", R"(c:\)"},
       {R"(c:\.)", R"(c:\)"},
-      {R"(\..)", R"(\)"},
+      // TODO: fix llvm::sys::path::remove_dots() to return "\" below.
+      {R"(\..)", R"(\..)"},
       //      {R"(c:..)", R"(c:..)"},
       {R"(..)", R"(..)"},
-      {R"(.)", R"(.)"},
-      {R"(c:..\..)", R"(c:..\..)"},
+      {R"(.)", R"()"},
+      // TODO: fix llvm::sys::path::remove_dots() to return "c:\" below.
+      {R"(c:..\..)", R"(c:\..\..)"},
       {R"(..\..)", R"(..\..)"},
-      {R"(foo\..)", R"(.)"},
+      {R"(foo\..)", R"()"},
       {R"(foo\..\bar)", R"(bar)"},
       {R"(..\foo\..)", R"(..)"},
       {R"(.\foo)", R"(foo)"},
+      {R"(.\.\foo)", R"(foo)"},
+      {R"(..\foo)", R"(..\foo)"},
+      {R"(..\..\foo)", R"(..\..\foo)"},
   };
   for (auto test : windows_tests) {
     EXPECT_EQ(test.second,
               FileSpec(test.first, false, FileSpec::ePathSyntaxWindows)
-                  .GetNormalizedPath()
                   .GetPath())
         << "Original path: " << test.first;
   }
 }
 
 TEST(FileSpecTest, FormatFileSpec) {
   auto win = FileSpec::ePathSyntaxWindows;
 
TEST(FileSpecTest, FormatFileSpec) {
   EXPECT_EQ("foo\\", llvm::formatv("{0:D}", F).str());
   EXPECT_EQ("bar.txt", llvm::formatv("{0:F}", F).str());
 
   F = FileSpec("foo", false, win);
   EXPECT_EQ("foo", llvm::formatv("{0}", F).str());
   EXPECT_EQ("foo", llvm::formatv("{0:F}", F).str());
   EXPECT_EQ("(empty)", llvm::formatv("{0:D}", F).str());
 }
+