Guidpcg #2

khlebnikov · 2020-10-07T20:56:25Z

No description provided.

Modified bdlb.mem to add PCGAdded bdlb_pcg.t.cpp

…erateFromPCG

…eNonSecure

khlebnikov

Added some comments to the GuidUtil integration. Will make another review round when you apply all the changes we discussed.

groups/bdl/bdlb/bdlb_guidutil.cpp

groups/bdl/bdlb/bdlb_guidutil.h

…tation of GuidUtil::generateNonSecure; modified PCG documentation

… PCG

…le; modified bldb_guidutil.cpp and bldb.mem accordingly

groups/bdl/bdlb/bdlb_pcg.h

khlebnikov · 2020-10-08T21:24:10Z

groups/bdl/bdlb/bdlb_guidutil.cpp

+        if (0 != RandomDevice::getRandomBytes((unsigned char *)&state,
+                                              sizeof(state))) {
+            state = time(NULL) ^ (intptr_t)&bsl::printf;  // fallback state
+        }


(for the future) This might be a candidate for factoring into local function - this is repeated twice here.

Yes, I considered that. Also in the GuidUtil implementation, the following code is duplicated (by my new function). Perhaps a candidate for factoring as well:
while (bytes != end) {
typedef unsigned char uc;
bytes[6] = uc(0x40 | (bytes[6] & 0x0F));
bytes[8] = uc(0x80 | (bytes[8] & 0x3F));
bytes += Guid::k_GUID_NUM_BYTES;
}
My reasoning there was that I ought not touch working code.

khlebnikov · 2020-10-08T21:53:23Z

groups/bdl/bdlb/bdlb_pcgrandomgenerator.h

+///Example 1: Generating random 32-bit numbers
+///- - - - - - - - - - - - - - - - - - - - - -
+// The PCGRandomGenerator constructor takes two 64-bit constants (the initial
+// state, and the rng sequence selector.  Instances of PCGRandomGenerator with


Suggested change

// state, and the rng sequence selector. Instances of PCGRandomGenerator with

// state, and the rng sequence selector. Instances of 'PCGRandomGenerator' with

groups/bdl/bdlb/bdlb_pcgrandomgenerator.h

Co-authored-by: khlebnikov <[email protected]>

…tion

mversche

I would lean towards having:

bsl::uint32_t bdlb::Random::generatePcg(PcgRandomGenerator *);

Which I realize is window dressing, but is where users will likely first look for a function providing random numbers.

mversche · 2020-10-09T13:23:48Z

groups/bdl/bdlb/bdlb_guidutil.h

+        // The behavior is undefined unless 'result' refers to a contiguous
+        // sequence of at least 'numGuids' Guid objects.  Note that this
+        // function uses the PCG random engine to generate high quality, albeit
+        // not cryptographically secure, random numbers for Guids.


We should explain why someone might chose t use this method:

...random numbers for Guids, which is typically faster than generating cryptographically secure guids.

groups/bdl/bdlb/bdlb_pcgrandomgenerator.h

mversche · 2020-10-09T13:30:44Z

groups/bdl/bdlb/bdlb_pcgrandomgenerator.h

+        // specified, 0 is used as the initial state.  If 'streamSelector' is
+        // not specified, 1 is used as the stream selector.
+
+    //! PCGRandomGenerator(const PCGRandomGenerator& original) = default;


Why is this a value semantic type?

mversche · 2020-10-09T13:41:24Z

groups/bdl/bdlb/bdlb_pcgrandomgenerator.h

