[PATCH] D60835: [Serialization] Stable serialization order for OpenCLTypeExtMap and OpenCLDeclExtMap

Thu Apr 18 06:17:57 PDT 2019

riccibruno added a comment.

In D60835#1471498 <https://reviews.llvm.org/D60835#1471498>, @Anastasia wrote:

> In D60835#1470805 <https://reviews.llvm.org/D60835#1470805>, @riccibruno wrote:
>
> > By the way, I am wondering about how much this is tested. I did look quickly in `test/PCH` and it appears that there are only 3 (short) tests : `ocl_types.cl`, `opencl-extensions.cl` and `no-validate-pch`.
>
>
> This is tested in  test/SemaOpenCL/extension-begin.cl: https://reviews.llvm.org/D53200

Ah I see, I did indeed miss that.

> The ordering in PCH isn't tested anywhere however, but I am not clear how to check for nondeterminism properly. Because the fact that the same output is generated N times won't guarantee that it's the same N+1 times. Especially I do believe it's likely on the same revision the same output to be produced but it's less likely across revisions. :(
> 
> We could still go for something like the following but accept that some a random failure might happen not necessarily on a commit that introduces it?

I am not sure that this is needed. Non-deterministic tests are really annoying.

> In D60778#1468825 <https://reviews.llvm.org/D60778#1468825>, @lebedev.ri wrote:
> 
>> > Not sure how to test this though? I guess we can trust that std::map is always sorted just as it says in its description.
>>
>> You could add a test that contains several entries in `OpenCLTypeExtMap` and several entries in `OpenCLDeclExtMap`.
>>  In that test, write te PCH, and then apply `llvm-bcanalyzer` to that PCH. It should dump it the bitcode in textual dump.
>>  And then simply apply FileCheck to that textual dump, with CHECK lines requiring the specific order of these entries.
>>  If the order is not deterministic in PCH, then that test would (should!) fail sporadically.
>>  At least that is my guess.

Repository:
  rC Clang

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

https://reviews.llvm.org/D60835