-
Notifications
You must be signed in to change notification settings - Fork 67
C: Address remaining gcc compiler compatability issues #247
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This test case had some syntactically invalid C code, and a name clash, which have now been addressed.
Labels cannot be before declarations, only before expressions or statements.
Compliers already exclude the #define defined case.
Cases are labels, and cannot be before declarations.
This rule has a clang specific error, not permitting certain function declarations. Update test case and expected results to reflect this.
For this test case our standard library stubs (MUSL) use a different form of macro for these standard library functions that gcc and clang, so update the expected result files.
The <ctype.h> commonly implements its APIs using either macros or functions or some combination of the two. Our query only assumed functions were used, whereas macros are practically used by both gcc and clang, and these can vary depending on compiler flags. The CharFunctions.qll library now provides a unified interface from which to get a unique expression for each use of an API in the library, hopefully regardless of whether it is a macro or a function. To do this we have had to hard code assumptions about the structure of the macros, however our matrix compiler testing should flag if these assumptions are broken with a particular version of a supported compiler.
🤖 Beep Boop! Matrix Testing for this PR has been initiated. Please check back later for results. |
🤖 Beep Boop! clang/cpp/x86_64 Matrix Testing for this PR has been completed but I didn't find anything to test! |
🤖 Beep Boop! gcc/cpp/x86_64 Matrix Testing for this PR has been completed but I didn't find anything to test! |
🤖 Beep Boop! gcc/c/x86_64 Matrix Testing for this PR has been completed. See below for the results!
|
🤖 Beep Boop! clang/c/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! Matrix Testing for this PR has been initiated. Please check back later for results. |
🤖 Beep Boop! clang/cpp/x86_64 Matrix Testing for this PR has been completed but I didn't find anything to test! |
🤖 Beep Boop! gcc/cpp/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. See below for the results!
|
🤖 Beep Boop! gcc/c/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! |
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.
Great work @lcartey! Make sure to close the backing issues before/after merging!
Description
This PR addresses all the remaining
gcc
issuesC
compiler compatibility issues.In line with the previous issues we've reviewed, the majority of the fixes are either (a) fixing test cases with syntactically invalid C code; or (b) including a different expected results file for different compilers, because the results are morally equivalent but textually different.
The sole exception in this PR is
STR37-C
. This rule prohibits the use of the<ctype>
functions. However, the C standard allows these "functions" to be implemented either as functions or macros, and we only detected the former. Detecting "arguments" to macros is tricky, because they are textually expanded and may appear in multiple places in the AST generated from the macro, with no easy way to determine which aspects came from the argument itself. We handle this by looking for specific AST patterns corresponding to known, supported compilers, and identifying a "unique" element to report as the argument itself.Fixes #246.
Finally, as an additional "bonus" I've updated
Rule 2.4
to excluded template parameters.Change request type
.ql
,.qll
,.qls
or unit tests)Rules with added or modified queries
Rule 2.4
Rule 2.6
Rule 21.1
DCL41-C
Rule 8.2
STR34-C
STR37-C
Release 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.