The Wayback Machine - https://web.archive.org/web/20260323042612/https://github.com/github/codeql/pull/8912
Skip to content

C++: Fix IR variable reuse for global var inits#8912

Merged
MathiasVP merged 15 commits intogithub:mainfrom
rdmarsh2:rdmarsh2/fix-ir-globals
Jun 20, 2022
Merged

C++: Fix IR variable reuse for global var inits#8912
MathiasVP merged 15 commits intogithub:mainfrom
rdmarsh2:rdmarsh2/fix-ir-globals

Conversation

@rdmarsh2
Copy link
Contributor

Fixes a performance issue where all IRVariables corresponding to a given global variable were being used in the initializer IRFunction for that global variable.

@rdmarsh2 rdmarsh2 requested review from a team as code owners April 27, 2022 20:38
@rdmarsh2 rdmarsh2 added the no-change-note-required This PR does not need a change note label Apr 27, 2022
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found 1 vulnerability.

@jketema

This comment was marked as outdated.

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found 1 vulnerability.

dbartol
dbartol previously approved these changes Apr 27, 2022
Copy link

@dbartol dbartol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as long as the tests pass.

@jketema
Copy link
Contributor

jketema commented Apr 28, 2022

I fixed the test regressions caused by running against the old frontend. However, it turns out, also some syntax zoo tests broke (unrelated to the frontend). I could update those, but I cannot tell if the changes are correct.

Also the IR VariableAddresses without names look funny especially for declarations like:

int global_6 = global_2;

This becomes:

# 1769| int global_6
# 1769|   Block 0
# 1769|     v1769_1(void)       = EnterFunction     : 
# 1769|     mu1769_2(unknown)   = AliasedDefinition : 
# 1769|     r1769_3(glval<int>) = VariableAddress   : 
# 1769|     r1769_4(glval<int>) = VariableAddress   : 
# 1769|     r1769_5(int)        = Load[?]           : &:r1769_4, ~m?
# 1769|     mu1769_6(int)       = Store[?]          : &:r1769_3, r1769_5
# 1769|     v1769_7(void)       = ReturnVoid        : 
# 1769|     v1769_8(void)       = AliasedUse        : ~m?
# 1769|     v1769_9(void)       = ExitFunction      : 

Which VariableAddress is which here?

@MathiasVP MathiasVP mentioned this pull request Apr 28, 2022
@rdmarsh2 rdmarsh2 force-pushed the rdmarsh2/fix-ir-globals branch from 37fb227 to fe52dd9 Compare April 29, 2022 19:30
@rdmarsh2
Copy link
Contributor Author

rdmarsh2 commented May 2, 2022

Also the IR VariableAddresses without names look funny especially for declarations like:

Fixed by d1c6022

@jketema
Copy link
Contributor

jketema commented May 2, 2022

Fixed by d1c6022

Thanks. Would it be possible to add int global_6 = global_2; as a test?

@rdmarsh2
Copy link
Contributor Author

rdmarsh2 commented May 2, 2022

Good call - I've added the test and fixed another case of missing variables

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found 9 vulnerabilities.

Comment on lines +949 to +951
/**
* Represents the IR translation of a root element, either a function or a global variable.
*/

Check warning

Code scanning / CodeQL

Class QLDoc style.

The QLDoc for a class should start with 'A', 'An', or 'The'.
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found 1 vulnerability.

result = this.getInstruction(InitializerVariableAddressTag())
or
tag = InitializerVariableAddressTag() and
result = getChild(1).getFirstInstruction()

Check warning

Code scanning / CodeQL

Using implicit `this`

Use of implicit `this`.
Copy link
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything LGTM! Do we need to rerun DCA? I don't think much has changed apart from a merge-from-main and accepting some test changes, right?

@rdmarsh2
Copy link
Contributor Author

rdmarsh2 commented Jun 15, 2022

I don't think we do... My only worry would be how much change there's been to main in the meantime, but there hasn't been much else happening on the C++ dataflow side

@MathiasVP MathiasVP merged commit 35c8ca1 into github:main Jun 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C# C++ no-change-note-required This PR does not need a change note

4 participants