C++: Fix join order in bbSuccessorEntryReaches

[MathiasVP]

This PR does two things:

• Removes a join-on-SemanticStackVariable (which was giving horrible tuple counts):
• Adds nomagic to bbSuccessorEntryReaches.

The added magic wasn't actually bad, but it shouldn't be necessary to evaluate the predicate. And there's no reason to wait for some bad magic to be pushed into the predicate before adding a nomagic.

Here are the tuple counts before these changes:

Starting to evaluate predicate StackVariableReachability::StackVariableReachability::bbSuccessorEntryReaches#f0820431#bfbff/5@i7#3d62cw2q (iteration 7)
Tuple counts for StackVariableReachability::StackVariableReachability::bbSuccessorEntryReaches#f0820431#bfbff/5@i7#3d62cw2q after 6.4s:
  496610   ~94%     {5} r1 = SCAN StackVariableReachability::StackVariableReachability::bbSuccessorEntryReaches#f0820431#bfbff#prev_delta OUTPUT In.2 'v', In.0 'this', In.1, In.3 'node', In.4
  
  248305   ~0%      {5} r2 = JOIN r1 WITH UninitializedLocal::declWithNoInit#9d706ecd#ff ON FIRST 1 OUTPUT Lhs.1 'this', Lhs.2, Lhs.0 'v', Lhs.3 'node', Lhs.4
  0        ~0%      {5} r3 = r2 AND NOT UninitializedLocal::UninitialisedLocalReachability#class#9d706ecd#f(Lhs.0 'this')
  0        ~0%      {5} r4 = SCAN r3 OUTPUT In.1, In.4, In.0 'this', In.2 'v', In.3 'node'
  0        ~0%      {5} r5 = JOIN r4 WITH StackVariableReachability::bbSuccessorEntryReachesLoopInvariant#4b7489ec#ffff_1302#join_rhs ON FIRST 2 OUTPUT Lhs.2 'this', Lhs.3 'v', Lhs.4 'node', Rhs.2 'bb', Rhs.3 'skipsFirstLoopAlwaysTrueUponEntry'
  
  66303687 ~5%      {6} r6 = JOIN r1 WITH StackVariableReachability::StackVariableReachability::isBarrier#dispred#f0820431#cpe#23#fb_10#join_rhs ON FIRST 1 OUTPUT Rhs.1, Lhs.2, Lhs.1 'this', Lhs.0 'v', Lhs.3 'node', Lhs.4
  29847    ~34%     {5} r7 = JOIN r6 WITH project#BasicBlocks::Cached::basic_block_member#8748103a ON FIRST 2 OUTPUT Lhs.2 'this', Lhs.1, Lhs.3 'v', Lhs.4 'node', Lhs.5
                    {5} r8 = MATERIALIZE r7 AS unknown
  
  226089   ~0%      {5} r9 = StackVariableReachability::StackVariableReachability::bbSuccessorEntryReaches#f0820431#bfbff#prev_delta AND NOT r8(Lhs.0 'this', Lhs.1, Lhs.2 'v', Lhs.3 'node', Lhs.4)
  226089   ~0%      {5} r10 = SCAN r9 OUTPUT In.2 'v', In.0 'this', In.1, In.3 'node', In.4
  226089   ~7%      {5} r11 = JOIN r10 WITH UninitializedLocal::declWithNoInit#9d706ecd#ff ON FIRST 1 OUTPUT Lhs.2, Lhs.4, Lhs.1 'this', Lhs.0 'v', Lhs.3 'node'
  427750   ~17%     {5} r12 = JOIN r11 WITH StackVariableReachability::bbSuccessorEntryReachesLoopInvariant#4b7489ec#ffff_1302#join_rhs ON FIRST 2 OUTPUT Lhs.2 'this', Lhs.3 'v', Lhs.4 'node', Rhs.2 'bb', Rhs.3 'skipsFirstLoopAlwaysTrueUponEntry'
  
  427750   ~17%     {5} r13 = r5 UNION r12
  388172   ~15%     {5} r14 = r13 AND NOT StackVariableReachability::StackVariableReachability::bbSuccessorEntryReaches#f0820431#bfbff#prev(Lhs.0 'this', Lhs.3 'bb', Lhs.1 'v', Lhs.2 'node', Lhs.4 'skipsFirstLoopAlwaysTrueUponEntry')
  388172   ~5%      {5} r15 = SCAN r14 OUTPUT In.0 'this', In.3 'bb', In.1 'v', In.2 'node', In.4 'skipsFirstLoopAlwaysTrueUponEntry'
                    return r15

