[PATCH] D121556: [randstruct] Add randomize structure layout support

Aaron Ballman via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Apr 12 04:14:39 PDT 2022


aaron.ballman added a comment.

In D121556#3444837 <https://reviews.llvm.org/D121556#3444837>, @void wrote:

> In D121556#3444434 <https://reviews.llvm.org/D121556#3444434>, @MaskRay wrote:
>
>> In D121556#3444260 <https://reviews.llvm.org/D121556#3444260>, @void wrote:
>>
>>> In D121556#3444221 <https://reviews.llvm.org/D121556#3444221>, @MaskRay wrote:
>>>
>>>> In D121556#3444131 <https://reviews.llvm.org/D121556#3444131>, @void wrote:
>>>>
>>>>> In D121556#3444021 <https://reviews.llvm.org/D121556#3444021>, @MaskRay wrote:
>>>>>
>>>>>> 7aa8c38a9e190aea14116028c38b1d9f54cbb0b3 <https://reviews.llvm.org/rG7aa8c38a9e190aea14116028c38b1d9f54cbb0b3> still uses `std::shuffle`, not incorporating the `llvm::shuffle` fixes I did.
>>>>>
>>>>> You said it was still failing after the std::shuffle to llvm::shuffle change.
>>>>
>>>> By saying it still failed, I meant there were other Windows vs non-Windows differences, not that std::shuffle=>llvm::shuffle was an unintended change.
>>>
>>> So does it work or not? If I change it to `llvm::shuffle`, will the tests fail or will they pass regardless of the platform?
>>
>> If you change `std::shuffle` to `llvm::shuffle` and  add back tests like `EXPECT_EQ(Expected, getFieldNamesFromRecord(RD));`, the test will likely fail on Windows.
>> I recall that @aaron.ballman uses Windows and may help you find the differences.
>
> It was also failing on MacOS. And it failed a different way on another Windows version. That's why I removed the test for the deterministic shuffle.
>
>> I tend to agree with your `I think it's just a case where Windows' algorithm for std::mt19937 is subtly different than the one for Linux.`
>
> The only other option would be to add the `EXPECT_*` stuff on one platform (like Linux). I suppose that would be better than nothing.

Here's what has me confused... You're using `std::mt19937` which is a very specific random number algorithm and you're giving it the same seed. I don't think we *should* be getting different behavior from the random number generator across platforms. The issue is the call to `std::shuffle` (see https://en.cppreference.com/w/cpp/algorithm/random_shuffle#Notes), so I think using `llvm::shuffle` will fix all the tests on all the platforms.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121556/new/

https://reviews.llvm.org/D121556



More information about the cfe-commits mailing list