r368446 - More warnings regarding gsl::Pointer and gsl::Owner attributes

Nico Weber via cfe-commits cfe-commits at lists.llvm.org
Sun Aug 11 16:35:25 PDT 2019


Confirmed, after r368534 the Chromium builds looks good again. (r368528
without r368534 regressed something else, but r368534 fixed things. I'm
assuming you don't need a repro for the breakage in r368528
without r368534.)

Your warnings even found a bug:
https://bugs.chromium.org/p/openscreen/issues/detail?id=63 Thanks! :)

On Sun, Aug 11, 2019 at 3:18 PM Gábor Horváth <xazax.hun at gmail.com> wrote:

> This should be fixed now.
>
> On Sat, Aug 10, 2019, 8:08 PM Gábor Horváth <xazax.hun at gmail.com> wrote:
>
>> You are right!
>>
>> I will look into this tomorrow. If you think this is urgent feel free to
>> revert the commits.
>>
>> Cheers,
>> Gabor
>>
>> On Sat, Aug 10, 2019, 6:41 PM Nico Weber <thakis at chromium.org> wrote:
>>
>>> Hi Gabor,
>>>
>>> this makes clang warn on this program:
>>>
>>> $ cat test.cc
>>> #include <algorithm>
>>> #include <vector>
>>>
>>> struct S {
>>>   bool operator==(const S &rhs) const;
>>> };
>>>
>>> const S &f(const std::vector<S> &v) {
>>>   const auto &it = std::find(v.rbegin(), v.rend(), S());
>>>   return *it;
>>> }
>>>
>>>
>>> $ out/gn/bin/clang -c test.cc -isysroot $(xcrun -show-sdk-path) -isystem
>>> libcxx/include/ -Wall
>>> test.cc:10:11: warning: returning reference to local temporary object
>>> [-Wreturn-stack-address]
>>>   return *it;
>>>           ^~
>>> test.cc:9:15: note: binding reference variable 'it' here
>>>   const auto &it = std::find(v.rbegin(), v.rend(), S());
>>>               ^    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> 1 warning generated.
>>>
>>>
>>> Is the warning correct here? The `const auto &it` is lifetime-extended
>>> to the end of the block, and *it should return a reference to an element in
>>> the vector. Since the vector's passed in, this should be fine. Or am I
>>> missing something?
>>>
>>> Thanks,
>>> Nico
>>>
>>> On Fri, Aug 9, 2019 at 11:15 AM Gabor Horvath via cfe-commits <
>>> cfe-commits at lists.llvm.org> wrote:
>>>
>>>> Author: xazax
>>>> Date: Fri Aug  9 08:16:35 2019
>>>> New Revision: 368446
>>>>
>>>> URL: http://llvm.org/viewvc/llvm-project?rev=368446&view=rev
>>>> Log:
>>>> More warnings regarding gsl::Pointer and gsl::Owner attributes
>>>>
>>>> Differential Revision: https://reviews.llvm.org/D65120
>>>>
>>>> Modified:
>>>>     cfe/trunk/lib/Sema/SemaInit.cpp
>>>>     cfe/trunk/test/Analysis/inner-pointer.cpp
>>>>     cfe/trunk/test/Sema/warn-lifetime-analysis-nocfg.cpp
>>>>
>>>> Modified: cfe/trunk/lib/Sema/SemaInit.cpp
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaInit.cpp?rev=368446&r1=368445&r2=368446&view=diff
>>>>
>>>> ==============================================================================
>>>> --- cfe/trunk/lib/Sema/SemaInit.cpp (original)
>>>> +++ cfe/trunk/lib/Sema/SemaInit.cpp Fri Aug  9 08:16:35 2019
>>>> @@ -6564,6 +6564,25 @@ template <typename T> static bool isReco
>>>>    return false;
>>>>  }
>>>>
>>>> +static bool shouldTrackImplicitObjectArg(const CXXMethodDecl *Callee) {
>>>> +  if (auto *Conv = dyn_cast_or_null<CXXConversionDecl>(Callee))
>>>> +    if (isRecordWithAttr<PointerAttr>(Conv->getConversionType()))
>>>> +      return true;
>>>> +  if (!Callee->getParent()->isInStdNamespace() ||
>>>> !Callee->getIdentifier())
>>>> +    return false;
>>>> +  if (!isRecordWithAttr<PointerAttr>(Callee->getThisObjectType()) &&
>>>> +      !isRecordWithAttr<OwnerAttr>(Callee->getThisObjectType()))
>>>> +    return false;
>>>> +  if (!isRecordWithAttr<PointerAttr>(Callee->getReturnType()) &&
>>>> +      !Callee->getReturnType()->isPointerType())
>>>> +    return false;
>>>> +  return llvm::StringSwitch<bool>(Callee->getName())
>>>> +      .Cases("begin", "rbegin", "cbegin", "crbegin", true)
>>>> +      .Cases("end", "rend", "cend", "crend", true)
>>>> +      .Cases("c_str", "data", "get", true)
>>>> +      .Default(false);
>>>> +}
>>>> +
>>>>  static void handleGslAnnotatedTypes(IndirectLocalPath &Path, Expr
>>>> *Call,
>>>>                                      LocalVisitor Visit) {
>>>>    auto VisitPointerArg = [&](const Decl *D, Expr *Arg) {
>>>> @@ -6577,10 +6596,9 @@ static void handleGslAnnotatedTypes(Indi
>>>>    };
>>>>
>>>>    if (auto *MCE = dyn_cast<CXXMemberCallExpr>(Call)) {
>>>> -    const FunctionDecl *Callee = MCE->getDirectCallee();
>>>> -    if (auto *Conv = dyn_cast_or_null<CXXConversionDecl>(Callee))
>>>> -      if (isRecordWithAttr<PointerAttr>(Conv->getConversionType()))
>>>> -        VisitPointerArg(Callee, MCE->getImplicitObjectArgument());
>>>> +    const auto *MD =
>>>> cast_or_null<CXXMethodDecl>(MCE->getDirectCallee());
>>>> +    if (MD && shouldTrackImplicitObjectArg(MD))
>>>> +      VisitPointerArg(MD, MCE->getImplicitObjectArgument());
>>>>      return;
>>>>    }
>>>>
>>>>
>>>> Modified: cfe/trunk/test/Analysis/inner-pointer.cpp
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/inner-pointer.cpp?rev=368446&r1=368445&r2=368446&view=diff
>>>>
>>>> ==============================================================================
>>>> --- cfe/trunk/test/Analysis/inner-pointer.cpp (original)
>>>> +++ cfe/trunk/test/Analysis/inner-pointer.cpp Fri Aug  9 08:16:35 2019
>>>> @@ -1,4 +1,5 @@
>>>> -// RUN: %clang_analyze_cc1 -analyzer-checker=cplusplus.InnerPointer \
>>>> +// RUN: %clang_analyze_cc1 -analyzer-checker=cplusplus.InnerPointer   \
>>>> +// RUN:   -Wno-dangling -Wno-dangling-field -Wno-return-stack-address \
>>>>  // RUN:   %s -analyzer-output=text -verify
>>>>
>>>>  #include "Inputs/system-header-simulator-cxx.h"
>>>>
>>>> Modified: cfe/trunk/test/Sema/warn-lifetime-analysis-nocfg.cpp
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/warn-lifetime-analysis-nocfg.cpp?rev=368446&r1=368445&r2=368446&view=diff
>>>>
>>>> ==============================================================================
>>>> --- cfe/trunk/test/Sema/warn-lifetime-analysis-nocfg.cpp (original)
>>>> +++ cfe/trunk/test/Sema/warn-lifetime-analysis-nocfg.cpp Fri Aug  9
>>>> 08:16:35 2019
>>>> @@ -2,7 +2,6 @@
>>>>  struct [[gsl::Owner(int)]] MyIntOwner {
>>>>    MyIntOwner();
>>>>    int &operator*();
>>>> -  int *c_str() const;
>>>>  };
>>>>
>>>>  struct [[gsl::Pointer(int)]] MyIntPointer {
>>>> @@ -52,16 +51,6 @@ long *ownershipTransferToRawPointer() {
>>>>    return t.releaseAsRawPointer(); // ok
>>>>  }
>>>>
>>>> -int *danglingRawPtrFromLocal() {
>>>> -  MyIntOwner t;
>>>> -  return t.c_str(); // TODO
>>>> -}
>>>> -
>>>> -int *danglingRawPtrFromTemp() {
>>>> -  MyIntPointer p;
>>>> -  return p.toOwner().c_str(); // TODO
>>>> -}
>>>> -
>>>>  struct Y {
>>>>    int a[4];
>>>>  };
>>>> @@ -103,6 +92,12 @@ MyIntPointer danglingGslPtrFromTemporary
>>>>    return MyIntOwner{}; // expected-warning {{returning address of
>>>> local temporary object}}
>>>>  }
>>>>
>>>> +MyIntOwner makeTempOwner();
>>>> +
>>>> +MyIntPointer danglingGslPtrFromTemporary2() {
>>>> +  return makeTempOwner(); // expected-warning {{returning address of
>>>> local temporary object}}
>>>> +}
>>>> +
>>>>  MyLongPointerFromConversion danglingGslPtrFromTemporaryConv() {
>>>>    return MyLongOwnerWithConversion{}; // expected-warning {{returning
>>>> address of local temporary object}}
>>>>  }
>>>> @@ -124,12 +119,52 @@ void initLocalGslPtrWithTempOwner() {
>>>>    global2 = MyLongOwnerWithConversion{}; // TODO ?
>>>>  }
>>>>
>>>> -struct IntVector {
>>>> -  int *begin();
>>>> -  int *end();
>>>> +namespace std {
>>>> +template <typename T>
>>>> +struct basic_iterator {};
>>>> +
>>>> +template <typename T>
>>>> +struct vector {
>>>> +  typedef basic_iterator<T> iterator;
>>>> +  iterator begin();
>>>> +  T *data();
>>>> +};
>>>> +
>>>> +template<typename T>
>>>> +struct basic_string {
>>>> +  const T *c_str() const;
>>>> +};
>>>> +
>>>> +template<typename T>
>>>> +struct unique_ptr {
>>>> +  T *get() const;
>>>>  };
>>>> +}
>>>>
>>>>  void modelIterators() {
>>>> -  int *it = IntVector{}.begin(); // TODO ?
>>>> +  std::vector<int>::iterator it = std::vector<int>().begin(); //
>>>> expected-warning {{object backing the pointer will be destroyed at the end
>>>> of the full-expression}}
>>>>    (void)it;
>>>>  }
>>>> +
>>>> +std::vector<int>::iterator modelIteratorReturn() {
>>>> +  return std::vector<int>().begin(); // expected-warning {{returning
>>>> address of local temporary object}}
>>>> +}
>>>> +
>>>> +const char *danglingRawPtrFromLocal() {
>>>> +  std::basic_string<char> s;
>>>> +  return s.c_str(); // expected-warning {{address of stack memory
>>>> associated with local variable 's' returned}}
>>>> +}
>>>> +
>>>> +const char *danglingRawPtrFromTemp() {
>>>> +  return std::basic_string<char>().c_str(); // expected-warning
>>>> {{returning address of local temporary object}}
>>>> +}
>>>> +
>>>> +std::unique_ptr<int> getUniquePtr();
>>>> +
>>>> +int *danglingUniquePtrFromTemp() {
>>>> +  return getUniquePtr().get(); // expected-warning {{returning address
>>>> of local temporary object}}
>>>> +}
>>>> +
>>>> +int *danglingUniquePtrFromTemp2() {
>>>> +  return std::unique_ptr<int>().get(); // expected-warning {{returning
>>>> address of local temporary object}}
>>>> +}
>>>>
>>>>
>>>> _______________________________________________
>>>> cfe-commits mailing list
>>>> cfe-commits at lists.llvm.org
>>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>>
>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190811/9b8aab1e/attachment-0001.html>


More information about the cfe-commits mailing list