<div dir="ltr"><div>Hi Diana,</div><div>hi Richard,</div><div><br></div><div>thank you for the feedback! <br></div><div><br></div>Diana,<div><div>I remember that some gcc version had issues with raw string literals inside macros. I'll fix that to use normal<br></div><div>string literals instead.</div><div><br></div><div><div>Richard,</div><div>We are definitely want the gsl::Pointer-based warnings that are enabled by default to be free of any false-positives.<br></div><div>As you expected, this is showing a problem we have in another part of our analysis, and we will fix it before landing</div><div>this PR again.</div><div><br></div></div></div><div>Both, sorry for the breakage!</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">Am Do., 22. Aug. 2019 um 19:47 Uhr schrieb Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>>:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">Reverted in r369677.<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, 22 Aug 2019 at 10:34, Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>Hi Matthias,</div><div><br></div><div>This introduces false positives into -Wreturn-stack-address for an example such as:</div><div><br></div><div>#include <vector><br><br>std::vector<int>::iterator downcast_to(std::vector<int>::iterator value) {<br>  return *&value;<br>}<br></div><div><br></div><div>This breaks an internal build bot for us, so I'm going to revert this for now (though I expect this isn't the cause of the problem, but is rather exposing a problem elsewhere).</div><div><br></div><div>If we can make the gsl::Pointer diagnostics false-positive-free, that's great, but otherwise we should use a different warning flag for the warnings that involve these annotations and use -Wreturn-stack-address for only the zero-false-positive cases that it historically diagnosed.</div><div><br></div><div>Thanks, and sorry for the trouble.</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 21 Aug 2019 at 15:07, Matthias Gehre via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Author: mgehre<br>
Date: Wed Aug 21 15:08:59 2019<br>
New Revision: 369591<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=369591&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=369591&view=rev</a><br>
Log:<br>
[LifetimeAnalysis] Support more STL idioms (template forward declaration and DependentNameType)<br>
<br>
Summary:<br>
This fixes inference of gsl::Pointer on std::set::iterator with libstdc++ (the typedef for iterator<br>
on the template is a DependentNameType - we can only put the gsl::Pointer attribute<br>
on the underlaying record after instantiation)<br>
<br>
inference of gsl::Pointer on std::vector::iterator with libc++ (the class was forward-declared,<br>
we added the gsl::Pointer on the canonical decl (the forward decl), and later when the<br>
template was instantiated, there was no attribute on the definition so it was not instantiated).<br>
<br>
and a duplicate gsl::Pointer on some class with libstdc++ (we first added an attribute to<br>
a incomplete instantiation, and then another was copied from the template definition<br>
when the instantiation was completed).<br>
<br>
We now add the attributes to all redeclarations to fix thos issues and make their usage easier.<br>
<br>
Reviewers: gribozavr<br>
<br>
Subscribers: Szelethus, xazax.hun, cfe-commits<br>
<br>
Tags: #clang<br>
<br>
Differential Revision: <a href="https://reviews.llvm.org/D66179" rel="noreferrer" target="_blank">https://reviews.llvm.org/D66179</a><br>
<br>
Added:<br>
    cfe/trunk/unittests/Sema/GslOwnerPointerInference.cpp<br>
Modified:<br>
    cfe/trunk/lib/Sema/SemaAttr.cpp<br>
    cfe/trunk/lib/Sema/SemaDeclAttr.cpp<br>
    cfe/trunk/lib/Sema/SemaInit.cpp<br>
    cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp<br>
    cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer-std.cpp<br>
    cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer.cpp<br>
    cfe/trunk/unittests/Sema/CMakeLists.txt<br>
<br>
Modified: cfe/trunk/lib/Sema/SemaAttr.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaAttr.cpp?rev=369591&r1=369590&r2=369591&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaAttr.cpp?rev=369591&r1=369590&r2=369591&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Sema/SemaAttr.cpp (original)<br>
+++ cfe/trunk/lib/Sema/SemaAttr.cpp Wed Aug 21 15:08:59 2019<br>
@@ -88,13 +88,11 @@ void Sema::AddMsStructLayoutForRecord(Re<br>
 template <typename Attribute><br>
 static void addGslOwnerPointerAttributeIfNotExisting(ASTContext &Context,<br>
                                                      CXXRecordDecl *Record) {<br>
-  CXXRecordDecl *Canonical = Record->getCanonicalDecl();<br>
-  if (Canonical->hasAttr<OwnerAttr>() || Canonical->hasAttr<PointerAttr>())<br>
+  if (Record->hasAttr<OwnerAttr>() || Record->hasAttr<PointerAttr>())<br>
     return;<br>
<br>
-  Canonical->addAttr(::new (Context) Attribute(SourceRange{}, Context,<br>
-                                               /*DerefType*/ nullptr,<br>
-                                               /*Spelling=*/0));<br>
+  for (Decl *Redecl : Record->redecls())<br>
+    Redecl->addAttr(Attribute::CreateImplicit(Context, /*DerefType=*/nullptr));<br>
 }<br>
<br>
 void Sema::inferGslPointerAttribute(NamedDecl *ND,<br>
@@ -189,8 +187,7 @@ void Sema::inferGslOwnerPointerAttribute<br>
<br>
   // Handle classes that directly appear in std namespace.<br>
   if (Record->isInStdNamespace()) {<br>
-    CXXRecordDecl *Canonical = Record->getCanonicalDecl();<br>
-    if (Canonical->hasAttr<OwnerAttr>() || Canonical->hasAttr<PointerAttr>())<br>
+    if (Record->hasAttr<OwnerAttr>() || Record->hasAttr<PointerAttr>())<br>
       return;<br>
<br>
     if (StdOwners.count(Record->getName()))<br>
<br>
Modified: cfe/trunk/lib/Sema/SemaDeclAttr.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclAttr.cpp?rev=369591&r1=369590&r2=369591&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclAttr.cpp?rev=369591&r1=369590&r2=369591&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Sema/SemaDeclAttr.cpp (original)<br>
+++ cfe/trunk/lib/Sema/SemaDeclAttr.cpp Wed Aug 21 15:08:59 2019<br>
@@ -4592,9 +4592,11 @@ static void handleLifetimeCategoryAttr(S<br>
       }<br>
       return;<br>
     }<br>
-    D->addAttr(::new (S.Context)<br>
-                   OwnerAttr(AL.getRange(), S.Context, DerefTypeLoc,<br>
-                             AL.getAttributeSpellingListIndex()));<br>
+    for (Decl *Redecl : D->redecls()) {<br>
+      Redecl->addAttr(::new (S.Context)<br>
+                          OwnerAttr(AL.getRange(), S.Context, DerefTypeLoc,<br>
+                                    AL.getAttributeSpellingListIndex()));<br>
+    }<br>
   } else {<br>
     if (checkAttrMutualExclusion<OwnerAttr>(S, D, AL))<br>
       return;<br>
@@ -4609,9 +4611,11 @@ static void handleLifetimeCategoryAttr(S<br>
       }<br>
       return;<br>
     }<br>
-    D->addAttr(::new (S.Context)<br>
-                   PointerAttr(AL.getRange(), S.Context, DerefTypeLoc,<br>
-                               AL.getAttributeSpellingListIndex()));<br>
+    for (Decl *Redecl : D->redecls()) {<br>
+      Redecl->addAttr(::new (S.Context)<br>
+                          PointerAttr(AL.getRange(), S.Context, DerefTypeLoc,<br>
+                                      AL.getAttributeSpellingListIndex()));<br>
+    }<br>
   }<br>
 }<br>