And after:

Starting to evaluate predicate StackVariableReachability::StackVariableReachability::bbSuccessorEntryReaches#f0820431#fffff/5@i7#4c3cex3c (iteration 7)
Tuple counts for StackVariableReachability::StackVariableReachability::bbSuccessorEntryReaches#f0820431#fffff/5@i7#4c3cex3c after 673ms:
  749842  ~94%      {5} r1 = SCAN StackVariableReachability::StackVariableReachability::bbSuccessorEntryReaches#dispred#f0820431#fffff#prev_delta OUTPUT In.2 'v', In.0 'this', In.1, In.3 'node', In.4
  749842  ~101%     {5} r2 = JOIN r1 WITH Variable::SemanticStackVariable#class#7a968d4e#f ON FIRST 1 OUTPUT Lhs.1 'this', Lhs.2, Lhs.3 'node', Lhs.4, Lhs.0 'v'
  
  374921  ~4%       {5} r3 = JOIN r2 WITH UninitializedLocal::UninitialisedLocalReachability#class#9d706ecd#f ON FIRST 1 OUTPUT Lhs.1, Lhs.0 'this', Lhs.2 'node', Lhs.3, Lhs.4 'v'
  2469321 ~0%       {6} r4 = JOIN r3 WITH project#BasicBlocks::Cached::basic_block_member#8748103a_10#join_rhs ON FIRST 1 OUTPUT Rhs.1, Lhs.4 'v', Lhs.1 'this', Lhs.0, Lhs.2 'node', Lhs.3
  110157  ~17%      {5} r5 = JOIN r4 WITH StackVariableReachability::StackVariableReachability::isBarrier#dispred#f0820431#cpe#23#ff ON FIRST 2 OUTPUT Lhs.2 'this', Lhs.3, Lhs.4 'node', Lhs.5, Lhs.1 'v'
                    {5} r6 = MATERIALIZE r5 AS unknown
  
  281138  ~0%       {5} r7 = r2 AND NOT r6(Lhs.0 'this', Lhs.1, Lhs.2 'node', Lhs.3, Lhs.4 'v')
  281138  ~10%      {5} r8 = SCAN r7 OUTPUT In.1, In.3, In.0 'this', In.2 'node', In.4 'v'
  337571  ~4%       {5} r9 = JOIN r8 WITH StackVariableReachability::bbSuccessorEntryReachesLoopInvariant#4b7489ec#ffff_1302#join_rhs ON FIRST 2 OUTPUT Lhs.2 'this', Lhs.3 'node', Lhs.4 'v', Rhs.2 'bb', Rhs.3 'skipsFirstLoopAlwaysTrueUponEntry'
  293370  ~2%       {5} r10 = r9 AND NOT StackVariableReachability::StackVariableReachability::bbSuccessorEntryReaches#f0820431#fffff#prev(Lhs.0 'this', Lhs.3 'bb', Lhs.2 'v', Lhs.1 'node', Lhs.4 'skipsFirstLoopAlwaysTrueUponEntry')
  293370  ~0%       {5} r11 = SCAN r10 OUTPUT In.0 'this', In.3 'bb', In.2 'v', In.1 'node', In.4 'skipsFirstLoopAlwaysTrueUponEntry'
                    return r11

[jketema]

LGTM assuming DCA results are ok.

The only thing I'm a bit puzzled about: where is UninitialisedLocalReachability coming from that I see in the tuple counts?

[MathiasVP]

The only thing I'm a bit puzzled about: where is UninitialisedLocalReachability coming from that I see in the tuple counts?

Yeah, sorry. I should've been more clear. This join appears when running the UninitializedLocal.ql query.

The UninitialisedLocalReachability part is the extension of the abstract "configuration" class StackVariableReachability from here: https://github.com/github/codeql/blob/main/cpp/ql/src/Likely%20Bugs/Memory%20Management/UninitializedLocal.ql#L47

[jketema]

abstract "configuration" class

I don't know where else this is used, but did you look at the tuple counts in those contexts?

[MathiasVP]

I don't know where else this is used, but did you look at the tuple counts in those contexts?

Fair point. I didn't actually investigate the tuple counts for the other uses of this class. With that said, I doubt joining on the variable is ever a good idea. But let's wait and see what DCA says! 🤞

[MathiasVP]

I think DCA is okay, but there's some slowdown on two projects that I'd like to check up on.