From e6d3e0610a31a800514b3b29f8b444bf9fb15f16 Mon Sep 17 00:00:00 2001 From: "Matt McCutchen (Correct Computation)" Date: Mon, 4 Jan 2021 16:36:53 -0500 Subject: [PATCH 1/2] Make the major _3CInterface methods return false if a tool execution fails. It's unclear whether (1) there are still more operations in these methods that can fail and would merit an early return and (2) these early returns could cause problems for callers such as clangd3c that want to recover. But for now, adding these early returns is better than the status quo, and the only caller we support is the 3c tool, which just exits when one of these methods returns false. This exposed a problem with the partial_checked_arr test: it ran 3c on a lit temporary file named partial_checked_arr.c.tmp, and that file extension seems to cause 3c to spew errors. Previously 3c nevertheless exited 0 and the test carried on; now 3c would exit 1. Work around this by using -output-postfix like many of our other tests. This may not be the approach we want in the long term, but we don't have the new approach ready yet. --- clang/lib/3C/3C.cpp | 39 ++++++++++++++++++----------- clang/test/3C/partial_checked_arr.c | 5 ++-- 2 files changed, 28 insertions(+), 16 deletions(-) diff --git a/clang/lib/3C/3C.cpp b/clang/lib/3C/3C.cpp index 5b1b776c8b51..6ddfc121ed57 100644 --- a/clang/lib/3C/3C.cpp +++ b/clang/lib/3C/3C.cpp @@ -246,9 +246,11 @@ bool _3CInterface::buildInitialConstraints() { std::unique_ptr ConstraintTool = newFrontendActionFactoryA< GenericAction>(GlobalProgramInfo); - if (ConstraintTool) - Tool.run(ConstraintTool.get()); - else + if (ConstraintTool) { + int ToolExitCode = Tool.run(ConstraintTool.get()); + if (ToolExitCode != 0) + return false; + } else llvm_unreachable("No action"); if (!GlobalProgramInfo.link()) { @@ -297,9 +299,11 @@ bool _3CInterface::solveConstraints(bool ComputeInterimState) { std::unique_ptr ABInfTool = newFrontendActionFactoryA< GenericAction>( GlobalProgramInfo); - if (ABInfTool) - Tool.run(ABInfTool.get()); - else + if (ABInfTool) { + int ToolExitCode = Tool.run(ABInfTool.get()); + if (ToolExitCode != 0) + return false; + } else llvm_unreachable("No Action"); // Propagate the information from allocator bounds. @@ -310,9 +314,11 @@ bool _3CInterface::solveConstraints(bool ComputeInterimState) { // after constraint solving but before rewriting. std::unique_ptr IMTool = newFrontendActionFactoryA< GenericAction>(GlobalProgramInfo); - if (IMTool) - Tool.run(IMTool.get()); - else + if (IMTool) { + int ToolExitCode = Tool.run(IMTool.get()); + if (ToolExitCode != 0) + return false; + } else llvm_unreachable("No Action"); if (AllTypes) { @@ -364,8 +370,11 @@ bool _3CInterface::writeConvertedFileToDisk(const std::string &FilePath) { newFrontendActionFactoryA>( GlobalProgramInfo); - if (RewriteTool) - Tool.run(RewriteTool.get()); + if (RewriteTool) { + int ToolExitCode = Tool.run(RewriteTool.get()); + if (ToolExitCode != 0) + return false; + } return true; } return false; @@ -380,9 +389,11 @@ bool _3CInterface::writeAllConvertedFilesToDisk() { std::unique_ptr RewriteTool = newFrontendActionFactoryA>( GlobalProgramInfo); - if (RewriteTool) - Tool.run(RewriteTool.get()); - else + if (RewriteTool) { + int ToolExitCode = Tool.run(RewriteTool.get()); + if (ToolExitCode != 0) + return false; + } else llvm_unreachable("No action"); return true; diff --git a/clang/test/3C/partial_checked_arr.c b/clang/test/3C/partial_checked_arr.c index c1c6c54ca73c..a640d6a6dac5 100644 --- a/clang/test/3C/partial_checked_arr.c +++ b/clang/test/3C/partial_checked_arr.c @@ -2,8 +2,9 @@ // RUN: 3c -addcr -alltypes %s | %clang -c -f3c-tool -fcheckedc-extension -x c -o %t1.unused - // RUN: 3c -addcr %s | FileCheck -match-full-lines -check-prefixes="CHECK_NOALL","CHECK" %s // RUN: 3c -addcr %s | %clang -c -f3c-tool -fcheckedc-extension -x c -o %t2.unused - -// RUN: 3c -alltypes %s > %t -// RUN: 3c -alltypes %t | count 0 +// RUN: 3c -alltypes -output-postfix=checked %s +// RUN: 3c -alltypes %S/partial_checked_arr.checked.c -- | count 0 +// RUN: rm %S/partial_checked_arr.checked.c int strcmp(const char *src1 : itype(_Nt_array_ptr), const char *src2 : itype(_Nt_array_ptr)); From cf652f5aaa971202ac76f82f50768a5e534cb358 Mon Sep 17 00:00:00 2001 From: "Matt McCutchen (Correct Computation)" Date: Thu, 31 Dec 2020 14:10:57 -0500 Subject: [PATCH 2/2] Convert root_cause test to use Clang's built-in diagnostic verifier. This means we now verify the line numbers of the diagnostics for free, which would have been a pain with the existing CHECK-DAG system. - Make the 3c tool accept a -verify option analogous to that of `clang -cc1` to verify diagnostics against `// expected-warning` comments. Currently, this applies only to the rewriting phase. - Fix a few mistakes in the expected diagnostics in the root_cause test. --- clang/include/clang/3C/3C.h | 5 +++++ clang/lib/3C/3C.cpp | 32 ++++++++++++++++++++++++----- clang/test/3C/root_cause.c | 36 +++++++++++---------------------- clang/tools/3c/3CStandalone.cpp | 15 ++++++++++++++ 4 files changed, 59 insertions(+), 29 deletions(-) diff --git a/clang/include/clang/3C/3C.h b/clang/include/clang/3C/3C.h index ce98c4f915fe..ff004e5df6de 100644 --- a/clang/include/clang/3C/3C.h +++ b/clang/include/clang/3C/3C.h @@ -65,6 +65,11 @@ struct _3COptions { bool RemoveItypes; bool ForceItypes; #endif + + // Currently applies only to the rewriting phase (because it is the only phase + // that generates diagnostics, except for the declaration merging diagnostics + // that are currently fatal) and uses the default "expected" prefix. + bool VerifyDiagnosticOutput; }; // The main interface exposed by the 3C to interact with the tool. diff --git a/clang/lib/3C/3C.cpp b/clang/lib/3C/3C.cpp index 6ddfc121ed57..a2162b162438 100644 --- a/clang/lib/3C/3C.cpp +++ b/clang/lib/3C/3C.cpp @@ -54,6 +54,7 @@ bool DisableCCTypeChecker; bool WarnRootCause; bool WarnAllRootCause; std::set FilePaths; +bool VerifyDiagnosticOutput; #ifdef FIVE_C bool RemoveItypes; @@ -95,19 +96,39 @@ class RewriteAction : public ASTFrontendAction { template std::unique_ptr -newFrontendActionFactoryA(ProgramInfo &I) { +newFrontendActionFactoryA(ProgramInfo &I, bool VerifyTheseDiagnostics = false) { class ArgFrontendActionFactory : public FrontendActionFactory { public: - explicit ArgFrontendActionFactory(ProgramInfo &I) : Info(I) {} + explicit ArgFrontendActionFactory(ProgramInfo &I, + bool VerifyTheseDiagnostics) + : Info(I), VerifyTheseDiagnostics(VerifyTheseDiagnostics) {} FrontendAction *create() override { return new T(Info); } + bool runInvocation(std::shared_ptr Invocation, + FileManager *Files, + std::shared_ptr PCHContainerOps, + DiagnosticConsumer *DiagConsumer) override { + if (VerifyTheseDiagnostics) { + // Mirroring the logic of clang::ParseDiagnosticArgs in + // clang/lib/Frontend/CompilerInvocation.cpp. In particular, note that + // VerifyPrefixes is assumed to be sorted, in case we add more in the + // future. + DiagnosticOptions &DiagOpts = Invocation->getDiagnosticOpts(); + DiagOpts.VerifyDiagnostics = true; + DiagOpts.VerifyPrefixes.push_back("expected"); + } + return FrontendActionFactory::runInvocation( + Invocation, Files, PCHContainerOps, DiagConsumer); + } + private: ProgramInfo &Info; + bool VerifyTheseDiagnostics; }; return std::unique_ptr( - new ArgFrontendActionFactory(I)); + new ArgFrontendActionFactory(I, VerifyTheseDiagnostics)); } ArgumentsAdjuster getIgnoreCheckedPointerAdjuster() { @@ -192,6 +213,7 @@ _3CInterface::_3CInterface(const struct _3COptions &CCopt, AllocatorFunctions = CCopt.AllocatorFunctions; WarnRootCause = CCopt.WarnRootCause || CCopt.WarnAllRootCause; WarnAllRootCause = CCopt.WarnAllRootCause; + VerifyDiagnosticOutput = CCopt.VerifyDiagnosticOutput; #ifdef FIVE_C RemoveItypes = CCopt.RemoveItypes; @@ -368,7 +390,7 @@ bool _3CInterface::writeConvertedFileToDisk(const std::string &FilePath) { Tool.appendArgumentsAdjuster(getIgnoreCheckedPointerAdjuster()); std::unique_ptr RewriteTool = newFrontendActionFactoryA>( - GlobalProgramInfo); + GlobalProgramInfo, VerifyDiagnosticOutput); if (RewriteTool) { int ToolExitCode = Tool.run(RewriteTool.get()); @@ -388,7 +410,7 @@ bool _3CInterface::writeAllConvertedFilesToDisk() { // Rewrite the input files std::unique_ptr RewriteTool = newFrontendActionFactoryA>( - GlobalProgramInfo); + GlobalProgramInfo, VerifyDiagnosticOutput); if (RewriteTool) { int ToolExitCode = Tool.run(RewriteTool.get()); if (ToolExitCode != 0) diff --git a/clang/test/3C/root_cause.c b/clang/test/3C/root_cause.c index a9cb3323b082..5d0ee6c2e320 100644 --- a/clang/test/3C/root_cause.c +++ b/clang/test/3C/root_cause.c @@ -1,55 +1,43 @@ -// RUN: 3c -extra-arg="-Wno-everything" -alltypes -warn-root-cause %s 2>&1 >%t.unused | FileCheck %s +// RUN: 3c -extra-arg="-Wno-everything" -verify -alltypes -warn-root-cause %s // This test is unusual in that it checks for the errors in the code #include extern _Itype_for_any(T) void *malloc(size_t size) : itype(_Array_ptr) byte_count(size); -void *x; -// CHECK-DAG: Default void* type +void *x; // expected-warning {{Default void* type}} void test0() { int *a; char *b; - a = b; - // CHECK-DAG: Cast from int * to char * + a = b; // expected-warning {{Cast from char * to int *}} int *c; - (char*) c; - // CHECK-DAG: Cast from int * to char * + (char*) c; // expected-warning {{Cast from int * to char *}} int *e; char *f; - f = (char*) e; - // CHECK-DAG: Cast from char * to int * + f = (char*) e; // expected-warning {{Cast from int * to char *}} } void test1() { int a; int *b; - b = malloc(sizeof(int)); + b = malloc(sizeof(int)); // expected-warning {{Bad pointer type solution}} b[0] = 1; union u { - int *a; - // CHECK-DAG: Union or external struct field encountered - int *b; - // CHECK-DAG: Union or external struct field encountered + int *a; // expected-warning {{Union or external struct field encountered}} + int *b; // expected-warning {{Union or external struct field encountered}} }; void (*c)(void); - c++; - // CHECK-DAG: Pointer arithmetic performed on a function pointer + c++; // expected-warning {{Pointer arithmetic performed on a function pointer}} - int *d = malloc(1); - // CHECK-DAG: Unsafe call to allocator function + int *d = malloc(1); // expected-warning {{Unsafe call to allocator function}} } -extern int *glob; -// CHECK-DAG: External global variable glob has no definition +extern int *glob; // expected-warning {{External global variable glob has no definition}} -void (*f)(void *); -// CHECK-DAG: Default void* type - -// CHECK-DAG: 11 warnings generated. +void (*f)(void *); // expected-warning {{Default void* type}} diff --git a/clang/tools/3c/3CStandalone.cpp b/clang/tools/3c/3CStandalone.cpp index 587b3a737eb6..4d330de81584 100644 --- a/clang/tools/3c/3CStandalone.cpp +++ b/clang/tools/3c/3CStandalone.cpp @@ -121,6 +121,20 @@ static cl::opt "even those unlikely to be interesting."), cl::init(false), cl::cat(_3CCategory)); +// https://clang.llvm.org/doxygen/classclang_1_1VerifyDiagnosticConsumer.html#details +// +// Analogous to the -verify option of `clang -cc1`, but currently applies only +// to the rewriting phase (because it is the only phase that generates +// diagnostics, except for the declaration merging diagnostics that are +// currently fatal). No checking of diagnostics from the other phases is +// performed. We cannot simply have the caller pass `-extra-arg=-Xclang +// -extra-arg=-verify` because that would expect each phase to produce the same +// set of diagnostics. +static cl::opt OptVerifyDiagnosticOutput( + "verify", + cl::desc("Verify diagnostic output (for automated testing of 3C)."), + cl::init(false), cl::cat(_3CCategory), cl::Hidden); + #ifdef FIVE_C static cl::opt OptRemoveItypes( "remove-itypes", @@ -161,6 +175,7 @@ int main(int argc, const char **argv) { CcOptions.DisableCCTypeChecker = OptDiableCCTypeChecker; CcOptions.WarnRootCause = OptWarnRootCause; CcOptions.WarnAllRootCause = OptWarnAllRootCause; + CcOptions.VerifyDiagnosticOutput = OptVerifyDiagnosticOutput; #ifdef FIVE_C CcOptions.RemoveItypes = OptRemoveItypes;