<br>
<br>
Modified: cfe/trunk/lib/Sema/SemaInit.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaInit.cpp?rev=369591&r1=369590&r2=369591&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaInit.cpp?rev=369591&r1=369590&r2=369591&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Sema/SemaInit.cpp (original)<br>
+++ cfe/trunk/lib/Sema/SemaInit.cpp Wed Aug 21 15:08:59 2019<br>
@@ -6561,7 +6561,7 @@ static void visitLocalsRetainedByReferen<br>
<br>
 template <typename T> static bool isRecordWithAttr(QualType Type) {<br>
   if (auto *RD = Type->getAsCXXRecordDecl())<br>
-    return RD->getCanonicalDecl()->hasAttr<T>();<br>
+    return RD->hasAttr<T>();<br>
   return false;<br>
 }<br>
<br>
@@ -6672,7 +6672,7 @@ static void handleGslAnnotatedTypes(Indi<br>
<br>
   if (auto *CCE = dyn_cast<CXXConstructExpr>(Call)) {<br>
     const auto *Ctor = CCE->getConstructor();<br>
-    const CXXRecordDecl *RD = Ctor->getParent()->getCanonicalDecl();<br>
+    const CXXRecordDecl *RD = Ctor->getParent();<br>
     if (CCE->getNumArgs() > 0 && RD->hasAttr<PointerAttr>())<br>
       VisitPointerArg(Ctor->getParamDecl(0), CCE->getArgs()[0]);<br>
   }<br>
<br>
Modified: cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp?rev=369591&r1=369590&r2=369591&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp?rev=369591&r1=369590&r2=369591&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp (original)<br>
+++ cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp Wed Aug 21 15:08:59 2019<br>
@@ -552,6 +552,18 @@ void Sema::InstantiateAttrs(const MultiL<br>
       continue;<br>
     }<br>
<br>
+    if (auto *A = dyn_cast<PointerAttr>(TmplAttr)) {<br>
+      if (!New->hasAttr<PointerAttr>())<br>
+        New->addAttr(A->clone(Context));<br>
+      continue;<br>
+    }<br>
+<br>
+    if (auto *A = dyn_cast<OwnerAttr>(TmplAttr)) {<br>
+      if (!New->hasAttr<OwnerAttr>())<br>
+        New->addAttr(A->clone(Context));<br>
+      continue;<br>
+    }<br>
+<br>
     assert(!TmplAttr->isPackExpansion());<br>
     if (TmplAttr->isLateParsed() && LateAttrs) {<br>
       // Late parsed attributes must be instantiated and attached after the<br>
@@ -711,6 +723,9 @@ Decl *TemplateDeclInstantiator::Instanti<br>
<br>
   SemaRef.InstantiateAttrs(TemplateArgs, D, Typedef);<br>
<br>
+  if (D->getUnderlyingType()->getAs<DependentNameType>())<br>
+    SemaRef.inferGslPointerAttribute(Typedef);<br>
+<br>
   Typedef->setAccess(D->getAccess());<br>
<br>
   return Typedef;<br>
<br>
Modified: cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer-std.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer-std.cpp?rev=369591&r1=369590&r2=369591&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer-std.cpp?rev=369591&r1=369590&r2=369591&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer-std.cpp (original)<br>
+++ cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer-std.cpp Wed Aug 21 15:08:59 2019<br>
@@ -92,6 +92,59 @@ public:<br>
 static_assert(sizeof(unordered_map<int>::iterator), ""); // Force instantiation.<br>
 } // namespace inlinens<br>
<br>
+// The iterator typedef is a DependentNameType.<br>
+template <typename T><br>
+class __unordered_multimap_iterator {};<br>
+// CHECK: ClassTemplateDecl {{.*}} __unordered_multimap_iterator<br>
+// CHECK: ClassTemplateSpecializationDecl {{.*}} __unordered_multimap_iterator<br>
+// CHECK: TemplateArgument type 'int'<br>
+// CHECK: PointerAttr<br>
+<br>
+template <typename T><br>
+class __unordered_multimap_base {<br>
+public:<br>
+  using iterator = __unordered_multimap_iterator<T>;<br>
+};<br>
+<br>
+template <typename T><br>
+class unordered_multimap {<br>
+  // CHECK: ClassTemplateDecl {{.*}} unordered_multimap<br>
+  // CHECK: OwnerAttr {{.*}}<br>
+  // CHECK: ClassTemplateSpecializationDecl {{.*}} unordered_multimap<br>
+  // CHECK: OwnerAttr {{.*}}<br>
+public:<br>
+  using _Mybase = __unordered_multimap_base<T>;<br>
+  using iterator = typename _Mybase::iterator;<br>
+};<br>
+static_assert(sizeof(unordered_multimap<int>::iterator), ""); // Force instantiation.<br>
+<br>
+// The canonical declaration of the iterator template is not its definition.<br>
+template <typename T><br>
+class __unordered_multiset_iterator;<br>
+// CHECK: ClassTemplateDecl {{.*}} __unordered_multiset_iterator<br>
+// CHECK: PointerAttr<br>
+// CHECK: ClassTemplateSpecializationDecl {{.*}} __unordered_multiset_iterator<br>
+// CHECK: TemplateArgument type 'int'<br>
+// CHECK: PointerAttr<br>
+<br>
+template <typename T><br>
+class __unordered_multiset_iterator {<br>
+  // CHECK: ClassTemplateDecl {{.*}} prev {{.*}} __unordered_multiset_iterator<br>
+  // CHECK: PointerAttr<br>
+};<br>
+<br>
+template <typename T><br>
+class unordered_multiset {<br>
+  // CHECK: ClassTemplateDecl {{.*}} unordered_multiset<br>
+  // CHECK: OwnerAttr {{.*}}<br>
+  // CHECK: ClassTemplateSpecializationDecl {{.*}} unordered_multiset<br>
+  // CHECK: OwnerAttr {{.*}}<br>
+public:<br>
+  using iterator = __unordered_multiset_iterator<T>;<br>
+};<br>
+<br>
+static_assert(sizeof(unordered_multiset<int>::iterator), ""); // Force instantiation.<br>
+<br>
 // std::list has an implicit gsl::Owner attribute,<br>
 // but explicit attributes take precedence.<br>
 template <typename T><br>
<br>
Modified: cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer.cpp?rev=369591&r1=369590&r2=369591&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer.cpp?rev=369591&r1=369590&r2=369591&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer.cpp (original)<br>
+++ cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer.cpp Wed Aug 21 15:08:59 2019<br>
@@ -105,3 +105,20 @@ class [[gsl::Owner(int)]] AddTheSameLate<br>
 class [[gsl::Owner(int)]] AddTheSameLater;<br>
 // CHECK: CXXRecordDecl {{.*}} prev {{.*}} AddTheSameLater<br>
 // CHECK: OwnerAttr {{.*}} int<br>
+<br>
+template <class T><br>
+class [[gsl::Owner]] ForwardDeclared;<br>
+// CHECK: ClassTemplateDecl {{.*}} ForwardDeclared<br>
+// CHECK: OwnerAttr {{.*}}<br>
+// CHECK: ClassTemplateSpecializationDecl {{.*}} ForwardDeclared<br>
+// CHECK: TemplateArgument type 'int'<br>
+// CHECK: OwnerAttr {{.*}}<br>
+<br>
+template <class T><br>
+class [[gsl::Owner]] ForwardDeclared {<br>
+// CHECK: ClassTemplateDecl {{.*}} ForwardDeclared<br>
+// CHECK: CXXRecordDecl {{.*}} ForwardDeclared definition<br>
+// CHECK: OwnerAttr {{.*}}<br>
+};<br>
+<br>
+static_assert(sizeof(ForwardDeclared<int>), ""); // Force instantiation.<br>
<br>
Modified: cfe/trunk/unittests/Sema/CMakeLists.txt<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Sema/CMakeLists.txt?rev=369591&r1=369590&r2=369591&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Sema/CMakeLists.txt?rev=369591&r1=369590&r2=369591&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/unittests/Sema/CMakeLists.txt (original)<br>
+++ cfe/trunk/unittests/Sema/CMakeLists.txt Wed Aug 21 15:08:59 2019<br>
@@ -5,11 +5,13 @@ set(LLVM_LINK_COMPONENTS<br>
 add_clang_unittest(SemaTests<br>
   ExternalSemaSourceTest.cpp<br>
   CodeCompleteTest.cpp<br>
+  GslOwnerPointerInference.cpp<br>
   )<br>
<br>
 clang_target_link_libraries(SemaTests<br>
   PRIVATE<br>
   clangAST<br>
+  clangASTMatchers<br>
   clangBasic<br>
   clangFrontend<br>
   clangParse<br>
<br>
Added: cfe/trunk/unittests/Sema/GslOwnerPointerInference.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Sema/GslOwnerPointerInference.cpp?rev=369591&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Sema/GslOwnerPointerInference.cpp?rev=369591&view=auto</a><br>
==============================================================================<br>
--- cfe/trunk/unittests/Sema/GslOwnerPointerInference.cpp (added)<br>
+++ cfe/trunk/unittests/Sema/GslOwnerPointerInference.cpp Wed Aug 21 15:08:59 2019<br>
@@ -0,0 +1,61 @@<br>
+//== unittests/Sema/GslOwnerPointerInference.cpp - gsl::Owner/Pointer ========//<br>
+//<br>
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.<br>
+// See <a href="https://llvm.org/LICENSE.txt" rel="noreferrer" target="_blank">https://llvm.org/LICENSE.txt</a> for license information.<br>
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception<br>
+//<br>
+//===----------------------------------------------------------------------===//<br>
+<br>
+#include "../ASTMatchers/ASTMatchersTest.h"<br>
+#include "clang/ASTMatchers/ASTMatchers.h"<br>
+#include "gtest/gtest.h"<br>
+<br>
+namespace clang {<br>
+using namespace ast_matchers;<br>
+<br>
+TEST(OwnerPointer, BothHaveAttributes) {<br>
+  EXPECT_TRUE(matches(<br>
+    R"cpp(<br>
+      template<class T><br>
+      class [[gsl::Owner]] C;<br>
+<br>
+      template<class T><br>
+      class [[gsl::Owner]] C {};<br>
+<br>
+      C<int> c;<br>
+  )cpp",<br>
+    classTemplateSpecializationDecl(hasName("C"), hasAttr(clang::attr::Owner))));<br>
+}<br>
+<br>
+TEST(OwnerPointer, ForwardDeclOnly) {<br>
+  EXPECT_TRUE(matches(<br>
+    R"cpp(<br>
+      template<class T><br>
+      class [[gsl::Owner]] C;<br>
+<br>
+      template<class T><br>
+      class C {};<br>
+<br>
+      C<int> c;<br>
+  )cpp",<br>
+    classTemplateSpecializationDecl(hasName("C"), hasAttr(clang::attr::Owner))));<br>
+}<br>
+<br>
+TEST(OwnerPointer, LateForwardDeclOnly) {<br>
+  EXPECT_TRUE(matches(<br>
+    R"cpp(<br>
+      template<class T><br>
+      class C;<br>
+<br>
+      template<class T><br>
+      class C {};<br>
+<br>
+      template<class T><br>
+      class [[gsl::Owner]] C;<br>
+<br>
+      C<int> c;<br>
+  )cpp",<br>
+    classTemplateSpecializationDecl(hasName("C"), hasAttr(clang::attr::Owner))));<br>
+}<br>
+<br>
+} // namespace clang<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</blockquote></div></div>
</blockquote></div>
</blockquote></div>