+
+  public:
+    // CREATORS
+    PCGRandomGenerator(bsl::uint64_t initState      = 0,


What are 'initState' and 'streamSelector'? they are not explained what they do or control.

Further, there needs to be some discussion in this component (probably in whatever usage example we choose to show first) how these values should typically be chosen. This should probably show both how one might want to deterministically select them, and how one might want to generate them randomly from bdlb_RandomDevice (or whatever).

In summary: explain what initState and streamSelector do in the function doc here, then have the function doc refer to a usage example showing how someone might use these values

…nced documentation

…these; enhanced Pcgrandomgenerator documentation

khlebnikov · 2020-10-12T15:35:52Z

groups/bdl/bdlb/bdlb_guidutil.h

        // 'variant' bits set to '10' and four 'version' bits set to '0100'.

+    static void generateNonSecure(Guid *result, bsl::size_t numGuids = 1);
+    // Generate a sequence of GUIDs meeting the RFC 4122 version 4


khlebnikov · 2020-10-12T15:37:31Z

groups/bdl/bdlb/bdlb_guidutil.h

+        // is typically faster than generating cryptographically secure Guids.
+
+    static Guid generateNonSecure();
+    // Generate and return a single GUID meeting the RFC 4122 version 4


Suggested change

// Generate and return a single GUID meeting the RFC 4122 version 4

// Generate and return a single GUID meeting the RFC 4122 version 4

khlebnikov · 2020-10-12T15:42:16Z

groups/bdl/bdlb/bdlb_guidutil.t.cpp


    cout << "TEST " << __FILE__ << " CASE " << test << endl;;
    switch (test)  { case 0:
+      case 8: {


Make this case 7, Usage - case 8.

khlebnikov · 2020-10-12T15:43:12Z

groups/bdl/bdlb/bdlb_pcgrandomgenerator.h

+// int Die::roll()
+// {
+//     int result;
+


khlebnikov · 2020-10-12T15:49:37Z

groups/bdl/bdlb/bdlb_pcgrandomgenerator.h

+
+//     int d1 = a.roll();
+//     int d2 = b.roll();
+//     ASSERT(d2 == d1);


Suggested change

// ASSERT(d2 == d1);

// assert(d2 == d1);

khlebnikov · 2020-10-12T15:53:23Z

groups/bdl/bdlb/bdlb_random.h

+
+    static bsl::uint32_t generatePcg(PcgRandomGenerator *generator);
+        // Return the next unsigned 32-bit random number generated from the
+        // specified PcgRandomGenerator.


Suggested change

// specified PcgRandomGenerator.

// specified 'PcgRandomGenerator'.

khlebnikov · 2020-10-12T15:55:32Z

groups/bdl/bdlb/bdlb_random.t.cpp

                                  "==============" "\n";

        enum { NUM_ITERATIONS = 2000 };
+        {


Add generatePcg to the test plan.

…anced Random test documentation

khlebnikov

Couple small comments.

khlebnikov · 2020-10-13T19:02:32Z

groups/bdl/bdlb/bdlb_guidutil.cpp


-void GuidUtil::generate(unsigned char *result, bsl::size_t numGuids)
+namespace {
+inline


Suggested change

inline

khlebnikov · 2020-10-13T19:04:04Z

groups/bdl/bdlb/bdlb_guidutil.cpp

-void GuidUtil::generate(unsigned char *result, bsl::size_t numGuids)
+namespace {
+inline
+void makeRfc4122Compliant(unsigned char *bytes, unsigned char *end)


Move this function into the anonymous namespace at the beginning of the file: we always put such implementation helper functions before implementation of any functions from the header.

khlebnikov · 2020-10-13T19:04:40Z

groups/bdl/bdlb/bdlb_pcgrandomgenerator.h

+    // algorithm.  PCG stands for "permuted congruential generator." The state
+    // is 64 bits.  It uses a so-called stream selector, also 64 bits. For
+    // details, please refer below to the constructor.  For details of the
+    // algorithm,see http://www.pcg-random.org.


Suggested change

// algorithm,see http://www.pcg-random.org.

// algorithm, see http://www.pcg-random.org.

…; added documentation to GuidUtil component; changed to the way streamSelector is generated in GuidUtil::generateNonSecure

… beginning of the file; cleaned up some formatting

khlebnikov · 2020-10-15T16:39:19Z

groups/bdl/bdlb/bdlb_guidutil.h

+///RANDOMNESS:
+///-----------
+// There are two different ways provided for generating GUIDs:
+// 1. 'generate': comparatively more secure but slower
+// 2. 'generateNonSecure': comparatively less secure but faster
+// 'generateNonSecure' employs the PCG random engine toproduce 
+// high-quality, but not cryptographically secure, GUIDs.
+//


I think the proper name for this section is Cryptographic Security rather than RANDOMNESS

EXAMPLES below refers to the GUID String Format section, so the security section should go after those examples.

We need to provide the people who read our doc with a coherent narrative, otherwise it's difficult to understand without prior knowledge. Maybe something like this:

'GuidUtil' provides two families of functions for generating GUIDs: 'generate' and 'generateNonSecure'. The primary difference between them is cryptographic security of the resulting GUIDs and performance. The slower 'generate' methods aim to produce cryptographically secure GUIDs by accessing the underlying system resources to obtain truly random numbers, whereas faster 'generateNonSecure' methods use a fast high-quality (but not strictly cryptographically secure) in-process random-number generator.

khlebnikov · 2020-10-15T16:42:55Z

groups/bdl/bdlb/bdlb_guidutil.h

        // sequence of at least 'numGuids' Guid objects.  Note that this
        // function uses the PCG random engine to generate high quality, albeit
-        // not cryptographically secure, random numbers for Guids. This method
-        // is typically faster than generating cryptographically secure Guids.
+        // not cryptographically secure, random numbers for GUIDs. This method
+        // is typically faster than generating cryptographically secure GUIDs.


I think we should remove mention of our implementation details from out public documentation. Maybe we decide to switch to something else than PCG - it shouldn't matter to our clients. Given our changes to the component documentation, I think we should only leave Note that this function generates high-quality, albeit not cryptographically secure GUIDs. Kind of "for safety" and to explain the function name.

khlebnikov · 2020-10-15T16:45:16Z

groups/bdl/bdlb/bdlb_pcgrandomgenerator.h

+// linear congruential generator. In the PCG algorithm, rather than a single
+// seed parameter, there are two parameters, 'initState' and 'streamSelector'.


Suggested change

// linear congruential generator. In the PCG algorithm, rather than a single

// seed parameter, there are two parameters, 'initState' and 'streamSelector'.

// linear congruential generator. The PCG algorithm is seeded with two

// values: its initial state and a "stream selector".

khlebnikov · 2020-10-15T16:45:37Z

groups/bdl/bdlb/bdlb_pcgrandomgenerator.h

-// linear congruential generator.
+// linear congruential generator. In the PCG algorithm, rather than a single
+// seed parameter, there are two parameters, 'initState' and 'streamSelector'.
+// The use of a second parameter ('streamSelector') is to address a potential


Suggested change

// The use of a second parameter ('streamSelector') is to address a potential

// Stream selector is intended to address a potential

khlebnikov · 2020-10-15T16:46:19Z

groups/bdl/bdlb/bdlb_pcgrandomgenerator.h

+// linear congruential generator. In the PCG algorithm, rather than a single
+// seed parameter, there are two parameters, 'initState' and 'streamSelector'.
+// The use of a second parameter ('streamSelector') is to address a potential
+// hazard when multiple instances of a random number generator are used: an


Suggested change

// hazard when multiple instances of a random number generator are used: an

// hazard of multiple instances of a random number generators having unintended correlation between their outputs.

khlebnikov · 2020-10-15T16:51:18Z

groups/bdl/bdlb/bdlb_pcgrandomgenerator.h

+    // This class implements a random number generator (RNG) based on the PCG
+    // algorithm.  PCG stands for "permuted congruential generator." The state
+    // is 64 bits.  It uses a so-called stream selector, also 64 bits. For
+    // details, please refer below to the constructor.  For details of the
+    // algorithm, see http://www.pcg-random.org.


Incorporate this sentence PCG stands for "permuted congruential generator." and the link in component doc. Leave only the first sentence here.

khlebnikov · 2020-10-15T16:56:00Z

groups/bdl/bdlb/bdlb_pcgrandomgenerator.h

+    PcgRandomGenerator(bsl::uint64_t initState      = 0,
+                       bsl::uint64_t streamSelector = 1);
+        // Create a 'PcgRandomGenerator' object and seed it with the optionally
+        // specified 'initState' and 'streamSelector.'  If 'initState' is not


Suggested change

// specified 'initState' and 'streamSelector.' If 'initState' is not

// specified 'initState' and 'streamSelector'. If 'initState' is not

The ticks are for code font, they don't abide by the rules of English text, so the period goes outside the quote.

khlebnikov · 2020-10-15T16:58:28Z

groups/bdl/bdlb/bdlb_pcgrandomgenerator.h

+        // not specified, 1 is used as the stream selector. 'initState' is the
+        // starting state for the RNG.  Any 64-bit value may be passed.
+        // 'streamSelector' selects the output sequence for the RNG.  Any
+        // 64-bit value may be passed, although only the low 63 bits are
+        // significant.  There are 2^63 different RNGs available, and
+        // 'streamSelector' selects from among them.  Invoking different


Too much information here, which is at the same time unclear. Most of it covered in component doc, so let's keep this simple.

Suggested change

// not specified, 1 is used as the stream selector. 'initState' is the

// starting state for the RNG. Any 64-bit value may be passed.

// 'streamSelector' selects the output sequence for the RNG. Any

// 64-bit value may be passed, although only the low 63 bits are

// significant. There are 2^63 different RNGs available, and

// 'streamSelector' selects from among them. Invoking different

// not specified, 1 is used as the stream selector. Only the

// low 63 bits of 'streamSelector' are

// significant. Note that invoking different

khlebnikov · 2020-10-15T17:01:37Z

groups/bdl/bdlb/bdlb_random.t.cpp

+        //   Extracted from component header file. For 'generatePcg' (PCG)
+        //   usage example please refer bdlb_pcgrandomgenerator.t.cpp.


Suggested change

// Extracted from component header file. For 'generatePcg' (PCG)

// usage example please refer bdlb_pcgrandomgenerator.t.cpp.

// Extracted from component header file.

This is not really relevant for the test driver. The PCG stuff should be just be linked in component documentation in @SEE_ALSO section - search the code for where it goes and how it's formatted.

khlebnikov · 2020-10-15T17:02:17Z

groups/bdl/bdlb/bdlb_random.h

+
+    static bsl::uint32_t generatePcg(PcgRandomGenerator *generator);
+        // Return the next unsigned 32-bit random number generated from the
+        // specified 'PcgRandomGenerator'.


Suggested change

// specified 'PcgRandomGenerator'.

// specified 'generator'.

khlebnikov · 2020-10-15T21:32:14Z

groups/bdl/bdlb/bdlb_guidutil.h

+        // secure, random numbers for GUIDs. This method is typically faster
+        // than generating cryptographically secure GUIDs.


Remove the last sentence - it doesn't refer to this function only, thus this information is in the component doc. Also please update the rest of generateNonSecure function doc to match.

…tation

khlebnikov · 2020-10-16T15:56:58Z

groups/bdl/bdlb/bdlb_pcgrandomgenerator.h

-// a backup mechanism for obtaining random bytes.
+///Example 2: Generating unique random sequences of 32-bit numbers
+///- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
+// This example shows how one might construct two sequences of random numbers


I took a stab at this example - please see the differences in the delivery style (also note that code is indented with 2 spaces after //):

///Example 2: Using stream selector to guarantee uncorrelated sequences /// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - // This example illustrates how the stream selector can be used to guarantee // that two distinct instances of 'PCGRandomGenerator' produce uncorrelated // sequences. // // First, we use the 'RandomDevice' to choose the initial states for the // generators using a source of true randomness: //.. // uint64_t state1; // int rc = bdlb::RandomDevice::getRandomBytes( // reinterpret_cast<unsigned char *>(&state1), sizeof(state1)); // (void)rc; // error handling omitted // // uint64_t state2; // int rc = bdlb::RandomDevice::getRandomBytes( // reinterpret_cast<unsigned char *>(&state2), sizeof(state2)); // (void)rc; // error handling omitted //.. // Then we select two distinct stream selectors for the generators, which, due // to the PCG algorithmic properties will guarantee that the sequences will be // uncorrelated even if the initial state is exactly the same: //.. // const uint64_t streamSelector1 = 1; // const uint64_t streamSelector2 = 2; //.. // Finally, we initialize the generators with their respective initial states // and stream selectors and check that they produce distinct sequences of // random numbers. The check is guaranteed to pass even in the rare, but // possible case of 'state1 == state2'. //.. // bdlb::PcgRandomGenerator rng1(state1, streamSelector1); // bdlb::PcgRandomGenerator rng2(state2, streamSelector2); // // const int NUM_VALUES = 1000; // bsl::vector<bsl::uint32_t> sequence1(NUM_VALUES); // bsl::vector<bsl::uint32_t> sequence2(NUM_VALUES); // // for (int i = 0; i < NUM_VALUES; ++i) { // sequence1[i] = rng1.generate(); // sequence2[i] = rng2.generate(); // } // // assert(sequence1 != sequence2); //..

khlebnikov · 2020-10-16T15:57:58Z

groups/bdl/bdlb/bdlb_pcgrandomgenerator.h

+// stands for "permuted congruential generator." For details of the algorithm,
+// see http://www.pcg-random.org.  The PCG technique employs the concepts of


Suggested change

// stands for "permuted congruential generator." For details of the algorithm,

// see http://www.pcg-random.org. The PCG technique employs the concepts of

// stands for "permuted congruential generator" (see http://www.pcg-random.org

// for details). The PCG technique employs the concepts of

* add references to 'bdlb_stringviewutil' in the doc * add 'std::string' overloads; test 'toLower', 'toUpper' * 'std::string' overloads: test 'pad' * 'std::string' overloads: test 'ltrim', 'rtrim', 'trim' * add 'std::pmr::string' overloads; test 'toLower', 'toUpper' * clear compiler warnings and bde_verify diagnostics * 'std::pmr::string' overloads: test 'pad' * 'std::pmr::string' overloads: test 'ltrim', 'rtrim', 'trim' * fix build for use of 'std::pmr::string' * adjust 'bdlb_string.h' doc * clear bde_verify warning * doc improvements prompted by Attila's review comments * implementation changes suggested by Attila * test driver changes per Attila's review comments * final test driver changes per Attila's review comments * address Attila's review comments

Harold Bott added 7 commits September 25, 2020 10:44

adding PCG

1769661

Added operator== and operator!= to bdlb_pcg.h

788a07d

Modified bdlb.mem to add PCGAdded bdlb_pcg.t.cpp

removed exception language from .h; added a stub for the .t.cpp

39f4ece

fleshed out the test driver a bit

e32c04c

added return code for test failure

5d3ea52

modified PCG to change method name; modified GuidUtil, adding new gen…

d83f918

…erateFromPCG

modified PCG documentation; changed GuidUtil function name to generat…

a66dfe4

…eNonSecure

khlebnikov commented Oct 7, 2020

View reviewed changes

Harold Bott added 3 commits October 7, 2020 20:03

reverted inadvertent formatting changes in GuidUtil; changed implemen…

5446630

…tation of GuidUtil::generateNonSecure; modified PCG documentation

let the compiler generate copy constructor and assignment operator of…

888d419

… PCG

changed the name of PCG to PCGRandomGenerator; simplified usage examp…

a67b5d7

…le; modified bldb_guidutil.cpp and bldb.mem accordingly

khlebnikov commented Oct 8, 2020

View reviewed changes

groups/bdl/bdlb/bdlb_pcg.h Outdated Show resolved Hide resolved

khlebnikov commented Oct 8, 2020

View reviewed changes

added renamed pcgrandomgenerator files

19b505b

khlebnikov commented Oct 8, 2020

View reviewed changes

bottha123 and others added 3 commits October 8, 2020 20:08

Update groups/bdl/bdlb/bdlb_pcgrandomgenerator.h

1d0b36f

Co-authored-by: khlebnikov <[email protected]>

added example to pcgrandomgenerator; minor modifications to documenta…

7fe591b

…tion

Merge branch 'guidpcg' of github.com:bottha123/bde into guidpcg

d82e06b

mversche approved these changes Oct 9, 2020

View reviewed changes

Harold Bott added 3 commits October 9, 2020 12:36

minor change to return type in usage example

514c563

added test cases and usage examples for bdlb_pcgrandomgenerator; enha…

697b172

…nced documentation

added overloads to GuidUtil::generateNonSecure; added test cases for …

dd33c42

…these; enhanced Pcgrandomgenerator documentation

khlebnikov commented Oct 13, 2020

View reviewed changes

addressing review comments; refactored GuidUtil bit manipulation; enh…

369d3f1

…anced Random test documentation

khlebnikov commented Oct 13, 2020

View reviewed changes

Harold Bott added 3 commits October 13, 2020 17:04

explanation of the streamSelector in Pcgrandomgenerator documentation…

f68db7b

…; added documentation to GuidUtil component; changed to the way streamSelector is generated in GuidUtil::generateNonSecure

in GuidUtil, moved makeRfc4122Compliant to anonymous namespace at the…

99834c6

… beginning of the file; cleaned up some formatting

fixed output formatting

ef9257b

khlebnikov commented Oct 15, 2020

View reviewed changes

changes to the documentation

d6f994d

khlebnikov commented Oct 15, 2020

View reviewed changes

Harold Bott added 2 commits October 15, 2020 20:03

replaced Usage Example 2 in bdlb_pcgrandomgenerator; modified documen…

7215dd4

…tation

enhanced Usage Example 2; modified formatting and documentation

00d98ff

khlebnikov commented Oct 16, 2020

View reviewed changes

reworking of Example 2; small change to DESCRIPTION

d8c1c42

	// state, and the rng sequence selector. Instances of PCGRandomGenerator with
	// state, and the rng sequence selector. Instances of 'PCGRandomGenerator' with

	// Generate and return a single GUID meeting the RFC 4122 version 4
	// Generate and return a single GUID meeting the RFC 4122 version 4

	// specified PcgRandomGenerator.
	// specified 'PcgRandomGenerator'.

	// algorithm,see http://www.pcg-random.org.
	// algorithm, see http://www.pcg-random.org.

		// linear congruential generator. In the PCG algorithm, rather than a single
		// seed parameter, there are two parameters, 'initState' and 'streamSelector'.

	// The use of a second parameter ('streamSelector') is to address a potential
	// Stream selector is intended to address a potential

	// hazard when multiple instances of a random number generator are used: an
	// hazard of multiple instances of a random number generators having unintended correlation between their outputs.

	// specified 'initState' and 'streamSelector.' If 'initState' is not
	// specified 'initState' and 'streamSelector'. If 'initState' is not

		// Extracted from component header file. For 'generatePcg' (PCG)
		// usage example please refer bdlb_pcgrandomgenerator.t.cpp.

		// secure, random numbers for GUIDs. This method is typically faster
		// than generating cryptographically secure GUIDs.

		// stands for "permuted congruential generator." For details of the algorithm,
		// see http://www.pcg-random.org. The PCG technique employs the concepts of

Guidpcg #2

Are you sure you want to change the base?

Guidpcg #2

Uh oh!

Conversation

khlebnikov commented Oct 7, 2020

Uh oh!

khlebnikov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mversche left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

khlebnikov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

khlebnikov Oct 16, 2020 •

edited

Loading