[Support] use larger character set for creating unique filenames

[inglorion]

Summary

This changes the character set we use to create unique filenames from
16 characters to 36, which allows more unique filenames to be
generated for the same name length.

[inglorion]

The inspiration for this is http://crbug.com/856635, where we're seeing the same file name generated multiple times during a single run.

[zturner]

It would be an extra line of code, but I think this would be clearer (with
even less chance of collision) if you just choose a random number in [0,35]
and map it to [0-9][a-z]. The bit twiddling and array indexing seems like
unnecessary cleverness and someone could accidentally come along and break
it if they weren’t careful.

[inglorion]

Yeah. On top of the cost of getting a number from the cryptographically secure random number generator, I don't think using mod instead of and will cost that much. I'll make the change. We could add a few more characters ("-_@", probably others), but I think digits+letters is already a good improvement to what we currently have, without having to worry about file systems not liking some characters.

[inglorion]

Use 36-character set instead of 32-character set, per @zturner's suggestion.

I also realized that this would make the TempFileCollisions test flaky
(about 2% failure rate), so I modified it to get the expected failure
rate down below 1 per million.

[thakis]

In D50127#1183838, @inglorion wrote:

The inspiration for this is http://crbug.com/856635, where we're seeing the same file name generated multiple times during a single run.

https://bugs.chromium.org/p/chromium/issues/detail?id=856635#c10 sounds like we only saw this if the pattern was made smaller. Are we sure this change fixes an actual problem?

[inglorion]

In D50127#1185981, @thakis wrote:

https://bugs.chromium.org/p/chromium/issues/detail?id=856635#c10 sounds like we only saw this if the pattern was made smaller. Are we sure this change fixes an actual problem?

The failure in that bug does not reproduce on my local machine, but when I make the pattern smaller, I end up with a failure that looks a lot like it. For reference, with the old 16-character set and models of length 6, we have a 50%+ chance of collision after 5000 tempfiles or so. We easily generate that many in a ThinLTO build of Chromium. So it seems at least plausible that this is the same problem.

Aside from that, I think this is just a good thing to do. We're effectively increasing the probability that a name supposed to be unique is indeed unique, without making file names longer or consuming more entropy from the random number generator.

clang/test/Analysis/diagnostics/plist-multi-file.c

 // CHECK-NEXT:  <key>location</key>
 // CHECK-NEXT:  <dict>
 // CHECK-NEXT:   <key>line</key><integer>2</integer>
 // CHECK-NEXT:   <key>col</key><integer>8</integer>
 // CHECK-NEXT:   <key>file</key><integer>1</integer>
 // CHECK-NEXT:  </dict>
 // CHECK-NEXT:  <key>HTMLDiagnostics_files</key>
 // CHECK-NEXT:  <array>
-// CHECK-NEXT:   <string>report-{{([0-9a-f]{6})}}.html</string>
+// CHECK-NEXT:   <string>report-{{([0-9a-z]{6})}}.html</string>
 // CHECK-NEXT:  </array>
 // CHECK-NEXT:  </dict>
 // CHECK-NEXT: </array>

llvm/lib/Support/Path.cpp

createUniqueEntity(const Twine &Model, int &ResultFD,
 
   // Limit the number of attempts we make, so that we don't infinite loop when
   // we run out of filenames that fit the model.
   std::error_code EC;
   for (int Retries = 128; Retries > 0; --Retries) {
     // Replace '%' with random chars.
     for (unsigned i = 0, e = ModelStorage.size(); i != e; ++i) {
       if (ModelStorage[i] == '%')
-        ResultPath[i] =
-            "0123456789abcdef"[sys::Process::GetRandomNumber() & 15];
+        ResultPath[i] = "0123456789abcdefghijklmnopqrstuvwxyz"
+            [sys::Process::GetRandomNumber() % 36];
     }
 
     // Try to open + create the file.
     switch (Type) {
     case FS_File: {
       EC = sys::fs::openFileForReadWrite(Twine(ResultPath.begin()), ResultFD,
                                          sys::fs::CD_CreateNew, Flags, Mode);
       if (EC) {

llvm/unittests/Support/Path.cpp

ASSERT_NO_ERROR(
       fs::createUniqueDirectory("CreateUniqueFileTest", TestDirectory));
   FileRemover Cleanup(TestDirectory);
   SmallString<128> Model = TestDirectory;
   path::append(Model, "%.tmp");
   SmallString<128> Path;
   std::vector<fs::TempFile> TempFiles;
 
   auto TryCreateTempFile = [&]() {
+    // For the last available name, there is a 35/36 probability that we will
+    // pick a name that is already in use. TempFile::create() tries a number
+    // of names before giving up, but with only one character, this still leaves
+    // a chance of failure. We retry a few times here to get the probability
+    // of failure down to less than one in a million.
     Expected<fs::TempFile> T = fs::TempFile::create(Model);
+    for (int retries = 4; !T && retries > 0; --retries) {
+      consumeError(T.takeError());
+      T = fs::TempFile::create(Model);
+    }
     if (T) {
       TempFiles.push_back(std::move(*T));
       return true;
     } else {
       logAllUnhandledErrors(T.takeError(), errs(),
                             "Failed to create temporary file: ");
       return false;
     }
   };
 
-  // We should be able to create exactly 16 temporary files.
-  for (int i = 0; i < 16; ++i)
+  // We should be able to create exactly 36 temporary files.
+  for (int i = 0; i < 36; ++i)
     EXPECT_TRUE(TryCreateTempFile());
   EXPECT_FALSE(TryCreateTempFile());
 
   for (fs::TempFile &T : TempFiles)
     cantFail(T.discard());
 }
 
 TEST_F(FileSystemTest, CreateDir) {