Cherrypick 229035: Fix error checking in get_temp_file_name().

test/support/platform_support.h

 //===----------------------------------------------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is dual licensed under the MIT and the University of Illinois Open
 // Source Licenses. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 // Define a bunch of macros that can be used in the tests instead of
@@ skipping 44 matching lines @@
 #if defined(_LIBCPP_MSVCRT) || defined(__MINGW32__)
     char Path[MAX_PATH+1];
     char FN[MAX_PATH+1];
     do { } while (0 == GetTempPath(MAX_PATH+1, Path));
     do { } while (0 == GetTempFileName(Path, "libcxx", 0, FN));
     return FN;
 #else
     std::string Name;
     int FD = -1;
     do {
-      Name = "libcxx.XXXXXX";
-      FD = mkstemp(&Name[0]);
-      assert(errno != EINVAL && "Something is wrong with the mkstemp's argument");
-    } while (FD == -1 || errno == EEXIST);
+        Name = "libcxx.XXXXXX";
+        FD = mkstemp(&Name[0]);
+        if (FD == -1 && errno == EINVAL) {

[Roland McGrath, 2015/04/15 22:35:22]

This makes an assert into something not affected by NDEBUG.
I think 'if (fd == -1 && errno == EINVAL) assert("explanation");' would be about
right.

[jvoung (off chromium), 2015/04/15 22:45:33]

I think the way it is now without the assert is okay/better. This is support
code purely for tests, so it shouldn't have mattered if there was NDEBUG or not
when building the test files.

+            perror("mkstemp");
+            abort();
+        }
+    } while (FD == -1);

[Roland McGrath, 2015/04/15 22:35:22]

do ... while (FD == -1 && errno != EEXIST);
seems to have been the original intent.

But there is no explanation about why there is a loop here at all, so some
comments are warranted.

EEXIST is the only "normal" failure mode for mkstemp, and it indicates that you
must give up now because all candidate file names are exhausted.  Other errors
mean that open failed with some other error, and repeating mkstemp might well
just repeat that other error.

Using mkstemp in a loop doesn't make sense to me.

[Sam Clegg, 2015/04/15 22:40:10]

Yes, we couldn't figure out why they would want a loop here at all.  But this is
upstream's fix for the issue.. and it does technically work.

Presumably the while loop in the win32 section above is also pointless (or
perhaps dangerous).

Simply "if (fd == -1) then ... abort" would make the most sense to me.  There is
no sensible recovery from mkstemp failure, right?

     close(FD);
     return Name;
 #endif
 }
 
 #endif // PLATFORM_SUPPORT_H