NoQ added a comment.
Awesome, we're getting closer to producing our first fix!
I think the most important thing right now is to add a lot of
`SourceLocation::isMacroID()` checks everywhere, so that to *never* try to fix
code that's fully or partially expanded from macros. I suspect that we can do
that "after the fact": just scan the list of emitted fixits for any source
ranges that start or end in macros, and if even one such range is found,
abandon the fix. Might be worth double-checking that the compiler doesn't
already do that automatically for all emitted fixits.
I see that the patch doesn't touch `handleFixableVariable()`. Do we still
attach fixits to the warning, or did we already change it to be attached to a
note associated with the warning? Because fixits with placeholders aren't
allowed on warnings, we should make sure it's attached to note before landing
this patch.
================
Comment at: clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h:42
+ /// Returns the text indicating that the user needs to provide input there:
+ static std::string
+ getUserFillPlaceHolder(const StringRef &HintTextToUser = "placeholder") {
----------------
Do we really want this method in the public interface? If the idea is to let
people the user override it, maybe let's actually allow that by dropping
`static` and adding `virtual`? I think this is actually a good idea, maybe if
somebody uses our analysis outside of the `clang` binary, they may want a
different placeholder.
================
Comment at: clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h:43
+ static std::string
+ getUserFillPlaceHolder(const StringRef &HintTextToUser = "placeholder") {
+ std::string s = std::string("<# ");
----------------
`const StringRef &` seems redundant given that `StringRef` is already a "Ref".
Just pass by value.
================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:408
+ auto BaseIsArrayOrPtrDRE =
+ hasBase(ignoringParenImpCasts(allOf(declRefExpr(), ArrayOrPtr)));
+ auto Target =
----------------
A bit more concise.
================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:637
-static Strategy
-getNaiveStrategy(const llvm::SmallVectorImpl<const VarDecl *> &UnsafeVars) {
----------------
Hmm, did this need to be moved? I don't think you're calling this function from
the new code.
================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:726
+template <typename NodeTy>
+static std::optional<SourceLocation> getPassedLoc(const NodeTy *Node,
+ const SourceManager &SM,
----------------
(https://www.oxfordinternationalenglish.com/passed-vs-past-whats-the-difference/,
looks like "Passed` is always a verb)
================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:768
+ if (const Expr *Ext = CxxNew->getArraySize().value_or(nullptr)) {
+ if (!Ext->HasSideEffects(Ctx))
+ ExtentText =
----------------
Excellent observation!
================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:771
+ getExprTextOr(Ext, SM, LangOpts, StringRef(DefaultExtentTxt));
+ } else if (!CxxNew->isArray())
+ ExtentText = One;
----------------
I wonder how do we end up in such situation. If it's not array new, this means
it's also not a buffer of multiple elements. But I guess the pointer can be
reassigned later. Yeah I think you're right, this is the correct solution.
Maybe let's leave a comment explaining how this could happen?
================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:773-774
+ ExtentText = One;
+ } else if (auto CArrTy = dyn_cast<ConstantArrayType>(
+ Init->IgnoreImpCasts()->getType().getTypePtr())) {
+ // In cases `Init` is of an array type after stripping off implicit casts,
----------------
That's another case where `.getTypePtr()` is unnecessary:
`dyn_cast<T>(QT.getTypePtr())` is equivalent to`QT->getAs<T>()` where `->` is
`QualType`'s overloaded operator and `getAs` is `Type::getAs`.
Additionally, array types are special - see doxygen comments for
`ASTContext::getAsArrayType()`. For some reason you're suppposed to consult
ASTContext when casting them, which I did in the suggested fix. I'm not sure if
it matters in this case when we just want to extract the size.
================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:780-781
+ } else {
+ // In cases `Init` is of the form `&Var` after stripping of implicit
+ // casts, where `&` is the built-in operator, the extent is 1.
+ if (auto AddrOfExpr = dyn_cast<UnaryOperator>(Init->IgnoreImpCasts()))
----------------
```lang=c
int x = 1;
char *ptr = &x; // std::span<char> ptr { &x, 4 };
```
This is valid code. I suspect we want to check types as well, to see that type
sizes match.
Most of the time code like this violates strict aliasing, but `char` is
exceptional, and even if it did violate strict aliasing, people can compile
with `-fno-strict-aliasing` to define away the UB, so we have to respect that.
================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:818
+static FixItList fixVarDeclWithSpan(const VarDecl *D, const ASTContext &Ctx) {
+ const QualType &T = D->getType().getDesugaredType(Ctx);
+ const QualType &SpanEltT = T->getPointeeType();
----------------
`getDesugaredType()` looks redundant, `getPointeeType()` probably already does
that.
================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:856-858
+ assert(DS && "Fixing non-local variables not implemented yet!");
+ assert(DS->getSingleDecl() == VD &&
+ "Fixing non-single declarations not implemented yet!");
----------------
I suspect this code will crash whenever such declarations are encountered, so
this is probably not something we want to commit.
Typically `assert(..., "not implemented yet")` makes sense only when the
crashing codepath is also unreachable because no other facility in the compiler
exercises it.
It probably make sense to `return {}` in these cases: the function is still
responsible for them, we just haven't implemented them yet.
Let's also add a FIXME test to demonstrate that there's no fix yet in such
situations.
================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:892
+ // If we fail to produce Fix-It for the declaration we have to skip the
variable entirely.
+ if (FixItsForVariable[VD].empty()) {
+ FixItsForVariable.erase(VD);
----------------
Interesting, so you don't need to discriminate between situation "fix is
impossible" and situation "fix is not necessary" because the latter is
impossible because the variable type definitely needs fixing? Which is
different from the behavior of `FixableGadget::getFixits()` that returns an
empty optional when the fix is impossible and a non-empty optional with zero
fixes when the fix is not necessary. I guess let's leave a comment at
`fixVariable()` documenting this behavior?
I also suspect that eventually we'll want variable declarations to simply act
as another fixable gadget. In this case we might have to undo this solution and
go back to optionals.
================
Comment at:
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp:55
+ int *p = new int[n];
+ // If the extent expression does not have a constant value, we cannot fill
the extent for users...
+ int *q = new int[n++];
----------------
In the `new int[n]` case it also technically doesn't have a constant value. The
code correctly checks for side effects, the comment should also probably say
something about side effects.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D139737/new/
https://reviews.llvm.org/D139737
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
