[Patch] Implement sys::Process::GetRandomNumber on Windows

[Aaron Ballman]

Ah, okay! Well, thank you for the patch -- I committed it as r200767

~Aaron

On Tue, Feb 4, 2014 at 9:37 AM, Stephan Tolksdorf <st at quanttec.com> wrote:
> On 14-02-04 14:56, Aaron Ballman wrote:
>>
>> LGTM!
>
>
> I forgot to write: I don't have commit access, please commit for me. Thanks!
>
> - Stephan
>
>
>>
>> ~Aaron
>>
>> On Tue, Feb 4, 2014 at 7:54 AM, Stephan Tolksdorf <st at quanttec.com> wrote:
>>>
>>> Aaron Ballman wrote:
>>>>
>>>>
>>>> Aside from minor nits, LGTM. Nits below:
>>>
>>>
>>>
>>> Thank you for the review! I've attached an updated patch that
>>> incorporates
>>> the suggested changes.
>>>
>>> - Stephan
>>>
>>>
>>>>
>>>>>   From b2976da4fc38c9fb41498915af3d6eee8bac7267 Mon Sep 17 00:00:00
>>>>> 2001
>>>>> From: Stephan Tolksdorf <st at quanttec.com>
>>>>> Date: Mon, 3 Feb 2014 21:25:01 +0100
>>>>> Subject: Implement llvm::sys::Process::GetRandomNumber() for Windows
>>>>>
>>>>> This function was already implemented for Unix. The implementation uses
>>>>> the
>>>>> rand_s function, which according to MSDN is available on Windows XP and
>>>>> later.
>>>>> libc++ also uses rand_s for implementing random_device. I've checked
>>>>> that mingw-w64 supports rand_s.
>>>>> ---
>>>>>    lib/Support/Process.cpp           | 4 ++++
>>>>>    lib/Support/Windows/Process.inc   | 7 +++++++
>>>>>    unittests/Support/ProcessTest.cpp | 6 ++++++
>>>>>    3 files changed, 17 insertions(+)
>>>>>
>>>>> diff --git a/lib/Support/Process.cpp b/lib/Support/Process.cpp
>>>>> index d5168f0..009c7c8 100644
>>>>> --- a/lib/Support/Process.cpp
>>>>> +++ b/lib/Support/Process.cpp
>>>>> @@ -9,12 +9,16 @@
>>>>>    //
>>>>>    //  This header file implements the operating system Process
>>>>> concept.
>>>>>    //
>>>>>
>>>>>
>>>>> //===----------------------------------------------------------------------===//
>>>>>
>>>>>    #include "llvm/Config/config.h"
>>>>> +#if LLVM_ON_WIN32
>>>>> +# define _CRT_RAND_S // makes stdlib.h declare the rand_s function
>>>>
>>>>
>>>>
>>>> LLVM comments are usually complete sentences (capitalization, period,
>>>> etc).
>>>>
>>>>> +# include <stdlib.h>
>>>>> +#endif
>>>>>    #include "llvm/Support/ErrorHandling.h"
>>>>>    #include "llvm/Support/Process.h"
>>>>>
>>>>>    using namespace llvm;
>>>>>    using namespace sys;
>>>>>
>>>>> diff --git a/lib/Support/Windows/Process.inc
>>>>> b/lib/Support/Windows/Process.inc
>>>>> index 750097e..73ce97a 100644
>>>>> --- a/lib/Support/Windows/Process.inc
>>>>> +++ b/lib/Support/Windows/Process.inc
>>>>> @@ -357,6 +357,13 @@ const char *Process::OutputReverse() {
>>>>>
>>>>>    const char *Process::ResetColor() {
>>>>>      if (UseANSI) return "\033[0m";
>>>>>      SetConsoleTextAttribute(GetStdHandle(STD_OUTPUT_HANDLE),
>>>>> defaultColors());
>>>>>      return 0;
>>>>>    }
>>>>> +
>>>>> +unsigned Process::GetRandomNumber() {
>>>>> +  unsigned result;
>>>>> +  const errno_t ec = rand_s(&result); // supported in Win XP and later
>>>>
>>>>
>>>>
>>>> I would remove the comment; also, result should be declared to match
>>>> rand_s's signature (so unsigned int instead of vanilla unsigned).
>>>>
>>>>> +  assert(ec == 0 && "rand_s failed");
>>>>> +  return result;
>>>>> +}
>>>>> diff --git a/unittests/Support/ProcessTest.cpp
>>>>> b/unittests/Support/ProcessTest.cpp
>>>>> index af6a6f9..387d5d3 100644
>>>>> --- a/unittests/Support/ProcessTest.cpp
>>>>> +++ b/unittests/Support/ProcessTest.cpp
>>>>> @@ -36,12 +36,18 @@ TEST(ProcessTest, SelfProcess) {
>>>>>      EXPECT_LT(TimeValue::MinTime,
>>>>> process::get_self()->get_system_time());
>>>>>      EXPECT_GT(TimeValue::MaxTime,
>>>>> process::get_self()->get_system_time());
>>>>>      EXPECT_LT(TimeValue::MinTime,
>>>>> process::get_self()->get_wall_time());
>>>>>      EXPECT_GT(TimeValue::MaxTime,
>>>>> process::get_self()->get_wall_time());
>>>>>    }
>>>>>
>>>>> +TEST(ProcessTest, GetRandomNumberTest) {
>>>>> +  const unsigned r1 = Process::GetRandomNumber();
>>>>> +  const unsigned r2 = Process::GetRandomNumber();
>>>>> +  EXPECT_NE((r1 | r2), 0); // 0 should be extremely unlikely
>>>>> +}
>>>>> +
>>>>>    #ifdef _MSC_VER
>>>>>    #define setenv(name, var, ignore) _putenv_s(name, var)
>>>>>    #endif
>>>>>
>>>>>    #if HAVE_SETENV || _MSC_VER
>>>>>    TEST(ProcessTest, Basic) {
>>>>> --
>>>>> 1.7.11.msysgit.1
>>>>
>>>>
>>>>
>>>> ~Aaron
>>>>
>>>> On Mon, Feb 3, 2014 at 7:03 PM, Stephan Tolksdorf <st at quanttec.com>
>>>> wrote:
>>>>>
>>>>>
>>>>> Hi,
>>>>>
>>>>> This is a little patch that implements sys::Process::GetRandomNumber on
>>>>> Windows. Currently this function only has a Unix implementation.
>>>>>
>>>>> The implementation uses the s_rand function, which is available on
>>>>> Windows
>>>>> XP and later. It's the same function that libc++ uses for implementing
>>>>> random_device on Windows.
>>>>>
>>>>> - Stephan
>>>>>
>>>>> _______________________________________________
>>>>> llvm-commits mailing list
>>>>> llvm-commits at cs.uiuc.edu
>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>>>
>>>
>