[Consumed] Refactor handleCall to take function argument list. NFC.

[comex]

Summary

Currently, handleCall takes a CallExpr as an argument and retrieves the arguments from there. This changes it to take the argument list as a separate argument of type ArrayRef.

The main motivation that I want VisitCXXConstructExpr to also be able to invoke handleCall, even though CXXConstructExpr is not a subclass of CallExpr. But actually adding that will wait for a future patch.

However, this also fixes a minor bug in VisitCXXOperatorCallExpr:

if (const auto *MCall = dyn_cast<CXXMemberCallExpr>(Call))
  handleCall(MCall, MCall->getImplicitObjectArgument(), FunDecl);
else
  handleCall(Call, Call->getArg(0), FunDecl);

Call here is a CXXOperatorCallExpr, and CXXMemberCallExpr is a sibling class, so the cast will never succeed. CXXMemberCallExprs go through a separate visitor, VisitCXXMemberCallExpr.

On the other hand, this logic may be intended to reflect the fact that C++ operators can be declared as either methods or free functions. The correct way to differentiate these is shown at the beginning of handleCall:

unsigned Offset = 0;
if (isa<CXXOperatorCallExpr>(Call) && isa<CXXMethodDecl>(FunD))
  Offset = 1;  // first argument is 'this'

For CXXOperatorCallExprs, if the decl is a CXXMethodDecl, the first argument is this; otherwise there is no this.

Going back to the other first: currently, since the dyn_cast always fails, it always passes Call->getArg(0) as ObjArg (i.e. the expression representing this), even for operators declared as free functions. However, this is harmless, because ObjArg is only used if the function is marked as one of set_typestate or test_typestate, or callable_when, yet those attributes are only allowed on methods. Call->getArg(0) will crash if there are zero arguments, but the only kind of non-method operator function allowed to have zero arguments is a user-defined literal, and those do not produce CXXOperatorCallExprs.

The bug could be fixed by changing the first snippet to check for CXXMethodDecl like the second one, but this refactor makes things cleaner by only having to check in one place.

[dblaikie]

lib/Analysis/Consumed.cpp

752

