tahonermann wrote:
I don't think the currently proposed change is right for a few reasons.
The proposed change silently marks the declaration as invalid and performs an
early return. I don't think this should be a silent behavior. In most, if not
all, of the existing early return cases, a diagnostic is issued (perhaps via
another called function) and/or a recovery expression is created and used as
the initializer.
I think this prior statement of mine is incorrect:
> On the one hand, the assignment to `Init` looks to me like it must produce a
> non-null result due to the prior check to `Result.isInvalid()`.
It is possible for an `ExprResult` value to be valid and still hold a null
pointer value; `InitializationSequence::Perform()` explicitly returns a (valid)
null pointer in at least one case:
```
clang/lib/Sema/SemaInit.cpp:
8612 ExprResult InitializationSequence::Perform(Sema &S,
8613 const InitializedEntity
&Entity,
8614 const InitializationKind &Kind,
8615 MultiExprArg Args,
8616 QualType *ResultType) {
....
8699 // No steps means no initialization.
8700 if (Steps.empty())
8701 return ExprResult((Expr *)nullptr);
....
9539 }
```
This indicates that, absent a more complicated analysis that proves that
`Sema::AddInitializerToDecl()` will never call
`InitializationSequence::Perform()` in a context that will lead to a null
result, the static analysis tool is right to report a possible null dereference.
I suggest adding an assert here:
```
clang/lib/Sema/SemaDecl.cpp:
13660 if (!VDecl->isInvalidDecl()) {
.....
13690 ExprResult Result = InitSeq.Perform(*this, Entity, Kind, Args, &DclT);
13691 if (Result.isInvalid()) {
.....
13707 return;
13708 }
13709
13710 Init = Result.getAs<Expr>();
.....
13726 }
+
+ assert(Init && "Should have a valid initializer at this point);
+
13728 // Check for self-references within variable initializers.
13729 // Variables declared within a function/method body (except for
references)
13730 // are handled by a dataflow analysis.
13731 // This is undefined behavior in C++, but valid in C.
13732 if (getLangOpts().CPlusPlus)
13733 if (!VDecl->hasLocalStorage() || VDecl->getType()->isRecordType() ||
13734 VDecl->getType()->isReferenceType())
13735 CheckSelfReference(*this, RealDecl, Init, DirectInit);
```
Relatedly, I think the two checks for `Init` being non-null are probably wrong
in these cases:
```
13506 // WebAssembly tables can't be used to initialise a variable.
13507 if (Init && !Init->getType().isNull() &&
13508 Init->getType()->isWebAssemblyTableType()) {
13509 Diag(Init->getExprLoc(), diag::err_wasm_table_art) << 0;
13510 VDecl->setInvalidDecl();
13511 return;
13512 }
.....
13715 if (Init && !Init->getType().isNull() &&
13716 !Init->getType()->isDependentType() &&
!VDeclType->isDependentType() &&
13717 Context.getAsIncompleteArrayType(VDeclType) &&
13718 Context.getAsIncompleteArrayType(Init->getType())) {
13719 // Bail out if it is not possible to deduce array size from the
13720 // initializer.
13721 Diag(VDecl->getLocation(), diag::err_typecheck_decl_incomplete_type)
13722 << VDeclType;
13723 VDecl->setInvalidDecl();
13724 return;
13725 }
```
I think the null check at line 13507 can just be removed; previous uses of
`Init` appear to expect a non-null value. This looks like a case of a reflexive
defensive check (added in https://reviews.llvm.org/D139010).
The null check at line 13715 also looks like it was added as a reflexive
defensive check (added in https://reviews.llvm.org/D158615 for
https://github.com/llvm/llvm-project/issues/37257)
Adding the assert and removing the defensive null checks would eliminate the
evidence the static analysis tool is using to justify its report of a potential
null dereference.
If Clang tests pass with these changes (using an asserts enabled build of
course!), then I think we would be ok to proceed. If we then see failed
assertions or null pointer dereferences at some point, we'll be able to
identify test cases to add.
https://github.com/llvm/llvm-project/pull/94368
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
