Treat `this` as a parameter in IR generation by rdmarsh2 · Pull Request #3571 · github/codeql

rdmarsh2 · 2020-05-26T18:43:03Z

This stops generating InitalizeThis instructions for C++, in favor of treating this as a parameter, with corresponding InitializeParameter and InitializeIndirection instructions. This makes alias analysis handle the this pointer accurately. C# IR generation is not currently affected.

This closes https://github.com/github/codeql-c-analysis-team/issues/50

dbartol

Mostly LGTM. A few suggested tweaks.

Does this really not affect any dataflow results (yet)?

dbartol · 2020-05-26T19:03:20Z

 private newtype TAllocation =
  TVariableAllocation(IRVariable var) or
-  TIndirectParameterAllocation(IRAutomaticUserVariable var) {
+  TIndirectParameterAllocation(IRVariable var) {


Suggested change

TIndirectParameterAllocation(IRVariable var) {

TIndirectParameterAllocation(IRAutomaticVariable var) {

dbartol · 2020-05-26T19:03:38Z


 class IndirectParameterAllocation extends Allocation, TIndirectParameterAllocation {
-  IRAutomaticUserVariable var;
+  IRVariable var;


Suggested change

IRVariable var;

IRAutomaticVariable var;

dbartol · 2020-05-26T19:05:42Z


 newtype TInstructionTag =
  OnlyInstructionTag() or // Single instruction (not including implicit Load)
+  InitializeThisAddressTag() or


You could probably just reuse the tags we already use when generating other InitializeParameter sequences. Not a big deal either way, though.

rdmarsh2 · 2020-05-26T21:08:57Z

It does affect one result in dataflow, but this doesn't add the output side. I think it should affect a few more tests, though...

MathiasVP · 2020-05-27T06:56:25Z

Does this really not affect any dataflow results (yet)?

Apparently the reason we're not getting a lot of dataflow changes is because a setter like this:

struct A {
  int x; int y;
  void set_x(int val) {
    this->x = val;
  }
};

is not getting a ReturnIndirection from which the partial definition of this gets propagated out to the caller. With this PR I would expect that setter to behave as the following code, though:

void set_x(A* a, int val) {
  a->x = val;
}

But the IR for the first setter is:

#  118| void A::set_x(int)
#  118|   Block 0
#  118|     v118_1(void)           = EnterFunction                :
#  118|     m118_2(unknown)        = AliasedDefinition            :
#  118|     m118_3(unknown)        = InitializeNonLocal           :
#  118|     m118_4(unknown)        = Chi                          : total:m118_2, partial:m118_3
#  118|     r118_5(glval<unknown>) = VariableAddress[#this]       :
#  118|     m118_6(glval<A>)       = InitializeParameter[#this]   : &:r118_5
#  118|     r118_7(glval<A>)       = Load                         : &:r118_5, m118_6
#  118|     m118_8(A)              = InitializeIndirection[#this] : &:r118_7
#  118|     r118_9(glval<int>)     = VariableAddress[val]         :
#  118|     m118_10(int)           = InitializeParameter[val]     : &:r118_9
#  119|     r119_1(glval<int>)     = VariableAddress[val]         :
#  119|     r119_2(int)            = Load                         : &:r119_1, m118_10
#  119|     r119_3(glval<unknown>) = VariableAddress[#this]       :
#  119|     r119_4(A *)            = Load                         : &:r119_3, m118_6
#  119|     r119_5(glval<int>)     = FieldAddress[x]              : r119_4
#  119|     m119_6(int)            = Store                        : &:r119_5, r119_2
#  119|     m119_7(unknown)        = Chi                          : total:m118_8, partial:m119_6
#  120|     v120_1(void)           = NoOp                         :
#  118|     v118_11(void)          = ReturnVoid                   :
#  118|     v118_12(void)          = AliasedUse                   : m118_3
#  118|     v118_13(void)          = ExitFunction                 :

and for the second one it's:

#  124| void set_x(A*, int)
#  124|   Block 0
#  124|     v124_1(void)       = EnterFunction            :
#  124|     m124_2(unknown)    = AliasedDefinition        :
#  124|     m124_3(unknown)    = InitializeNonLocal       :
#  124|     m124_4(unknown)    = Chi                      : total:m124_2, partial:m124_3
#  124|     r124_5(glval<A *>) = VariableAddress[a]       :
#  124|     m124_6(A *)        = InitializeParameter[a]   : &:r124_5
#  124|     r124_7(A *)        = Load                     : &:r124_5, m124_6
#  124|     m124_8(unknown)    = InitializeIndirection[a] : &:r124_7
#  124|     r124_9(glval<int>) = VariableAddress[val]     :
#  124|     m124_10(int)       = InitializeParameter[val] : &:r124_9
#  125|     r125_1(glval<int>) = VariableAddress[val]     :
#  125|     r125_2(int)        = Load                     : &:r125_1, m124_10
#  125|     r125_3(glval<A *>) = VariableAddress[a]       :
#  125|     r125_4(A *)        = Load                     : &:r125_3, m124_6
#  125|     r125_5(glval<int>) = FieldAddress[x]          : r125_4
#  125|     m125_6(int)        = Store                    : &:r125_5, r125_2
#  125|     m125_7(unknown)    = Chi                      : total:m124_8, partial:m125_6
#  126|     v126_1(void)       = NoOp                     :
#  124|     v124_11(void)      = ReturnIndirection[a]     : &:r124_7, m125_7
#  124|     v124_12(void)      = ReturnVoid               :
#  124|     v124_13(void)      = AliasedUse               : m124_3
#  124|     v124_14(void)      = ExitFunction             :

@rdmarsh2 is that intentional?

rdmarsh2 · 2020-05-27T17:57:03Z

Yes. I was planning to add the ReturnIndirection in a followup PR.

Bring in new tests so their output can be fixed

dbartol

LGTM
Take a quick look at the new consistency failure in fields/dataflow-ir-consistency.expected, but if it's just a dupe of the other two existing failures, feel free to merge.

rdmarsh2 · 2020-05-28T21:15:57Z

It looks like the same issue as the existing two failures (Chi node associated with a call argument that's a pointer to a field having both the pointer type and an unknown type)

Robert Marsh added 5 commits May 22, 2020 15:35

C++: treat this as a parameter in IR

8a53dc8

C++: add getIRVariable for this temp variables

b4a947d

C#: fix compilation error in IR

1fa1bd5

C++/C#: Fix copy/pasted qldoc

43520b8

C++: add test case from issue

7ad45d5

rdmarsh2 added the C++ label May 26, 2020

rdmarsh2 requested review from a team as code owners May 26, 2020 18:43

dbartol self-assigned this May 26, 2020

dbartol reviewed May 26, 2020

View reviewed changes

Robert Marsh added 3 commits May 26, 2020 13:35

C++: Fix ThisParameterNode after IR changes

fb46002

C++: autoformat

70f6253

C++: more specific type in IndirectParameterAlloc

b45473e

Robert Marsh added 3 commits May 27, 2020 12:39

C++: accept consistency test fixes

be74616

C++: switch to TranslatedThisParameter

58673c4

C++:autoformat

54ed5d6

dbartol previously approved these changes May 28, 2020

View reviewed changes

Robert Marsh added 2 commits May 28, 2020 08:32

Merge branch 'master' into ir-this-parameter

693789c

Bring in new tests so their output can be fixed

C++: accept test fixes

d8b5d3b

rdmarsh2 dismissed dbartol’s stale review via d8b5d3b May 28, 2020 15:45

dbartol approved these changes May 28, 2020

View reviewed changes

rdmarsh2 merged commit 0d8472b into github:master May 28, 2020

rdmarsh2 mentioned this pull request May 28, 2020

C++: Fix MemoryLocation with multiple VirtualVariables #3577

Merged

MathiasVP mentioned this pull request Jun 5, 2020

C++: Remove TInitializeThisValueNumber from IR value numbering #3629

Merged

MathiasVP mentioned this pull request Nov 20, 2020

C++: Abstractions for treating qualifiers as parameters in IR #4702

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Treat `this` as a parameter in IR generation#3571

Treat `this` as a parameter in IR generation#3571
rdmarsh2 merged 13 commits into
github:masterfrom
rdmarsh2:ir-this-parameter

rdmarsh2 commented May 26, 2020

Uh oh!

dbartol left a comment

Uh oh!

dbartol May 26, 2020

Uh oh!

dbartol May 26, 2020

Uh oh!

dbartol May 26, 2020

Uh oh!

rdmarsh2 commented May 26, 2020

Uh oh!

MathiasVP commented May 27, 2020 •

edited

Loading

Uh oh!

rdmarsh2 commented May 27, 2020

Uh oh!

dbartol left a comment

Uh oh!

rdmarsh2 commented May 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	TIndirectParameterAllocation(IRVariable var) {
	TIndirectParameterAllocation(IRAutomaticVariable var) {

Uh oh!

Conversation

rdmarsh2 commented May 26, 2020

Uh oh!

dbartol left a comment

Choose a reason for hiding this comment

Uh oh!

dbartol May 26, 2020

Choose a reason for hiding this comment

Uh oh!

dbartol May 26, 2020

Choose a reason for hiding this comment

Uh oh!

dbartol May 26, 2020

Choose a reason for hiding this comment

Uh oh!

rdmarsh2 commented May 26, 2020

Uh oh!

MathiasVP commented May 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rdmarsh2 commented May 27, 2020

Uh oh!

dbartol left a comment

Choose a reason for hiding this comment

Uh oh!

rdmarsh2 commented May 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MathiasVP commented May 27, 2020 •

edited

Loading