diff --git a/nullability/inference/collect_evidence_test.cc b/nullability/inference/collect_evidence_test.cc index 6e2fcae46..45f2ee54e 100644 --- a/nullability/inference/collect_evidence_test.cc +++ b/nullability/inference/collect_evidence_test.cc @@ -80,10 +80,34 @@ using ::testing::SizeIs; using ::testing::UnorderedElementsAre; constexpr llvm::StringRef CheckMacroDefinitions = R"cc( - // Bodies must reference the first param so that args are in the AST, but - // otherwise don't matter. -#define CHECK(X) (X) -#define CHECK_NE(A, B) (A, B) + // A simplified version of Abseil's CHECK macro. We want just enough control + // flow that this should affect flow conditions. +#define CHECK(X) \ + if (!X) __builtin_abort(); + + // A simplified version of Abseil's CHECK_NE (enough to affect flow + // conditions). + struct string; + namespace absl::something { + template + const T& GetReferenceableValue(const T& X) { + return X; + } + int GetReferenceableValue(int t) { return t; } + + template + string* Check_NEImpl(const T1&, const T2&, const char*) { + return nullptr; + } + } // namespace absl::something + +#define CHECK_OP(name, op, a, b) \ + while (string* result = ::absl::something::name##Impl( \ + ::absl::something::GetReferenceableValue(a), \ + ::absl::something::GetReferenceableValue(b), "msg")) \ + __builtin_abort(); + +#define CHECK_NE(a, b) CHECK_OP(Check_NE, !=, (a), (b)) )cc"; constexpr llvm::StringRef GtestMacroDefinitions = R"cc( @@ -813,6 +837,22 @@ TEST(SmartPointerCollectEvidenceFromDefinitionTest, CheckMacro) { evidence(paramSlot(2), Evidence::ABORT_IF_NULL))); } +TEST(CollectEvidenceFromDefinitionTest, CheckMacroPostState) { + static constexpr llvm::StringRef BaseSrc = R"cc( + Nullable getOrNull(int x); + int* target() { + int* P = getOrNull(0); + CHECK(P); + return P; + } + )cc"; + EXPECT_THAT( + collectFromTargetFuncDefinition((CheckMacroDefinitions + BaseSrc).str()), + UnorderedElementsAre(evidence(SLOT_RETURN_TYPE, Evidence::NONNULL_RETURN), + evidence(Slot(0), Evidence::ASSIGNED_FROM_NULLABLE, + localVarNamed("P")))); +} + // This is a crash repro; see b/370737278. TEST(SmartPointerCollectEvidenceFromDefinitionTest, CheckMacroSmartPointerToPointer) { @@ -858,12 +898,12 @@ TEST(CollectEvidenceFromDefinitionTest, CheckNEMacro) { )cc"; EXPECT_THAT( collectFromTargetFuncDefinition((CheckMacroDefinitions + BaseSrc).str()), - UnorderedElementsAre(evidence(paramSlot(0), Evidence::ABORT_IF_NULL), - evidence(paramSlot(1), Evidence::ABORT_IF_NULL), - evidence(paramSlot(2), Evidence::ABORT_IF_NULL), - evidence(paramSlot(3), Evidence::ABORT_IF_NULL), - evidence(Slot(0), Evidence::ASSIGNED_FROM_NULLABLE, - localVarNamed("A")))); + IsSupersetOf({(evidence(paramSlot(0), Evidence::ABORT_IF_NULL), + evidence(paramSlot(1), Evidence::ABORT_IF_NULL), + evidence(paramSlot(2), Evidence::ABORT_IF_NULL), + evidence(paramSlot(3), Evidence::ABORT_IF_NULL), + evidence(Slot(0), Evidence::ASSIGNED_FROM_NULLABLE, + localVarNamed("A")))})); } TEST(SmartPointerCollectEvidenceFromDefinitionTest, CheckNEMacro) { @@ -880,15 +920,31 @@ TEST(SmartPointerCollectEvidenceFromDefinitionTest, CheckNEMacro) { )cc"; EXPECT_THAT( collectFromTargetFuncDefinition((CheckMacroDefinitions + BaseSrc).str()), - UnorderedElementsAre(evidence(paramSlot(0), Evidence::ABORT_IF_NULL), - evidence(paramSlot(1), Evidence::ABORT_IF_NULL), - evidence(paramSlot(3), Evidence::ABORT_IF_NULL))); + IsSupersetOf({(evidence(paramSlot(0), Evidence::ABORT_IF_NULL), + evidence(paramSlot(1), Evidence::ABORT_IF_NULL), + evidence(paramSlot(3), Evidence::ABORT_IF_NULL))})); +} + +TEST(CollectEvidenceFromDefinitionTest, CheckNEMacroPostState) { + static constexpr llvm::StringRef BaseSrc = R"cc( + Nullable getOrNull(int x); + int* target() { + int* P = getOrNull(0); + CHECK_NE(P, nullptr); + return P; + } + )cc"; + EXPECT_THAT( + collectFromTargetFuncDefinition((CheckMacroDefinitions + BaseSrc).str()), + IsSupersetOf({(evidence(SLOT_RETURN_TYPE, Evidence::NONNULL_RETURN), + evidence(Slot(0), Evidence::ASSIGNED_FROM_NULLABLE, + localVarNamed("P")))})); } TEST(CollectEvidenceFromDefinitionTest, NullableArgPassed) { static constexpr llvm::StringRef Src = R"cc( - void callee(int *Q); - void target(Nullable P) { callee(P); } + void callee(int* Q); + void target(Nullable P) { callee(P); } )cc"; EXPECT_THAT(collectFromTargetFuncDefinition(Src), Contains(evidence(paramSlot(0), Evidence::NULLABLE_ARGUMENT, @@ -4418,8 +4474,8 @@ TEST(CollectEvidenceFromDefinitionTest, PragmaLocalTopLevelPointer) { TEST(CollectEvidenceFromDefinitionTest, PragmaAndMacroReplace) { static constexpr llvm::StringRef BaseSrc = R"cc( #pragma nullability file_default nonnull - int* target(NullabilityUnknown P) { - CHECK(P); + int* target(NullabilityUnknown P, NullabilityUnknown Q) { + CHECK(Q); return P; } )cc"; @@ -4427,7 +4483,7 @@ TEST(CollectEvidenceFromDefinitionTest, PragmaAndMacroReplace) { collectFromTargetFuncDefinition((CheckMacroDefinitions + BaseSrc).str()), UnorderedElementsAre( evidence(paramSlot(0), Evidence::ASSIGNED_TO_NONNULL), - evidence(paramSlot(0), Evidence::ABORT_IF_NULL))); + evidence(paramSlot(1), Evidence::ABORT_IF_NULL))); } TEST(CollectEvidenceFromDefinitionTest, diff --git a/nullability/value_transferer.cc b/nullability/value_transferer.cc index 3b03c9f11..989734c29 100644 --- a/nullability/value_transferer.cc +++ b/nullability/value_transferer.cc @@ -910,6 +910,18 @@ static bool isDeclaredInAbseilOrUtil(const Decl& D) { (NS->getName() == "absl" || NS->getName() == "util"); } +// Models our macro replacement argument-capture functions (supporting +// inference). +static void modelArgCaptureAbortIfPassThrough(const CallExpr& CE, + Environment& Env) { + assert(CE.isGLValue()); + assert(CE.getNumArgs() >= 1); + assert(CE.getArg(0) != nullptr); + // Pass through the storage location of the first argument to the result. + if (StorageLocation* Loc = Env.getStorageLocation(*CE.getArg(0))) + Env.setStorageLocation(CE, *Loc); +} + // Models the `GetReferenceableValue` functions used in Abseil logging and // elsewhere. static void modelGetReferenceableValue(const CallExpr& CE, Environment& Env) { @@ -1019,6 +1031,11 @@ static void transferCallExpr(const CallExpr* absl_nonnull CE, modelCheckNE(*CE, State.Env); return; } + if (FunII->isStr(ArgCaptureAbortIfFalse) || + FunII->isStr(ArgCaptureAbortIfEqual)) { + modelArgCaptureAbortIfPassThrough(*CE, State.Env); + return; + } } } @@ -1071,11 +1088,8 @@ static void transferCallExpr(const CallExpr* absl_nonnull CE, if (CE->isCallToStdMove() || FuncDecl == nullptr) return; - // Don't treat parameters of our macro replacement argument-capture functions - // or of absl::StatusOr::value_or as output parameters. - if (FunII && (FunII->isStr(ArgCaptureAbortIfFalse) || - FunII->isStr(ArgCaptureAbortIfEqual) || - (FunII->isStr("value_or") && isMethodOfAbslStatusOr(FuncDecl)))) + // Don't treat parameters of absl::StatusOr::value_or as output parameters. + if (FunII && ((FunII->isStr("value_or") && isMethodOfAbslStatusOr(FuncDecl)))) return; // Make output parameters (with unknown nullability) initialized to unknown. for (ParamAndArgIterator Iter(*FuncDecl, *CE); Iter; ++Iter)