[PATCH] D141625: [DeclContext] Sort the Decls before adding into DeclContext

Tue Jan 31 13:40:33 PST 2023

steven_wu added a comment.

In D141625#4067225 <https://reviews.llvm.org/D141625#4067225>, @dblaikie wrote:

> In D141625#4066961 <https://reviews.llvm.org/D141625#4066961>, @steven_wu wrote:
>
>> No, reverse iteration will not break diff test for a small number of decls. Everything will be in reverse order so it is the same.
>
> Hmm, I'm not sure I'm following why that is - could you explain this in more detail? The usual problem is that, say, we're outputting based on an unstable order - even two items would be enough to cause a test of that to fail in either forward or reverse iteration mode until the order is stabilized.
>
> Is that not the case here? Is there some interaction between iteration order that's part of the nondeterminism here?

In order to make a test that will break before the change with reverse iteration, the test needs to check that the decls/records are serialized into the module in a pre deterministic order. It can't be just diff the output of two runs with a small input because small input will not overflow the smallptrset, thus the reverse iteration outputs from two runs will very likely to be identical, just in a different order from forward iteration.

It is also in the same time not easy to write a test that can check the pre deterministic order of the serialization because it is hard to identify which entry is which decl from the output of bcanalyzer. For example, an entry like `<DECL_PARM_VAR abbrevid=9 op0=0 op1=18 op2=0 op3=0 op4=0 op5=0 op6=0 op7=0 op8=0 op9=3 op10=1 op11=1 op12=0 op13=2 op14=0 op15=2424 op16=24822 op17=0 op18=2424 op19=0 op20=0 op21=0 op22=0 op23=0 op24=0 op25=0 op26=0 op27=0 op28=0 op29=0 op30=0 op31=0 op32=0 op33=24842 op34=40 op35=0 op36=29/>` means nothing just looking at itself. Even worse, the opcode I check `op13` in the test is assigned via iteration order and decls are serialized via iteration order. So all the entries will always kind of appear in ascending order.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141625