[Path] Fix bug in make_absolute logic

[JDevlieghere]

Summary

While writing a VFS test case I discovered an issue in the make_absolute implementation for path with a //net style root. These are used in the VFS test cases because they are legal absolute paths on both Windows and unix.

NOTE: in the tests below, we use 'root/' as our root directory, since it is
// a legal *absolute* path on Windows as well as *nix.

The test worked fine for a regular Unix path, which led me to look at the implementation. The problem appears to be that we're trying to use the root name from the relative path, which I believe is incorrect.

[labath]

There was definitely a problem with the previous implementation, but I don't believe this is quite right either. After all the code you're editing is in the rootName && !rootDir branch, so p should contain a root name, and it makes sense to use that instead of the one in the current dir. I am not super-familiar with this code, but it seems to me that the problem is that you even reach this piece of code for a path like foo.cpp. This happens because we hard-code rootName to true for posix-style paths. This happens to be correct for the "Already absolute?" check, but I'm not sure it's correct for everything that comes after that..

So, I'd try to fix this by splitting the logic for "is this a posix path" from "does this path contain a root name", but it would be nice if someone could confirm that this is the right approach...

[JDevlieghere]

I think Pavel's observation is correct and I think this is the correct solution.

[labath]

It looks like the new version shouldn't change the behavior in any other case except the one you ran into (which is obviously wrong), so I'd say this is conservatively correct.

llvm/trunk/lib/Support/Path.cpp

getPotentiallyUniqueTempFileName(const Twine &Prefix, StringRef Suffix,
   return createTemporaryFile(Prefix, Suffix, Dummy, ResultPath, FS_Name);
 }
 
 void make_absolute(const Twine &current_directory,
                    SmallVectorImpl<char> &path) {
   StringRef p(path.data(), path.size());
 
   bool rootDirectory = path::has_root_directory(p);
-  bool rootName =
-      (real_style(Style::native) != Style::windows) || path::has_root_name(p);
+  bool rootName = path::has_root_name(p);
 
   // Already absolute.
-  if (rootName && rootDirectory)
+  if ((rootName || real_style(Style::native) != Style::windows) &&
+      rootDirectory)
     return;
 
   // All of the following conditions will need the current directory.
   SmallString<128> current_dir;
   current_directory.toVector(current_dir);
 
   // Relative path. Prepend the current directory.
   if (!rootName && !rootDirectory) {

llvm/trunk/unittests/Support/Path.cpp

for (SmallVector<StringRef, 40>::const_iterator i = paths.begin(),
     StringRef filename(temp_store.begin(), temp_store.size()), stem, ext;
     stem = path::stem(filename);
     ext  = path::extension(filename);
     EXPECT_EQ(*sys::path::rbegin(filename), (stem + ext).str());
 
     path::native(*i, temp_store);
   }
 
+  {
     SmallString<32> Relative("foo.cpp");
     sys::fs::make_absolute("/root", Relative);
     Relative[5] = '/'; // Fix up windows paths.
     ASSERT_EQ("/root/foo.cpp", Relative);
   }
 
+  {
+    SmallString<32> Relative("foo.cpp");
+    sys::fs::make_absolute("//root", Relative);
+    Relative[6] = '/'; // Fix up windows paths.
+    ASSERT_EQ("//root/foo.cpp", Relative);
+  }
+}
+
 TEST(Support, FilenameParent) {
   EXPECT_EQ("/", path::filename("/"));
   EXPECT_EQ("", path::parent_path("/"));
 
   EXPECT_EQ("\\", path::filename("c:\\", path::Style::windows));
   EXPECT_EQ("c:", path::parent_path("c:\\", path::Style::windows));
 
   EXPECT_EQ("/", path::filename("///"));