Fixing the final compatibility issues #338
Merged
Conversation
This file contains 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 with the standard library structure.
This commit restructures the string, string.h and cstring headers to match the expected headers and namespaces of C++ standard declarations. This addresses compatibility issues with real compilers because our test cases expected to access 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 think this 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 compilers causing our detection of modifying function calls to fail. This has been addressed, with a bonus modification to more accurately represent which pointer/reference types are captured.
reset() is sometimes declared on a base class. Similar issue to A8-4-13, so I have refactored the SmartPointer class to provide predicates which identify the 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 in libstdc++, 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 retained to ensure we better reflect real compilers.
This query included some spurious edges for results that are outside the source location. We now exclude constructors outside the source archive to avoid these spurious edges, and make the result more stable.
Our useless include query is looking for includes where nothing from the included file is used by the including file. In this case, the declaration of v transitively uses std::size_t, and `#include <algorithm>` transitively includes the file that defines std::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 just exclude these expected results, as modifying the query is tricky. Furthermore, the intention of the rule is actually that we check standard library includes against the list of symbols as per the standard library, but again this is challenging.
Fix for `PreventDeadlockByLockingInPredefinedOrder`
lcartey
changed the title
|
/test-matrix |
|
/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. |
|
🤖 Beep Boop! qcc/cpp/AARCH64LE Matrix Testing for this PR won't happen because it is outside of license window! |
|
🤖 Beep Boop! qcc/c/AARCH64LE Matrix Testing for this PR won't happen because it is outside of license window! |
|
🤖 Beep Boop! gcc/c/X86_64 Matrix Testing for this PR has been completed but I didn't find anything to test! |
|
🤖 Beep Boop! clang/c/X86_64 Matrix Testing for this PR has been completed but I didn't find anything to test! |
|
🤖 Beep Boop! clang/cpp/X86_64 Matrix Testing for this PR has been completed. See below for the results! |
|
🤖 Beep Boop! gcc/cpp/X86_64 Matrix Testing for this PR has been completed. See below for the results! |
|
🤖 Beep Boop! Matrix Testing for this PR has been completed. If no reports were posted it means this PR does not contain things that need matrix testing! |
|
🏁 Beep Boop! Performance testing complete! See below for performance of the last 3 runs vs your PR. Times are based on predicate performance. You can find 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 to be reported by CodeQL. Adjust to summarize and exclude.
…hub/codeql-coding-standards into lcartey/final-compiler-compat-issues
|
No performance concerns, and I've addressed the |
jsinglet
approved these changes
Aug 15, 2023
Sign up for free
to 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.
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 of shared 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.