Skip to content

Conversation

MathiasVP
Copy link
Contributor

Now that arguments are present in IR path explanations (since #5361), this PR gets rid of the Argument -1 indirection string on indirections for this.

(Part of https://github.com/github/codeql-c-analysis-team/issues/242.)

@MathiasVP MathiasVP added the C++ label Mar 9, 2021
@MathiasVP MathiasVP requested a review from a team as a code owner March 9, 2021 09:24
@MathiasVP MathiasVP added the no-change-note-required This PR does not need a change note label Mar 9, 2021
@jbj
Copy link
Contributor

jbj commented Mar 9, 2021

This indirection is a case of this causing grammatical confusion because this might be mistaken for an English word rather than the this pointer. (sorry)

How about 'this-indirection' or 'this-object' instead? Then it's in lower case, and the hyphen prevents confusion with the English word.

@jbj
Copy link
Contributor

jbj commented Mar 9, 2021

I also liked *this for its brevity, but that style seems to have fallen out of favour after #5361.

@MathiasVP
Copy link
Contributor Author

MathiasVP commented Mar 9, 2021

This indirection is a case of this causing grammatical confusion because this might be mistaken for an English word rather than the this pointer. (sorry)

How about 'this-indirection' or 'this-object' instead? Then it's in lower case, and the hyphen prevents confusion with the English word.

I like this-indirection. I picked This indirection since it matched with what we have for ArgumentNodes (which are currently shown as This argument). Whatever we end up showing in the path explanations, I think they should follow the same grammatical style.

@MathiasVP
Copy link
Contributor Author

MathiasVP commented Mar 9, 2021

I also liked *this for its brevity, but that style seems to have fallen out of favour after #5361.

I'm not sure I understand what you mean. The *this string should still be present for the parameter indirection. So after merging this PR (argh, too many this!) and #5361 we should have flow from This indirection to *this in path explanations.

@MathiasVP
Copy link
Contributor Author

0f6c56a changes the names to better match the AST dataflow library. The number of strings generated appears to be mostly unchanged. On main with Wireshark we get:

Tuple counts for DataFlowUtil::Node::toString_dispred#ff/2@f564a8:
    17746    ~1%     {2} r1 = SCAN DataFlowUtil::ThisParameterNode#class#ff OUTPUT In.0 'this', "this"
    17746    ~1%     {2} r2 = STREAM DEDUP r1
    
    477155   ~0%     {2} r3 = JOIN DataFlowUtil::ParameterNode::isParameterOf_dispred#fff#shared WITH Element::ElementBase::toString_dispred#ff@staged_ext ON FIRST 1 OUTPUT Lhs.1 'this', Rhs.1 'result'
    
    494901   ~2%     {2} r4 = r2 UNION r3
    
    5520951  ~3%     {2} r5 = SCAN DataFlowUtil::ExprNode#class#ff OUTPUT In.1, In.0 'this'
    5520951  ~2%     {2} r6 = JOIN r5 WITH Instruction::Instruction::getConvertedResultExpression_dispred#ff ON FIRST 1 OUTPUT Rhs.1, Lhs.1 'this'
    5520950  ~0%     {2} r7 = JOIN r6 WITH Element::ElementBase::toString_dispred#ff@staged_ext ON FIRST 1 OUTPUT Lhs.1 'this', Rhs.1 'result'
    
    2317701  ~0%     {2} r8 = SCAN DataFlowPrivate::PrimaryArgumentNode#class#ff OUTPUT In.1, In.0 'this'
    
    2241169  ~0%     {2} r9 = JOIN r8 WITH Operand::PositionalArgumentOperand::getIndex_dispred#3#ff ON FIRST 1 OUTPUT Lhs.1 'this', ("Argument " ++ toString(Rhs.1))
    
    76532    ~1%     {2} r10 = JOIN r8 WITH Operand::ThisArgumentOperand#class#3#ffff ON FIRST 1 OUTPUT Lhs.1 'this', "This argument"
    
    2317701  ~0%     {2} r11 = r9 UNION r10
    7838651  ~0%     {2} r12 = r7 UNION r11
    8333552  ~0%     {2} r13 = r4 UNION r12
    
    20517434 ~4%     {2} r14 = DataFlowUtil::TOperandNode#ff@staged_ext AND NOT DataFlowPrivate::PrimaryArgumentNode#class#ff_0#antijoin_rhs(Lhs.1 'this')
    19269033 ~4%     {2} r15 = r14 AND NOT DataFlowPrivate::SideEffectArgumentNode#class#fff_0#antijoin_rhs(Lhs.1 'this')
    19269033 ~0%     {2} r16 = JOIN r15 WITH Operand::Operand::toString_dispred#ff ON FIRST 1 OUTPUT Lhs.1 'this', Rhs.1 'result'
    
    2392     ~2%     {2} r17 = DataFlowUtil::DefinitionByReferenceNode#class#ff AND NOT DataFlowUtil::Node::toString_dispred#ff#antijoin_rhs(Lhs.0 'this', Lhs.1)
    2392     ~3%     {2} r18 = SCAN r17 OUTPUT In.0 'this', "output argument"
    2392     ~3%     {2} r19 = STREAM DEDUP r18
    
    339416   ~0%     {2} r20 = SCAN DataFlowUtil::ParameterIndirectionNode#class#ff OUTPUT In.1, In.0 'this'
    340658   ~0%     {2} r21 = JOIN r20 WITH Instruction::VariableInstruction#3#ff ON FIRST 1 OUTPUT Rhs.1, Lhs.1 'this'
    340658   ~4%     {2} r22 = JOIN r21 WITH IRVariable::IRVariable::toString_dispred#2#ff ON FIRST 1 OUTPUT Lhs.1 'this', ("*" ++ Rhs.1)
    
    343050   ~4%     {2} r23 = r19 UNION r22
    19612083 ~0%     {2} r24 = r16 UNION r23
    
    10514309 ~2%     {2} r25 = JOIN DataFlowUtil::InstructionNode_not_DefinitionByReferenceNode_ExplicitParameterNode_ExprNode_ParameterIndirectionNode_ThisParameterNode#f WITH DataFlowUtil::TInstructionNode#ff_10#join_rhs ON FIRST 1 OUTPUT Rhs.1, Rhs.0
    10514309 ~0%     {2} r26 = JOIN r25 WITH SSAConstruction::Cached::getInstructionOpcode#2#ff@staged_ext ON FIRST 1 OUTPUT Rhs.1, Lhs.1 'this'
    10514309 ~3%     {2} r27 = JOIN r26 WITH Opcode::Opcode::toString_dispred#ff ON FIRST 1 OUTPUT Lhs.1 'this', Rhs.1 'result'
    
    1248401  ~0%     {2} r28 = SCAN DataFlowPrivate::SideEffectArgumentNode#class#fff OUTPUT In.2, In.0 'this'
    1248401  ~1%     {2} r29 = JOIN r28 WITH Instruction::IndexedInstruction#ff ON FIRST 1 OUTPUT Lhs.1 'this', ("Argument " ++ toString(Rhs.1) ++ " indirection")
    
    1252520  ~2%     {2} r30 = SCAN DataFlowUtil::DefinitionByReferenceNode#class#ff OUTPUT In.1, In.0 'this'
    1252520  ~3%     {2} r31 = JOIN r30 WITH Instruction::SideEffectInstruction::getPrimaryInstruction_dispred#3#ff ON FIRST 1 OUTPUT Rhs.1, Lhs.1 'this'
    1250513  ~5%     {2} r32 = JOIN r31 WITH Instruction::CallInstruction::getStaticCallTarget_dispred#3#ff ON FIRST 1 OUTPUT Rhs.1, Lhs.1 'this'
    1250509  ~0%     {2} r33 = JOIN r32 WITH Declaration::Declaration::getName_dispred#ff ON FIRST 1 OUTPUT Lhs.1 'this', (Rhs.1 ++ " output argument")
    
    2498910  ~2%     {2} r34 = r29 UNION r33
    13013219 ~4%     {2} r35 = r27 UNION r34
    32625302 ~4%     {2} r36 = r24 UNION r35
    40958854 ~2%     {2} r37 = r13 UNION r36
                     return r37
...
>>> Created relation DataFlowUtil::Node::toString_dispred#ff/2@f564a8 with 40957227 rows.

and with this PR:

(1403s) Tuple counts for DataFlowUtil::Node::toString_dispred#ff/2@b88dff:
    17746    ~1%     {2} r1 = SCAN DataFlowUtil::ThisParameterNode#class#ff OUTPUT In.0 'this', "this"
    17746    ~1%     {2} r2 = STREAM DEDUP r1
    
    477155   ~0%     {2} r3 = JOIN DataFlowUtil::ParameterNode::isParameterOf_dispred#fff#shared WITH Element::ElementBase::toString_dispred#ff@staged_ext ON FIRST 1 OUTPUT Lhs.1 'this', Rhs.1 'result'
    
    5520951  ~3%     {2} r4 = SCAN DataFlowUtil::ExprNode#class#ff OUTPUT In.1, In.0 'this'
    5520951  ~2%     {2} r5 = JOIN r4 WITH Instruction::Instruction::getConvertedResultExpression_dispred#ff ON FIRST 1 OUTPUT Rhs.1, Lhs.1 'this'
    5520950  ~0%     {2} r6 = JOIN r5 WITH Element::ElementBase::toString_dispred#ff@staged_ext ON FIRST 1 OUTPUT Lhs.1 'this', Rhs.1 'result'
    
    5998105  ~0%     {2} r7 = r3 UNION r6
    6015851  ~0%     {2} r8 = r2 UNION r7
    
    20517434 ~4%     {2} r9 = DataFlowUtil::TOperandNode#ff@staged_ext AND NOT DataFlowPrivate::PrimaryArgumentNode#class#ff_0#antijoin_rhs(Lhs.1 'this')
    19269033 ~4%     {2} r10 = r9 AND NOT DataFlowPrivate::SideEffectArgumentNode#class#fff_0#antijoin_rhs(Lhs.1 'this')
    19269033 ~6%     {2} r11 = JOIN r10 WITH Operand::Operand::toString_dispred#ff ON FIRST 1 OUTPUT Lhs.1 'this', Rhs.1 'result'
    
    2392     ~2%     {2} r12 = DataFlowUtil::DefinitionByReferenceNode#class#ff AND NOT DataFlowUtil::Node::toString_dispred#ff#antijoin_rhs(Lhs.0 'this', Lhs.1)
    2392     ~1%     {2} r13 = SCAN r12 OUTPUT In.0 'this', "output argument"
    2392     ~1%     {2} r14 = STREAM DEDUP r13
    
    19271425 ~6%     {2} r15 = r11 UNION r14
    
    339416   ~0%     {2} r16 = SCAN DataFlowUtil::ParameterIndirectionNode#class#ff OUTPUT In.1, In.0 'this'
    340658   ~0%     {2} r17 = JOIN r16 WITH Instruction::VariableInstruction#3#ff ON FIRST 1 OUTPUT Rhs.1, Lhs.1 'this'
    340658   ~0%     {2} r18 = JOIN r17 WITH IRVariable::IRVariable::toString_dispred#2#ff ON FIRST 1 OUTPUT Lhs.1 'this', ("*" ++ Rhs.1)
    
    10514309 ~2%     {2} r19 = JOIN DataFlowUtil::InstructionNode_not_DefinitionByReferenceNode_ExplicitParameterNode_ExprNode_ParameterIndirectionNode_ThisParameterNode#f WITH DataFlowUtil::TInstructionNode#ff_10#join_rhs ON FIRST 1 OUTPUT Rhs.1, Rhs.0
    10514309 ~0%     {2} r20 = JOIN r19 WITH SSAConstruction::Cached::getInstructionOpcode#2#ff@staged_ext ON FIRST 1 OUTPUT Rhs.1, Lhs.1 'this'
    10514309 ~0%     {2} r21 = JOIN r20 WITH Opcode::Opcode::toString_dispred#ff ON FIRST 1 OUTPUT Lhs.1 'this', Rhs.1 'result'
    
    10854967 ~0%     {2} r22 = r18 UNION r21
    30126392 ~3%     {2} r23 = r15 UNION r22
    36142243 ~0%     {2} r24 = r8 UNION r23
    
    11997    ~0%     {2} r25 = DataFlowPrivate::PrimaryArgumentNode#class#ff AND NOT DataFlowUtil::Node::toString_dispred#ff#antijoin_rhs#1(Lhs.0 'this', Lhs.1)
    
    11997    ~0%     {2} r26 = SCAN r25 OUTPUT In.1, In.0 'this'
    
    3196     ~0%     {2} r27 = JOIN r26 WITH Operand::PositionalArgumentOperand::getIndex_dispred#3#ff ON FIRST 1 OUTPUT Lhs.1 'this', ("Argument " ++ toString(Rhs.1))
    
    2317701  ~0%     {2} r28 = SCAN DataFlowPrivate::PrimaryArgumentNode#class#ff OUTPUT In.1, In.0 'this'
    2317701  ~4%     {2} r29 = JOIN r28 WITH Operand::Operand::getDef_dispred#3#ff ON FIRST 1 OUTPUT Rhs.1, Lhs.1 'this'
    2305704  ~2%     {2} r30 = JOIN r29 WITH Instruction::Instruction::getUnconvertedResultExpression_dispred#ff ON FIRST 1 OUTPUT Rhs.1, Lhs.1 'this'
    2305704  ~2%     {2} r31 = JOIN r30 WITH Element::ElementBase::toString_dispred#ff@staged_ext ON FIRST 1 OUTPUT Lhs.1 'this', Rhs.1 'result'
    
    8801     ~0%     {2} r32 = JOIN r26 WITH Operand::ThisArgumentOperand#class#3#ffff ON FIRST 1 OUTPUT Lhs.1 'this', "Argument this"
    
    2314505  ~1%     {2} r33 = r31 UNION r32
    2317701  ~1%     {2} r34 = r27 UNION r33
    
    4        ~0%     {3} r35 = SCAN DataFlowUtil::Node::toString_dispred#ff#shared OUTPUT In.1, -1, In.0 'this'
    0        ~0%     {2} r36 = JOIN r35 WITH Instruction::IndexedInstruction#ff ON FIRST 2 OUTPUT Lhs.2 'this', "Argument this indirection"
    
    4        ~0%     {2} r37 = DataFlowUtil::Node::toString_dispred#ff#shared AND NOT DataFlowUtil::Node::toString_dispred#ff#antijoin_rhs#3(Lhs.0 'this', Lhs.1)
    4        ~0%     {2} r38 = SCAN r37 OUTPUT In.1, In.0 'this'
    4        ~0%     {2} r39 = JOIN r38 WITH Instruction::IndexedInstruction#ff ON FIRST 1 OUTPUT Lhs.1 'this', ("Argument " ++ toString(Rhs.1) ++ " indirection")
    
    4        ~0%     {2} r40 = r36 UNION r39
    
    1248401  ~0%     {2} r41 = SCAN DataFlowPrivate::SideEffectArgumentNode#class#fff OUTPUT In.2, In.0 'this'
    1248397  ~2%     {2} r42 = JOIN r41 WITH Instruction::ReadSideEffectInstruction::getArgumentDef_dispred#ff ON FIRST 1 OUTPUT Rhs.1, Lhs.1 'this'
    1248397  ~0%     {2} r43 = JOIN r42 WITH Instruction::Instruction::getUnconvertedResultExpression_dispred#ff ON FIRST 1 OUTPUT Rhs.1, Lhs.1 'this'
    1248397  ~7%     {2} r44 = JOIN r43 WITH Element::ElementBase::toString_dispred#ff@staged_ext ON FIRST 1 OUTPUT Lhs.1 'this', (Rhs.1 'result' ++ " indirection")
    
    1252520  ~2%     {2} r45 = SCAN DataFlowUtil::DefinitionByReferenceNode#class#ff OUTPUT In.1, In.0 'this'
    1252520  ~3%     {2} r46 = JOIN r45 WITH Instruction::SideEffectInstruction::getPrimaryInstruction_dispred#3#ff ON FIRST 1 OUTPUT Rhs.1, Lhs.1 'this'
    1250513  ~5%     {2} r47 = JOIN r46 WITH Instruction::CallInstruction::getStaticCallTarget_dispred#3#ff ON FIRST 1 OUTPUT Rhs.1, Lhs.1 'this'
    1250509  ~7%     {2} r48 = JOIN r47 WITH Declaration::Declaration::getName_dispred#ff ON FIRST 1 OUTPUT Lhs.1 'this', (Rhs.1 ++ " output argument")
    
    2498906  ~4%     {2} r49 = r44 UNION r48
    2498910  ~4%     {2} r50 = r40 UNION r49
    4816611  ~2%     {2} r51 = r34 UNION r50
    40958854 ~0%     {2} r52 = r24 UNION r51
                     return r52
...
>>> Created relation DataFlowUtil::Node::toString_dispred#ff/2@b88dff with 40957227 rows.

@MathiasVP MathiasVP added the depends on internal PR This PR should only be merged in sync with an internal Semmle PR label Mar 11, 2021
@MathiasVP MathiasVP merged commit 01cc2f2 into github:main Mar 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C++ depends on internal PR This PR should only be merged in sync with an internal Semmle PR no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants