-
Notifications
You must be signed in to change notification settings - Fork 77
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
Only allow the Checked C extension flag for C. #16
Conversation
This change only allows the -fcheckedc-extension flag to be used for C programs in clang. The clang driver will reject the use of -fcheckedc-extension for other C family languages supported by clang, including C++, Objective C, OpenCL, and CUDA. This addresses issue checkedc#9 in the checked-clang Github repo. We are currently not modifying clang to support these other languages, which is why need to disallow using the extension with them. Testing: - Add 4 new tests to clang. They test that use of the extension flag is rejected for C++, Objective C, CUDA, and OpenCL. - As recommended by the clang documentation, I placed the tests in with other similar tests in the test tree. - I updated the testing baselines to reflect the new tests. I also updated the documentation to reflect the fact that we have Checked C specific tests in clang. We need to take the new tests into account when updating to new versions of the clang/LLVM sources.
Hi @dtarditi, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
TTYL, MSBOT; |
in these configurations. | ||
|
||
- For just clang with the x86 target (using the check-clang project), 3 additional `Expected Passes` tests. | ||
- For testing LLVM + clang on x86 (check-all), 8 additional `Expected Passes` tests |
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.
I saw your updated instructions for keeping this .md file up to date, but I just get the sense that it's going to get out-of-sync. (And if it stays in sync, I'm wondering if it's going to be unnecessary developer burden for each change that adds a new test.)
I know it's important to keep in agreement on which failures are expected (because they show up in the base line) and agree we should keep that updated--but do we need to update the counts of tests we've added? I'm not sure how much we'll get from that...
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.
Agreed. Updating precise counts will likely become a developer burden. We only need to track 'Unexpected failures' in the baseline branch. We don't particularly need to track the counts. I'll update the md files in the pull request to reflect that.
I did have some concerns that people will add tests incorrectly and not notice they aren't running. However, this seems pretty unlikely given the way that the LLVM test infrastructure works. It will produce an error for tests that are lacking RUN lines. The only requirement during code reviews to make sure that tests will run is to make sure they are placed within test suite directories.
Finished taking a pass over this; my main question is around the overhead to maintain the expected test counts for tests we add. Apart from that, this looks good. (And thanks for putting in the effort to add those test cases!) |
Modify the new C test to use Checked C syntax for an argument type. Compile the test using clang-cl.
I've updated the documentation so that we will not maintain expected passing test counts, which could add overhead. Instead we'll just make sure that there are no unexpected test failures. I addressed your other comments and updated the issue9 branch. One neat thing: we're now checking that clang-cl understands the -fcheckedc-extension flag when it is passed using the -Xclang option to clang-cl. |
…#16) Cherry-picked from commit 2ca19552da07bd697aa84cea8289cbe3e5bb1462 We have a separate test directory for Checked C language feature tests. These need their own lit configuration scripts. We've cloned the clang lit configuration scripts and altered them slightly for this purpose. The Checked C scripts have diverged from the clang ones because we haven't been tracking changes to the clang scripts. This brings the scripts back into alignment.
When `Target::GetEntryPointAddress()` calls `exe_module->GetObjectFile()->GetEntryPointAddress()`, and the returned `entry_addr` is valid, it can immediately be returned. However, just before that, an `llvm::Error` value has been setup, but in this case it is not consumed before returning, like is done further below in the function. In https://bugs.freebsd.org/248745 we got a bug report for this, where a very simple test case aborts and dumps core: ``` * thread #1, name = 'testcase', stop reason = breakpoint 1.1 frame #0: 0x00000000002018d4 testcase`main(argc=1, argv=0x00007fffffffea18) at testcase.c:3:5 1 int main(int argc, char *argv[]) 2 { -> 3 return 0; 4 } (lldb) p argc Program aborted due to an unhandled Error: Error value was Success. (Note: Success values must still be checked prior to being destroyed). Thread 1 received signal SIGABRT, Aborted. thr_kill () at thr_kill.S:3 3 thr_kill.S: No such file or directory. (gdb) bt #0 thr_kill () at thr_kill.S:3 #1 0x00000008049a0004 in __raise (s=6) at /usr/src/lib/libc/gen/raise.c:52 #2 0x0000000804916229 in abort () at /usr/src/lib/libc/stdlib/abort.c:67 #3 0x000000000451b5f5 in fatalUncheckedError () at /usr/src/contrib/llvm-project/llvm/lib/Support/Error.cpp:112 #4 0x00000000019cf008 in GetEntryPointAddress () at /usr/src/contrib/llvm-project/llvm/include/llvm/Support/Error.h:267 #5 0x0000000001bccbd8 in ConstructorSetup () at /usr/src/contrib/llvm-project/lldb/source/Target/ThreadPlanCallFunction.cpp:67 #6 0x0000000001bcd2c0 in ThreadPlanCallFunction () at /usr/src/contrib/llvm-project/lldb/source/Target/ThreadPlanCallFunction.cpp:114 #7 0x00000000020076d4 in InferiorCallMmap () at /usr/src/contrib/llvm-project/lldb/source/Plugins/Process/Utility/InferiorCallPOSIX.cpp:97 #8 0x0000000001f4be33 in DoAllocateMemory () at /usr/src/contrib/llvm-project/lldb/source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp:604 #9 0x0000000001fe51b9 in AllocatePage () at /usr/src/contrib/llvm-project/lldb/source/Target/Memory.cpp:347 #10 0x0000000001fe5385 in AllocateMemory () at /usr/src/contrib/llvm-project/lldb/source/Target/Memory.cpp:383 #11 0x0000000001974da2 in AllocateMemory () at /usr/src/contrib/llvm-project/lldb/source/Target/Process.cpp:2301 #12 CanJIT () at /usr/src/contrib/llvm-project/lldb/source/Target/Process.cpp:2331 #13 0x0000000001a1bf3d in Evaluate () at /usr/src/contrib/llvm-project/lldb/source/Expression/UserExpression.cpp:190 #14 0x00000000019ce7a2 in EvaluateExpression () at /usr/src/contrib/llvm-project/lldb/source/Target/Target.cpp:2372 #15 0x0000000001ad784c in EvaluateExpression () at /usr/src/contrib/llvm-project/lldb/source/Commands/CommandObjectExpression.cpp:414 #16 0x0000000001ad86ae in DoExecute () at /usr/src/contrib/llvm-project/lldb/source/Commands/CommandObjectExpression.cpp:646 #17 0x0000000001a5e3ed in Execute () at /usr/src/contrib/llvm-project/lldb/source/Interpreter/CommandObject.cpp:1003 #18 0x0000000001a6c4a3 in HandleCommand () at /usr/src/contrib/llvm-project/lldb/source/Interpreter/CommandInterpreter.cpp:1762 #19 0x0000000001a6f98c in IOHandlerInputComplete () at /usr/src/contrib/llvm-project/lldb/source/Interpreter/CommandInterpreter.cpp:2760 #20 0x0000000001a90b08 in Run () at /usr/src/contrib/llvm-project/lldb/source/Core/IOHandler.cpp:548 #21 0x00000000019a6c6a in ExecuteIOHandlers () at /usr/src/contrib/llvm-project/lldb/source/Core/Debugger.cpp:903 #22 0x0000000001a70337 in RunCommandInterpreter () at /usr/src/contrib/llvm-project/lldb/source/Interpreter/CommandInterpreter.cpp:2946 #23 0x0000000001d9d812 in RunCommandInterpreter () at /usr/src/contrib/llvm-project/lldb/source/API/SBDebugger.cpp:1169 #24 0x0000000001918be8 in MainLoop () at /usr/src/contrib/llvm-project/lldb/tools/driver/Driver.cpp:675 #25 0x000000000191a114 in main () at /usr/src/contrib/llvm-project/lldb/tools/driver/Driver.cpp:890``` Fix the incorrect error catch by only instantiating an `Error` object if it is necessary. Reviewed By: JDevlieghere Differential Revision: https://reviews.llvm.org/D86355 (cherry picked from commit 1ce07cd)
This change converts the "lessons" section to a comprehensive related-work section. It includes an overview of the large amount of related work in the area, divided into 5 areas: (1) runtime-based approaches (2) security mitigations (3) static analysis (4) program verification and (5) programming languages. This addresses feedback from people that the design document needs to be improved to explain clearly how Checked C relates to other work in the area. This drops some of the detailed comparison with Deputy in favor of a broader overview of related work.
This change only allows the -fcheckedc-extension flag to be used for C programs
in clang. The clang driver will reject the use of -fcheckedc-extension for
other C family languages supported by clang, including C++, Objective
C, OpenCL, and CUDA. This addresses issue #9 in the checked-clang Github repo.
We are currently not modifying clang to support these other languages, which
is why need to disallow using the extension with them.
Testing:
rejected for C++, Objective C, CUDA, and OpenCL.
other similar tests in the test tree.
the documentation to reflect the fact that we have Checked C specific tests
in clang. We need to take the new tests into account when updating to new
versions of the clang/LLVM sources.