- Notifications
You must be signed in to change notification settings - Fork70
Fixing the final compatibility issues#338
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Merged
Merged
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
This should have no impact on tests, but addresses an incompatibility withthe standard library structure.
This commit restructures the string, string.h and cstring headers to match theexpected headers and namespaces of C++ standard declarations. This addressescompatibility issues with real compilers because our test cases expected toaccess C string functionality through the "#include <string>" header include,which is not the case in practice.This commit addresses compiler compatibility issues in the following rules: * A12-0-2 * A27-0-2 * M16-0-5 * M18-0-5 * DCL55-CPP * EXP62-CPP * OOP57-CPP * STR50-CPP
clang also supports -w for disabling all options.clang/A1-1-2 was not on the list of open issues, but gcc/A1-1-2 was - I thinkthis is an error in the matrix testing.
Fix false positives identified by compiler compatibility testing on gcc/clang,which identified that shared_ptr used a hidden base class in real compilerscausing our detection of modifying function calls to fail. This has beenaddressed, with a bonus modification to more accurately represent whichpointer/reference types are captured.
reset() is sometimes declared on a base class. Similar issue to A8-4-13, so Ihave refactored the SmartPointer class to provide predicates which identifythe operations across multiple compilers.
Fix false negative issues related to the library structure of smart pointers.This commit makes the following changes: * Update `memory` stubs to move more functions to the __shared_ptr base class * Add dataflow summaries for smart pointer constructor calls and smart pointer get calls. * Add sanitizers to prevent flow into library code for the dataflow summaries added above.
The std::string::replace function uses an internal typedef __const_iterator inlibstdc++, instead of the const_iterator typedef.
Mark ~mutex() as deleted, as that is what we see in real libraries.Also modify lock_guard. This didn't have any affect on the test, but retainedto ensure we better reflect real compilers.
This query included some spurious edges for results that are outside thesource location. We now exclude constructors outside the source archiveto avoid these spurious edges, and make the result more stable.
Our useless include query is looking for includes where nothing from theincluded file is used by the including file. In this case, the declaration ofv transitively uses std::size_t, and `#include <algorithm>` transitivelyincludes the file that defines std::size_t. To detect such cases we would needto report redundant includes e.g. includes for which useful symbols areprovided, but which are made unnecessary by other imports in the file.For now we just exclude these expected results, as modifying the query istricky. Furthermore, the intention of the rule is actually that we checkstandard library includes against the list of symbols as per the standardlibrary, but again this is challenging.
Fix for `PreventDeadlockByLockingInPredefinedOrder`
ContributorAuthor
lcartey commentedAug 10, 2023
/test-matrix |
ContributorAuthor
lcartey commentedAug 10, 2023
/test-performance |
🤖 Beep Boop! Matrix Testing for this PR has been initiated. Please check back later for results. |
🏁 Beep Boop! Performance testing for this PR has been initiated. Please check back later for results. Note that the query package generation step must complete before testing will start so it might be a minute. |
Contributor
jsinglet commentedAug 10, 2023
🤖 Beep Boop!qcc/cpp/AARCH64LE Matrix Testing for this PR won't happen because it is outside of license window! |
Contributor
jsinglet commentedAug 10, 2023
🤖 Beep Boop!qcc/c/AARCH64LE Matrix Testing for this PR won't happen because it is outside of license window! |
Contributor
jsinglet commentedAug 10, 2023
🤖 Beep Boop!gcc/c/X86_64 Matrix Testing for this PR has been completed but I didn't find anything to test! |
Contributor
jsinglet commentedAug 10, 2023
🤖 Beep Boop!clang/c/X86_64 Matrix Testing for this PR has been completed but I didn't find anything to test! |
Contributor
jsinglet commentedAug 10, 2023
🤖 Beep Boop!clang/cpp/X86_64 Matrix Testing for this PR has been completed. See below for the results! |
Contributor
jsinglet commentedAug 10, 2023
🤖 Beep Boop!gcc/cpp/X86_64 Matrix Testing for this PR has been completed. See below for the results! |
Contributor
jsinglet commentedAug 10, 2023
🤖 Beep Boop! Matrix Testing for this PR has beencompleted. If no reports were posted it means this PR does not contain things that need matrix testing! |
Contributor
jsinglet commentedAug 11, 2023
🏁 Beep Boop! Performance testing complete! See below for performance of the last 3 runs vs your PR. Times are based on predicate performance. You canfind full graphs and stats in the PR that was created for this test in the release engineering repo. 🏁 Below are the slowest predicates for the last 2 releases vs this PR. |
libc++ defines release inline in the header, which causes extraneous paths tobe reported by CodeQL. Adjust to summarize and exclude.
…hub/codeql-coding-standards into lcartey/final-compiler-compat-issues
ContributorAuthor
lcartey commentedAug 13, 2023
No performance concerns, and I've addressed the |
jsinglet approved these changesAug 15, 2023
Contributor
jsinglet left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks great Luke! Thanks for all this work getting these sorted 💪 🚀
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading.Please reload this page.
Description
Address the remaining compiler compatibility issues identified by our integration testing.
Summary of notable commits:
string,string.handcstring#include <string>, and C++ string functionality was accessible via#include "string.h". All these issues have been addressed.A12-0-2,A27-0-2,M16-0-5,M18-0-5,DCL55-CPP,EXP62-CPP,OOP57-CPP,STR50-CPP.A8-4-13,A18-1-4,A20-8-1,MEM56-CPP.A20-8-1,MEM56-CPP.std::string::replacemodellingstd::stringon many standard libraries uses an internal typedef__const_iteratorinstead of the standard specifiedconst_iterator, and this was preventing us from identifying calls toreplace.STR51-CPP.operator delete.A15-5-1is updated to determinenoexceptstatus of the definition of a function only, and the alert message is updated to provide clarity on what is reported.A15-5-1.A18-5-5,A18-5-6.mutexstub header to better reflect real C++ librariesmutexand provide definitions forlock_guardconstructor/destructor to match gcc/clang.PreventDeadlockByLockingInPredefinedOrderbased on these changes.CON50-CPP.A15-2-2.vtransitively usesstd::size_t, and#include <algorithm>transitively includes the file that definesstd::size_t. To detect such cases we would need to report redundant includes e.g. includes for which useful symbols are provided, but which are made unnecessary by other imports in the file. For now we simply accept the results.A16-2-2.Change request type
.ql,.qll,.qlsor unit tests)Rules with added or modified queries
A12-0-2,A27-0-2,M16-0-5,M18-0-5,DCL55-CPP,EXP62-CPP,OOP57-CPP,STR50-CPP,A8-4-13,A18-1-4,A20-8-1,MEM56-CPP,A20-8-1,STR51-CPP,A15-5-1,A18-5-5,A18-5-6,CON50-CPP,A15-2-2,A16-2-2Release change checklist
A change note (development_handbook.md#change-notes) is required for any pull request which modifies:
If you are only adding new rule queries, a change note is not required.
Author: Is a change note required?
🚨🚨🚨
Reviewer: Confirm that format ofshared queries (not the .qll file, the
.ql file that imports it) is valid by running them within VS Code.
Reviewer: Confirm that either a change note is not required or the change note is required and has been added.
Query development review checklist
For PRs that add new queries or modify existing queries, the following checklist should be completed by both the author and reviewer:
Author
As a rule of thumb, predicates specific to the query should take no more than 1 minute, and for simple queries be under 10 seconds. If this is not the case, this should be highlighted and agreed in the code review process.
Reviewer
As a rule of thumb, predicates specific to the query should take no more than 1 minute, and for simple queries be under 10 seconds. If this is not the case, this should be highlighted and agreed in the code review process.