probably use "Call->arguments()" here & in the other places that need an range of the arguments? (& if that can be an ArrayRef - updating CallExpr's arguments() and the arg_range and const_arg_range to use/be ArrayRef would be good)

[comex]

Ugh, it looks like getArgs() is a massive strict aliasing violation. And it's used in enough different places in Clang that fixing the violation may require extensive refactoring... I've reported the issue as https://bugs.llvm.org/show_bug.cgi?id=43344, but I don't really want to fix it :)

However, I can at least update this patch to avoid using getArgs().

[dblaikie]

In D67647#1673363, @comex wrote:

Ugh, it looks like getArgs() is a massive strict aliasing violation. And it's used in enough different places in Clang that fixing the violation may require extensive refactoring... I've reported the issue as https://bugs.llvm.org/show_bug.cgi?id=43344, but I don't really want to fix it :)

However, I can at least update this patch to avoid using getArgs().

Yeah, existing technical debt - I don't think your situation is making that any worse or better, so no worries about it.

[comex]

Here's a new version of the patch that uses iterator ranges instead of ArrayRef, to avoid adding new uses of CallExpr::getArgs() in case it has to be removed in the future due to the strict aliasing issue.

To support this, the following additional changes are included:

• Fix CastIterator's use of iterator_adaptor_base so that operator[] isn't broken.

• Add an operator[] implementation to iterator_range.

• Improve the existing llvm::drop_begin helper to assert that it doesn't go out of bounds. (The implementation would be a lot less ugly if we could use C++17 features; oh well.)

[dblaikie]

In D67647#1674745, @comex wrote:

Here's a new version of the patch that uses iterator ranges instead of ArrayRef, to avoid adding new uses of CallExpr::getArgs() in case it has to be removed in the future due to the strict aliasing issue.

Not related to the strict aliasing issue - just seemed like it'd be tidier than reconstructing ranges manually from getArgs and getNumArgs.

To support this, the following additional changes are included:

I wasn't expecting it to involve all this work - but instead to change "arguments()" to return ArrayRef. Though perhaps you didn't go that route to allow future fixes to the strict aliasing (by having an adapting iterator - or perhaps arguments() already uses such an iterator that doesn't hit the strict aliasing issue?) needing to do even more cleaunp work?

• Fix CastIterator's use of iterator_adaptor_base so that operator[] isn't broken.

• Add an operator[] implementation to iterator_range.

I'm not sure that's appropriate - not all types with begin() and end() (even those with random access iterators) would have op[], so it doesn't seem appropriate to add it/depend on it?

• Improve the existing llvm::drop_begin helper to assert that it doesn't go out of bounds. (The implementation would be a lot less ugly if we could use C++17 features; oh well.)

[comex]

In D67647#1674773, @dblaikie wrote:

I wasn't expecting it to involve all this work - but instead to change "arguments()" to return ArrayRef. Though perhaps you didn't go that route to allow future fixes to the strict aliasing (by having an adapting iterator - or perhaps arguments() already uses such an iterator that doesn't hit the strict aliasing issue?) needing to do even more cleaunp work?

Yeah, arguments() returns an adapting iterator.

• Add an operator[] implementation to iterator_range.

I'm not sure that's appropriate - not all types with begin() and end() (even those with random access iterators) would have op[], so it doesn't seem appropriate to add it/depend on it?

Random access iterators are supposed to have operator[], according to:
http://www.cplusplus.com/reference/iterator/RandomAccessIterator/

But the use of a template ensures that it doesn't cause an error with types that don't have operator[], whether they are marked as random-access or not.

I checked the spec for "real" C++20 ranges... it seems that random access ranges don't always have operator[], but "view_interface" (which is a type of range) does:
https://eel.is/c++draft/view.interface

Anyway, I don't think it hurts to have it on this little imitation.

[dblaikie]

In D67647#1674781, @comex wrote:

In D67647#1674773, @dblaikie wrote:

I wasn't expecting it to involve all this work - but instead to change "arguments()" to return ArrayRef. Though perhaps you didn't go that route to allow future fixes to the strict aliasing (by having an adapting iterator - or perhaps arguments() already uses such an iterator that doesn't hit the strict aliasing issue?) needing to do even more cleaunp work?

Yeah, arguments() returns an adapting iterator.

Right - I was suggesting that could be changed. Would it be OK to you to change arguments() to return ArrayRef? Or would you rather avoid that to avoid baking in more dependence on that alias violation?

• Add an operator[] implementation to iterator_range.

I'm not sure that's appropriate - not all types with begin() and end() (even those with random access iterators) would have op[], so it doesn't seem appropriate to add it/depend on it?

Random access iterators are supposed to have operator[], according to:
http://www.cplusplus.com/reference/iterator/RandomAccessIterator/

Yep, I'm with you there

But the use of a template ensures that it doesn't cause an error with types that don't have operator[], whether they are marked as random-access or not.

I checked the spec for "real" C++20 ranges... it seems that random access ranges don't always have operator[],

Yeah, that's where I've got different feelings - a general purpose utility to take any range (thing with begin/end) over random access iterators and do indexed lookup, I'd be fine with that (implemented as "r.begin()[n]") but adding op[] to the range itself then leads code to depend on that feature which isn't a general purpose range feature - so changing this iterator_range to some other range implementation might break that code - it'd be nice to keep iterator_range's interface minimal to allow implementation flexibility in the APIs that expose it.

Same reason I've pushed back on adding things like "empty()" or "size()", etc. Which are fine (& have been added in STLExtras) as free functions.

but "view_interface" (which is a type of range) does:
https://eel.is/c++draft/view.interface

Fair - guess they've gone in a slightly different direction than we have in LLVM for now. I don't feel super strongly about it - but a bit.

Anyway, I don't think it hurts to have it on this little imitation.

[comex]

In D67647#1674795, @dblaikie wrote:

Right - I was suggesting that could be changed. Would it be OK to you to change arguments() to return ArrayRef? Or would you rather avoid that to avoid baking in more dependence on that alias violation?

I'd rather avoid that for the reason you said.

Same reason I've pushed back on adding things like "empty()" or "size()", etc. Which are fine (& have been added in STLExtras) as free functions.

Okay, I'll just do that then. Added as index(Range, N), matching range-v3 (though not the actual C++20 draft).

[dblaikie]

Sounds good - thanks!

llvm/include/llvm/ADT/iterator_range.h

27–33 ↗

(On Diff #220926)

Is this how the C++ standard is doing things these days? Or is it usually done with variable templates?

[comex]

llvm/include/llvm/ADT/iterator_range.h

27–33 ↗

(On Diff #220926)

"Type trait"-like things have gone through three different evolutions:

• C++11 struct templates: std::is_integral<T>::value
• C++17 variable templates: std::is_integral_v<T>
• C++20 concepts: std::integral<T>

In fact, the C++20 draft has a random_access_iterator<T> concept, though there is no is_random_access_iterator in prior versions.

However, with LLVM still built as C++11, this helper has to be a struct template or a constexpr function. It seemed a bit simpler to use a constexpr function, since template specialization wasn't needed.

[dblaikie]

llvm/include/llvm/ADT/iterator_range.h

27–33 ↗

(On Diff #220926)

Fair enough - thanks for the details :)

[comex]

So, I landed this patch but had to revert it as it broke the build on MSVC (and only MSVC). That was almost a month ago, but I haven't gotten back around to this until now – sorry :|

In this updated patch, I switched is_random_iterator from a constexpr function:

template <typename T>
constexpr bool is_random_iterator() {
  return std::is_same<
    typename std::iterator_traits<T>::iterator_category,
    std::random_access_iterator_tag>::value;
}

to a type alias:

template <typename T>
using is_random_iterator =
  std::is_same<typename std::iterator_traits<T>::iterator_category,
               std::random_access_iterator_tag>;

and changed the uses accordingly. For some reason, MSVC thought the two overloads of drop_begin, one with enable_if<!is_random_iterator<T>()> and one with enable_if<is_random_iterator<T>()>, were duplicate definitions. But with is_random_iterator changed to a type alias, MSVC is fine with them. GCC and Clang think both versions are valid, so I think it's just an MSVC bug.

Simplified example for reference: https://gcc.godbolt.org/z/niXCy4

[dblaikie]

In D67647#1713974, @comex wrote:

So, I landed this patch but had to revert it as it broke the build on MSVC (and only MSVC). That was almost a month ago, but I haven't gotten back around to this until now – sorry :|

In this updated patch, I switched is_random_iterator from a constexpr function:

template <typename T>
constexpr bool is_random_iterator() {
  return std::is_same<
    typename std::iterator_traits<T>::iterator_category,
    std::random_access_iterator_tag>::value;
}

to a type alias:

template <typename T>
using is_random_iterator =
  std::is_same<typename std::iterator_traits<T>::iterator_category,
               std::random_access_iterator_tag>;

and changed the uses accordingly. For some reason, MSVC thought the two overloads of drop_begin, one with enable_if<!is_random_iterator<T>()> and one with enable_if<is_random_iterator<T>()>, were duplicate definitions. But with is_random_iterator changed to a type alias, MSVC is fine with them. GCC and Clang think both versions are valid, so I think it's just an MSVC bug.

Simplified example for reference: https://gcc.godbolt.org/z/niXCy4

Sounds good to me. Maybe leave a comment explaining why a constexpr function was not used?

lib/Analysis/Consumed.cpp

class ConsumedStmtVisitor : public ConstStmtVisitor<ConsumedStmtVisitor> {
   ConsumedState getInfo(const Expr *From);
   void setInfo(const Expr *To, ConsumedState NS);
   void propagateReturnType(const Expr *Call, const FunctionDecl *Fun);
 
 public:
   void checkCallability(const PropagationInfo &PInfo,
                         const FunctionDecl *FunDecl,
                         SourceLocation BlameLoc);
-  bool handleCall(const CallExpr *Call, const Expr *ObjArg,
-                  const FunctionDecl *FunD);
+  bool handleCall(const Expr *Call, const Expr *ObjArg,
+                  ArrayRef<const Expr *> Args, const FunctionDecl *FunD);
 
   void VisitBinaryOperator(const BinaryOperator *BinOp);
   void VisitCallExpr(const CallExpr *Call);
   void VisitCastExpr(const CastExpr *Cast);
   void VisitCXXBindTemporaryExpr(const CXXBindTemporaryExpr *Temp);
   void VisitCXXConstructExpr(const CXXConstructExpr *Call);
   void VisitCXXMemberCallExpr(const CXXMemberCallExpr *Call);
   void VisitCXXOperatorCallExpr(const CXXOperatorCallExpr *Call);
if (PInfo.isVar()) {
     Analyzer.WarningsHandler.warnUseOfTempInInvalidState(
       FunDecl->getNameAsString(), stateToString(TmpState), BlameLoc);
   }
 }
 
 // Factors out common behavior for function, method, and operator calls.
 // Check parameters and set parameter state if necessary.
 // Returns true if the state of ObjArg is set, or false otherwise.
-bool ConsumedStmtVisitor::handleCall(const CallExpr *Call, const Expr *ObjArg,
+bool ConsumedStmtVisitor::handleCall(const Expr *Call,
+                                     const Expr *ObjArg,
+                                     ArrayRef<const Expr *> Args,
                                      const FunctionDecl *FunD) {
-  unsigned Offset = 0;
-  if (isa<CXXOperatorCallExpr>(Call) && isa<CXXMethodDecl>(FunD))
-    Offset = 1;  // first argument is 'this'
-
   // check explicit parameters
-  for (unsigned Index = Offset; Index < Call->getNumArgs(); ++Index) {
+  unsigned Index = 0;
+  for (const Expr *Arg : Args) {
     // Skip variable argument lists.
-    if (Index - Offset >= FunD->getNumParams())
+    if (Index >= FunD->getNumParams())
       break;
 
-    const ParmVarDecl *Param = FunD->getParamDecl(Index - Offset);
+    const ParmVarDecl *Param = FunD->getParamDecl(Index++);
     QualType ParamType = Param->getType();
 
-    InfoEntry Entry = findInfo(Call->getArg(Index));
+    InfoEntry Entry = findInfo(Arg);
 
     if (Entry == PropagationMap.end() || Entry->second.isTest())
       continue;
     PropagationInfo PInfo = Entry->second;
 
     // Check that the parameter is in the correct state.
     if (ParamTypestateAttr *PTA = Param->getAttr<ParamTypestateAttr>()) {
       ConsumedState ParamState = PInfo.getAsState(StateMap);
       ConsumedState ExpectedState = mapParamTypestateAttrState(PTA);
 
       if (ParamState != ExpectedState)
         Analyzer.WarningsHandler.warnParamTypestateMismatch(
-          Call->getArg(Index)->getExprLoc(),
+          Arg->getExprLoc(),
           stateToString(ExpectedState), stateToString(ParamState));
     }
 
     if (!(Entry->second.isVar() || Entry->second.isTmp()))
       continue;
 
     // Adjust state on the caller side.
     if (isRValueRef(ParamType))
void ConsumedStmtVisitor::VisitCallExpr(const CallExpr *Call) {
 
   // Special case for the std::move function.
   // TODO: Make this more specific. (Deferred)
   if (Call->isCallToStdMove()) {
     copyInfo(Call->getArg(0), Call, CS_Consumed);
     return;
   }
 
-  handleCall(Call, nullptr, FunDecl);
+  handleCall(Call, nullptr,
+             llvm::makeArrayRef(Call->getArgs(), Call->getNumArgs()),
+             FunDecl);
   propagateReturnType(Call, FunDecl);
 }
 
 void ConsumedStmtVisitor::VisitCastExpr(const CastExpr *Cast) {
   forwardInfo(Cast->getSubExpr(), Cast);
 }
 
 void ConsumedStmtVisitor::VisitCXXBindTemporaryExpr(

 }
 
 void ConsumedStmtVisitor::VisitCXXMemberCallExpr(
     const CXXMemberCallExpr *Call) {
   CXXMethodDecl* MD = Call->getMethodDecl();
   if (!MD)
     return;
 
-  handleCall(Call, Call->getImplicitObjectArgument(), MD);
+  handleCall(Call, Call->getImplicitObjectArgument(),
+    llvm::makeArrayRef(Call->getArgs(), Call->getNumArgs()), MD);
   propagateReturnType(Call, MD);
 }
 
 void ConsumedStmtVisitor::VisitCXXOperatorCallExpr(
     const CXXOperatorCallExpr *Call) {
   const auto *FunDecl = dyn_cast_or_null<FunctionDecl>(Call->getDirectCallee());
   if (!FunDecl) return;
+  ArrayRef<const Expr *> Args(Call->getArgs(), Call->getNumArgs());
 
   if (Call->getOperator() == OO_Equal) {
-    ConsumedState CS = getInfo(Call->getArg(1));
-    if (!handleCall(Call, Call->getArg(0), FunDecl))
-      setInfo(Call->getArg(0), CS);
+    ConsumedState CS = getInfo(Args[1]);
+    if (!handleCall(Call, Args[0], Args.drop_front(1), FunDecl))
+      setInfo(Args[0], CS);
     return;
   }
 
-  if (const auto *MCall = dyn_cast<CXXMemberCallExpr>(Call))
-    handleCall(MCall, MCall->getImplicitObjectArgument(), FunDecl);
+  if (isa<CXXMethodDecl>(FunDecl))
+    handleCall(Call, Args[0], Args.drop_front(1), FunDecl);
   else
-    handleCall(Call, Call->getArg(0), FunDecl);
+    handleCall(Call, nullptr, Args, FunDecl);
 
   propagateReturnType(Call, FunDecl);
 }
 
 void ConsumedStmtVisitor::VisitDeclRefExpr(const DeclRefExpr *DeclRef) {
   if (const auto *Var = dyn_cast_or_null<VarDecl>(DeclRef->getDecl()))
     if (StateMap->getState(Var) != consumed::CS_None)
       PropagationMap.insert(PairType(DeclRef, PropagationInfo(Var)));