Re: r369591 - [LifetimeAnalysis] Support more STL idioms (template forward declaration and DependentNameType)

2019-08-23 Thread Nico Weber via cfe-commits 
On Thu, Aug 22, 2019 at 4:05 PM Matthias Gehre via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> Hi Diana,
> hi Richard,
>
> thank you for the feedback!
>
> Diana,
> I remember that some gcc version had issues with raw string literals
> inside macros. I'll fix that to use normal
> string literals instead.
>

I think it's only a problem if the raw string is in a macro. Instead of

  FOO(R"(asdf)");

you can do

  const char kStr[] = R"(asdf)";
  FOO(kStr);

and gcc should be happy. (In case raw strings buy you something over normal
string literals.)


>
> Richard,
> We are definitely want the gsl::Pointer-based warnings that are enabled by
> default to be free of any false-positives.
> As you expected, this is showing a problem we have in another part of our
> analysis, and we will fix it before landing
> this PR again.
>
> Both, sorry for the breakage!
>
> Am Do., 22. Aug. 2019 um 19:47 Uhr schrieb Richard Smith <
> rich...@metafoo.co.uk>:
>
>> Reverted in r369677.
>>
>> On Thu, 22 Aug 2019 at 10:34, Richard Smith 
>> wrote:
>>
>>> Hi Matthias,
>>>
>>> This introduces false positives into -Wreturn-stack-address for an
>>> example such as:
>>>
>>> #include 
>>>
>>> std::vector::iterator downcast_to(std::vector::iterator value)
>>> {
>>>   return *
>>> }
>>>
>>> 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).
>>>
>>> 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.
>>>
>>> Thanks, and sorry for the trouble.
>>>
>>> On Wed, 21 Aug 2019 at 15:07, Matthias Gehre via cfe-commits <
>>> cfe-commits@lists.llvm.org> wrote:
>>>
 Author: mgehre
 Date: Wed Aug 21 15:08:59 2019
 New Revision: 369591

 URL: http://llvm.org/viewvc/llvm-project?rev=369591=rev
 Log:
 [LifetimeAnalysis] Support more STL idioms (template forward
 declaration and DependentNameType)

 Summary:
 This fixes inference of gsl::Pointer on std::set::iterator with
 libstdc++ (the typedef for iterator
 on the template is a DependentNameType - we can only put the
 gsl::Pointer attribute
 on the underlaying record after instantiation)

 inference of gsl::Pointer on std::vector::iterator with libc++ (the
 class was forward-declared,
 we added the gsl::Pointer on the canonical decl (the forward decl), and
 later when the
 template was instantiated, there was no attribute on the definition so
 it was not instantiated).

 and a duplicate gsl::Pointer on some class with libstdc++ (we first
 added an attribute to
 a incomplete instantiation, and then another was copied from the
 template definition
 when the instantiation was completed).

 We now add the attributes to all redeclarations to fix thos issues and
 make their usage easier.

 Reviewers: gribozavr

 Subscribers: Szelethus, xazax.hun, cfe-commits

 Tags: #clang

 Differential Revision: https://reviews.llvm.org/D66179

 Added:
 cfe/trunk/unittests/Sema/GslOwnerPointerInference.cpp
 Modified:
 cfe/trunk/lib/Sema/SemaAttr.cpp
 cfe/trunk/lib/Sema/SemaDeclAttr.cpp
 cfe/trunk/lib/Sema/SemaInit.cpp
 cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp
 cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer-std.cpp
 cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer.cpp
 cfe/trunk/unittests/Sema/CMakeLists.txt

 Modified: cfe/trunk/lib/Sema/SemaAttr.cpp
 URL:
 http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaAttr.cpp?rev=369591=369590=369591=diff

 ==
 --- cfe/trunk/lib/Sema/SemaAttr.cpp (original)
 +++ cfe/trunk/lib/Sema/SemaAttr.cpp Wed Aug 21 15:08:59 2019
 @@ -88,13 +88,11 @@ void Sema::AddMsStructLayoutForRecord(Re
  template 
  static void addGslOwnerPointerAttributeIfNotExisting(ASTContext
 ,
   CXXRecordDecl
 *Record) {
 -  CXXRecordDecl *Canonical = Record->getCanonicalDecl();
 -  if (Canonical->hasAttr() ||
 Canonical->hasAttr())
 +  if (Record->hasAttr() || Record->hasAttr())
  return;

 -  Canonical->addAttr(::new (Context) Attribute(SourceRange{}, Context,
 -   /*DerefType*/ nullptr,
 -   /*Spelling=*/0));
 +  for (Decl *Redecl : Record->redecls())
 +Redecl->addAttr(Attribute::CreateImplicit(Context,
 /*DerefType=*/nullptr));
  }

  void

Re: r369591 - [LifetimeAnalysis] Support more STL idioms (template forward declaration and DependentNameType)

2019-08-23 Thread Richard Smith via cfe-commits 
Thank you for the fast turnaround here!

On Fri, 23 Aug 2019 at 15:26, Gábor Horváth via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> I committed this in r369817 to keep things moving. If you have any
> suggestion, complaints let me know and I will do my best to solve it
> post-commit ASAP.
>
> On Fri, 23 Aug 2019 at 14:51, Gábor Horváth  wrote:
>
>> Hi Richard,
>>
>> Sorry for the slow response, unfortunately the compile times are not
>> great on the machine I have access to at the moment.
>> Here is a patch, I'll commit it if you agree with the approach:
>> https://reviews.llvm.org/D66686
>>
>> Basically, the idea is to not run the new warning related code at all
>> when it is turned off, so other warning flags like ReturnStackAddress
>> should never be triggered by the new warnings.
>> In the future, we can come up with something else but I wanted to go for
>> the safe solution given the urgency.
>>
>> Cheers,
>> Gabor
>>
>> On Fri, 23 Aug 2019 at 13:31, Gábor Horváth  wrote:
>>
>>> Hi Richard,
>>>
>>> I'll move these behind a flag today. Moving forward, it would be great
>>> to have a way to dogfood those warnings without blocking you. We do run
>>> them over ~340 open source projects regularly, but clearly that is not
>>> enough :)
>>>
>>> Thanks,
>>> Gabor
>>>
>>> On Fri, 23 Aug 2019 at 13:03, Richard Smith 
>>> wrote:
>>>
 On Thu, 22 Aug 2019 at 13:05, Matthias Gehre via cfe-commits <
 cfe-commits@lists.llvm.org> wrote:

> Hi Diana,
> hi Richard,
>
> thank you for the feedback!
>
> Diana,
> I remember that some gcc version had issues with raw string literals
> inside macros. I'll fix that to use normal
> string literals instead.
>
> Richard,
> We are definitely want the gsl::Pointer-based warnings that are
> enabled by default to be free of any false-positives.
> As you expected, this is showing a problem we have in another part of
> our analysis, and we will fix it before landing
> this PR again.
>

 It looks like this revert wasn't enough to unblock us. We're currently
 unable to release compilers due to the scale of the new enabled-by-default
 diagnostics produced by these warnings, and we're not happy about turning
 off the existing (zero false positives) warning flags here in order to
 unblock our releases, because they're so valuable in catching errors. I'd
 expect others will hit similar issues when upgrading Clang. Even if there
 were no false positives in the new warning, it appears to be undeployable
 as-is because the new warning is behind the same warning flag as an
 existing high-value warning. So I think we need the new warnings to be
 moved behind different warning flags (a subgroup of ReturnStackAddress
 would be OK, but it needs to be independently controllable).

 If this can be done imminently, that would be OK, but otherwise I think
 we should temporarily roll this back until it can be moved to a separate
 warning group.

 Both, sorry for the breakage!
>
> Am Do., 22. Aug. 2019 um 19:47 Uhr schrieb Richard Smith <
> rich...@metafoo.co.uk>:
>
>> Reverted in r369677.
>>
>> On Thu, 22 Aug 2019 at 10:34, Richard Smith 
>> wrote:
>>
>>> Hi Matthias,
>>>
>>> This introduces false positives into -Wreturn-stack-address for an
>>> example such as:
>>>
>>> #include 
>>>
>>> std::vector::iterator downcast_to(std::vector::iterator
>>> value) {
>>>   return *
>>> }
>>>
>>> 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).
>>>
>>> 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.
>>>
>>> Thanks, and sorry for the trouble.
>>>
>>> On Wed, 21 Aug 2019 at 15:07, Matthias Gehre via cfe-commits <
>>> cfe-commits@lists.llvm.org> wrote:
>>>
 Author: mgehre
 Date: Wed Aug 21 15:08:59 2019
 New Revision: 369591

 URL: http://llvm.org/viewvc/llvm-project?rev=369591=rev
 Log:
 [LifetimeAnalysis] Support more STL idioms (template forward
 declaration and DependentNameType)

 Summary:
 This fixes inference of gsl::Pointer on std::set::iterator with
 libstdc++ (the typedef for iterator
 on the template is a DependentNameType - we can only put the
 gsl::Pointer attribute
 on the underlaying record after instantiation)

 inference of gsl::Pointer on std::vector::iterator with libc++

Re: r369591 - [LifetimeAnalysis] Support more STL idioms (template forward declaration and DependentNameType)

2019-08-23 Thread Gábor Horváth via cfe-commits 
I committed this in r369817 to keep things moving. If you have any
suggestion, complaints let me know and I will do my best to solve it
post-commit ASAP.

On Fri, 23 Aug 2019 at 14:51, Gábor Horváth  wrote:

> Hi Richard,
>
> Sorry for the slow response, unfortunately the compile times are not great
> on the machine I have access to at the moment.
> Here is a patch, I'll commit it if you agree with the approach:
> https://reviews.llvm.org/D66686
>
> Basically, the idea is to not run the new warning related code at all when
> it is turned off, so other warning flags like ReturnStackAddress should
> never be triggered by the new warnings.
> In the future, we can come up with something else but I wanted to go for
> the safe solution given the urgency.
>
> Cheers,
> Gabor
>
> On Fri, 23 Aug 2019 at 13:31, Gábor Horváth  wrote:
>
>> Hi Richard,
>>
>> I'll move these behind a flag today. Moving forward, it would be great to
>> have a way to dogfood those warnings without blocking you. We do run them
>> over ~340 open source projects regularly, but clearly that is not enough :)
>>
>> Thanks,
>> Gabor
>>
>> On Fri, 23 Aug 2019 at 13:03, Richard Smith 
>> wrote:
>>
>>> On Thu, 22 Aug 2019 at 13:05, Matthias Gehre via cfe-commits <
>>> cfe-commits@lists.llvm.org> wrote:
>>>
 Hi Diana,
 hi Richard,

 thank you for the feedback!

 Diana,
 I remember that some gcc version had issues with raw string literals
 inside macros. I'll fix that to use normal
 string literals instead.

 Richard,
 We are definitely want the gsl::Pointer-based warnings that are enabled
 by default to be free of any false-positives.
 As you expected, this is showing a problem we have in another part of
 our analysis, and we will fix it before landing
 this PR again.

>>>
>>> It looks like this revert wasn't enough to unblock us. We're currently
>>> unable to release compilers due to the scale of the new enabled-by-default
>>> diagnostics produced by these warnings, and we're not happy about turning
>>> off the existing (zero false positives) warning flags here in order to
>>> unblock our releases, because they're so valuable in catching errors. I'd
>>> expect others will hit similar issues when upgrading Clang. Even if there
>>> were no false positives in the new warning, it appears to be undeployable
>>> as-is because the new warning is behind the same warning flag as an
>>> existing high-value warning. So I think we need the new warnings to be
>>> moved behind different warning flags (a subgroup of ReturnStackAddress
>>> would be OK, but it needs to be independently controllable).
>>>
>>> If this can be done imminently, that would be OK, but otherwise I think
>>> we should temporarily roll this back until it can be moved to a separate
>>> warning group.
>>>
>>> Both, sorry for the breakage!

 Am Do., 22. Aug. 2019 um 19:47 Uhr schrieb Richard Smith <
 rich...@metafoo.co.uk>:

> Reverted in r369677.
>
> On Thu, 22 Aug 2019 at 10:34, Richard Smith 
> wrote:
>
>> Hi Matthias,
>>
>> This introduces false positives into -Wreturn-stack-address for an
>> example such as:
>>
>> #include 
>>
>> std::vector::iterator downcast_to(std::vector::iterator
>> value) {
>>   return *
>> }
>>
>> 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).
>>
>> 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.
>>
>> Thanks, and sorry for the trouble.
>>
>> On Wed, 21 Aug 2019 at 15:07, Matthias Gehre via cfe-commits <
>> cfe-commits@lists.llvm.org> wrote:
>>
>>> Author: mgehre
>>> Date: Wed Aug 21 15:08:59 2019
>>> New Revision: 369591
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=369591=rev
>>> Log:
>>> [LifetimeAnalysis] Support more STL idioms (template forward
>>> declaration and DependentNameType)
>>>
>>> Summary:
>>> This fixes inference of gsl::Pointer on std::set::iterator with
>>> libstdc++ (the typedef for iterator
>>> on the template is a DependentNameType - we can only put the
>>> gsl::Pointer attribute
>>> on the underlaying record after instantiation)
>>>
>>> inference of gsl::Pointer on std::vector::iterator with libc++ (the
>>> class was forward-declared,
>>> we added the gsl::Pointer on the canonical decl (the forward decl),
>>> and later when the
>>> template was instantiated, there was no attribute on the definition
>>> so it was not instantiated).
>>>
>>> and

Re: r369591 - [LifetimeAnalysis] Support more STL idioms (template forward declaration and DependentNameType)

2019-08-23 Thread Gábor Horváth via cfe-commits 
Hi Richard,

Sorry for the slow response, unfortunately the compile times are not great
on the machine I have access to at the moment.
Here is a patch, I'll commit it if you agree with the approach:
https://reviews.llvm.org/D66686

Basically, the idea is to not run the new warning related code at all when
it is turned off, so other warning flags like ReturnStackAddress should
never be triggered by the new warnings.
In the future, we can come up with something else but I wanted to go for
the safe solution given the urgency.

Cheers,
Gabor

On Fri, 23 Aug 2019 at 13:31, Gábor Horváth  wrote:

> Hi Richard,
>
> I'll move these behind a flag today. Moving forward, it would be great to
> have a way to dogfood those warnings without blocking you. We do run them
> over ~340 open source projects regularly, but clearly that is not enough :)
>
> Thanks,
> Gabor
>
> On Fri, 23 Aug 2019 at 13:03, Richard Smith  wrote:
>
>> On Thu, 22 Aug 2019 at 13:05, Matthias Gehre via cfe-commits <
>> cfe-commits@lists.llvm.org> wrote:
>>
>>> Hi Diana,
>>> hi Richard,
>>>
>>> thank you for the feedback!
>>>
>>> Diana,
>>> I remember that some gcc version had issues with raw string literals
>>> inside macros. I'll fix that to use normal
>>> string literals instead.
>>>
>>> Richard,
>>> We are definitely want the gsl::Pointer-based warnings that are enabled
>>> by default to be free of any false-positives.
>>> As you expected, this is showing a problem we have in another part of
>>> our analysis, and we will fix it before landing
>>> this PR again.
>>>
>>
>> It looks like this revert wasn't enough to unblock us. We're currently
>> unable to release compilers due to the scale of the new enabled-by-default
>> diagnostics produced by these warnings, and we're not happy about turning
>> off the existing (zero false positives) warning flags here in order to
>> unblock our releases, because they're so valuable in catching errors. I'd
>> expect others will hit similar issues when upgrading Clang. Even if there
>> were no false positives in the new warning, it appears to be undeployable
>> as-is because the new warning is behind the same warning flag as an
>> existing high-value warning. So I think we need the new warnings to be
>> moved behind different warning flags (a subgroup of ReturnStackAddress
>> would be OK, but it needs to be independently controllable).
>>
>> If this can be done imminently, that would be OK, but otherwise I think
>> we should temporarily roll this back until it can be moved to a separate
>> warning group.
>>
>> Both, sorry for the breakage!
>>>
>>> Am Do., 22. Aug. 2019 um 19:47 Uhr schrieb Richard Smith <
>>> rich...@metafoo.co.uk>:
>>>
 Reverted in r369677.

 On Thu, 22 Aug 2019 at 10:34, Richard Smith 
 wrote:

> Hi Matthias,
>
> This introduces false positives into -Wreturn-stack-address for an
> example such as:
>
> #include 
>
> std::vector::iterator downcast_to(std::vector::iterator
> value) {
>   return *
> }
>
> 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).
>
> 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.
>
> Thanks, and sorry for the trouble.
>
> On Wed, 21 Aug 2019 at 15:07, Matthias Gehre via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>> Author: mgehre
>> Date: Wed Aug 21 15:08:59 2019
>> New Revision: 369591
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=369591=rev
>> Log:
>> [LifetimeAnalysis] Support more STL idioms (template forward
>> declaration and DependentNameType)
>>
>> Summary:
>> This fixes inference of gsl::Pointer on std::set::iterator with
>> libstdc++ (the typedef for iterator
>> on the template is a DependentNameType - we can only put the
>> gsl::Pointer attribute
>> on the underlaying record after instantiation)
>>
>> inference of gsl::Pointer on std::vector::iterator with libc++ (the
>> class was forward-declared,
>> we added the gsl::Pointer on the canonical decl (the forward decl),
>> and later when the
>> template was instantiated, there was no attribute on the definition
>> so it was not instantiated).
>>
>> and a duplicate gsl::Pointer on some class with libstdc++ (we first
>> added an attribute to
>> a incomplete instantiation, and then another was copied from the
>> template definition
>> when the instantiation was completed).
>>
>> We now add the attributes to all redeclarations to fix thos issues
>> and make their usage easier.
>>

Re: r369591 - [LifetimeAnalysis] Support more STL idioms (template forward declaration and DependentNameType)

2019-08-23 Thread Gábor Horváth via cfe-commits 
Hi Richard,

I'll move these behind a flag today. Moving forward, it would be great to
have a way to dogfood those warnings without blocking you. We do run them
over ~340 open source projects regularly, but clearly that is not enough :)

Thanks,
Gabor

On Fri, 23 Aug 2019 at 13:03, Richard Smith  wrote:

> On Thu, 22 Aug 2019 at 13:05, Matthias Gehre via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>> Hi Diana,
>> hi Richard,
>>
>> thank you for the feedback!
>>
>> Diana,
>> I remember that some gcc version had issues with raw string literals
>> inside macros. I'll fix that to use normal
>> string literals instead.
>>
>> Richard,
>> We are definitely want the gsl::Pointer-based warnings that are enabled
>> by default to be free of any false-positives.
>> As you expected, this is showing a problem we have in another part of our
>> analysis, and we will fix it before landing
>> this PR again.
>>
>
> It looks like this revert wasn't enough to unblock us. We're currently
> unable to release compilers due to the scale of the new enabled-by-default
> diagnostics produced by these warnings, and we're not happy about turning
> off the existing (zero false positives) warning flags here in order to
> unblock our releases, because they're so valuable in catching errors. I'd
> expect others will hit similar issues when upgrading Clang. Even if there
> were no false positives in the new warning, it appears to be undeployable
> as-is because the new warning is behind the same warning flag as an
> existing high-value warning. So I think we need the new warnings to be
> moved behind different warning flags (a subgroup of ReturnStackAddress
> would be OK, but it needs to be independently controllable).
>
> If this can be done imminently, that would be OK, but otherwise I think we
> should temporarily roll this back until it can be moved to a separate
> warning group.
>
> Both, sorry for the breakage!
>>
>> Am Do., 22. Aug. 2019 um 19:47 Uhr schrieb Richard Smith <
>> rich...@metafoo.co.uk>:
>>
>>> Reverted in r369677.
>>>
>>> On Thu, 22 Aug 2019 at 10:34, Richard Smith 
>>> wrote:
>>>
 Hi Matthias,

 This introduces false positives into -Wreturn-stack-address for an
 example such as:

 #include 

 std::vector::iterator downcast_to(std::vector::iterator
 value) {
   return *
 }

 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).

 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.

 Thanks, and sorry for the trouble.

 On Wed, 21 Aug 2019 at 15:07, Matthias Gehre via cfe-commits <
 cfe-commits@lists.llvm.org> wrote:

> Author: mgehre
> Date: Wed Aug 21 15:08:59 2019
> New Revision: 369591
>
> URL: http://llvm.org/viewvc/llvm-project?rev=369591=rev
> Log:
> [LifetimeAnalysis] Support more STL idioms (template forward
> declaration and DependentNameType)
>
> Summary:
> This fixes inference of gsl::Pointer on std::set::iterator with
> libstdc++ (the typedef for iterator
> on the template is a DependentNameType - we can only put the
> gsl::Pointer attribute
> on the underlaying record after instantiation)
>
> inference of gsl::Pointer on std::vector::iterator with libc++ (the
> class was forward-declared,
> we added the gsl::Pointer on the canonical decl (the forward decl),
> and later when the
> template was instantiated, there was no attribute on the definition so
> it was not instantiated).
>
> and a duplicate gsl::Pointer on some class with libstdc++ (we first
> added an attribute to
> a incomplete instantiation, and then another was copied from the
> template definition
> when the instantiation was completed).
>
> We now add the attributes to all redeclarations to fix thos issues and
> make their usage easier.
>
> Reviewers: gribozavr
>
> Subscribers: Szelethus, xazax.hun, cfe-commits
>
> Tags: #clang
>
> Differential Revision: https://reviews.llvm.org/D66179
>
> Added:
> cfe/trunk/unittests/Sema/GslOwnerPointerInference.cpp
> Modified:
> cfe/trunk/lib/Sema/SemaAttr.cpp
> cfe/trunk/lib/Sema/SemaDeclAttr.cpp
> cfe/trunk/lib/Sema/SemaInit.cpp
> cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp
> cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer-std.cpp
> cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer.cpp
> cfe/trunk/unittests/Sema/CMakeLists.txt
>
> Modified: cfe/trunk/lib/Sema/SemaAttr.cpp
> URL:
>

Re: r369591 - [LifetimeAnalysis] Support more STL idioms (template forward declaration and DependentNameType)

2019-08-23 Thread Richard Smith via cfe-commits 
On Thu, 22 Aug 2019 at 13:05, Matthias Gehre via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> Hi Diana,
> hi Richard,
>
> thank you for the feedback!
>
> Diana,
> I remember that some gcc version had issues with raw string literals
> inside macros. I'll fix that to use normal
> string literals instead.
>
> Richard,
> We are definitely want the gsl::Pointer-based warnings that are enabled by
> default to be free of any false-positives.
> As you expected, this is showing a problem we have in another part of our
> analysis, and we will fix it before landing
> this PR again.
>

It looks like this revert wasn't enough to unblock us. We're currently
unable to release compilers due to the scale of the new enabled-by-default
diagnostics produced by these warnings, and we're not happy about turning
off the existing (zero false positives) warning flags here in order to
unblock our releases, because they're so valuable in catching errors. I'd
expect others will hit similar issues when upgrading Clang. Even if there
were no false positives in the new warning, it appears to be undeployable
as-is because the new warning is behind the same warning flag as an
existing high-value warning. So I think we need the new warnings to be
moved behind different warning flags (a subgroup of ReturnStackAddress
would be OK, but it needs to be independently controllable).

If this can be done imminently, that would be OK, but otherwise I think we
should temporarily roll this back until it can be moved to a separate
warning group.

Both, sorry for the breakage!
>
> Am Do., 22. Aug. 2019 um 19:47 Uhr schrieb Richard Smith <
> rich...@metafoo.co.uk>:
>
>> Reverted in r369677.
>>
>> On Thu, 22 Aug 2019 at 10:34, Richard Smith 
>> wrote:
>>
>>> Hi Matthias,
>>>
>>> This introduces false positives into -Wreturn-stack-address for an
>>> example such as:
>>>
>>> #include 
>>>
>>> std::vector::iterator downcast_to(std::vector::iterator value)
>>> {
>>>   return *
>>> }
>>>
>>> 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).
>>>
>>> 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.
>>>
>>> Thanks, and sorry for the trouble.
>>>
>>> On Wed, 21 Aug 2019 at 15:07, Matthias Gehre via cfe-commits <
>>> cfe-commits@lists.llvm.org> wrote:
>>>
 Author: mgehre
 Date: Wed Aug 21 15:08:59 2019
 New Revision: 369591

 URL: http://llvm.org/viewvc/llvm-project?rev=369591=rev
 Log:
 [LifetimeAnalysis] Support more STL idioms (template forward
 declaration and DependentNameType)

 Summary:
 This fixes inference of gsl::Pointer on std::set::iterator with
 libstdc++ (the typedef for iterator
 on the template is a DependentNameType - we can only put the
 gsl::Pointer attribute
 on the underlaying record after instantiation)

 inference of gsl::Pointer on std::vector::iterator with libc++ (the
 class was forward-declared,
 we added the gsl::Pointer on the canonical decl (the forward decl), and
 later when the
 template was instantiated, there was no attribute on the definition so
 it was not instantiated).

 and a duplicate gsl::Pointer on some class with libstdc++ (we first
 added an attribute to
 a incomplete instantiation, and then another was copied from the
 template definition
 when the instantiation was completed).

 We now add the attributes to all redeclarations to fix thos issues and
 make their usage easier.

 Reviewers: gribozavr

 Subscribers: Szelethus, xazax.hun, cfe-commits

 Tags: #clang

 Differential Revision: https://reviews.llvm.org/D66179

 Added:
 cfe/trunk/unittests/Sema/GslOwnerPointerInference.cpp
 Modified:
 cfe/trunk/lib/Sema/SemaAttr.cpp
 cfe/trunk/lib/Sema/SemaDeclAttr.cpp
 cfe/trunk/lib/Sema/SemaInit.cpp
 cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp
 cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer-std.cpp
 cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer.cpp
 cfe/trunk/unittests/Sema/CMakeLists.txt

 Modified: cfe/trunk/lib/Sema/SemaAttr.cpp
 URL:
 http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaAttr.cpp?rev=369591=369590=369591=diff

 ==
 --- cfe/trunk/lib/Sema/SemaAttr.cpp (original)
 +++ cfe/trunk/lib/Sema/SemaAttr.cpp Wed Aug 21 15:08:59 2019
 @@ -88,13 +88,11 @@ void Sema::AddMsStructLayoutForRecord(Re
  template 
  static void addGslOwnerPointerAttributeIfNotExisting(ASTContext
 ,

Re: r369591 - [LifetimeAnalysis] Support more STL idioms (template forward declaration and DependentNameType)

2019-08-22 Thread Matthias Gehre via cfe-commits 
Hi Diana,
hi Richard,

thank you for the feedback!

Diana,
I remember that some gcc version had issues with raw string literals inside
macros. I'll fix that to use normal
string literals instead.

Richard,
We are definitely want the gsl::Pointer-based warnings that are enabled by
default to be free of any false-positives.
As you expected, this is showing a problem we have in another part of our
analysis, and we will fix it before landing
this PR again.

Both, sorry for the breakage!

Am Do., 22. Aug. 2019 um 19:47 Uhr schrieb Richard Smith <
rich...@metafoo.co.uk>:

> Reverted in r369677.
>
> On Thu, 22 Aug 2019 at 10:34, Richard Smith  wrote:
>
>> Hi Matthias,
>>
>> This introduces false positives into -Wreturn-stack-address for an
>> example such as:
>>
>> #include 
>>
>> std::vector::iterator downcast_to(std::vector::iterator value) {
>>   return *
>> }
>>
>> 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).
>>
>> 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.
>>
>> Thanks, and sorry for the trouble.
>>
>> On Wed, 21 Aug 2019 at 15:07, Matthias Gehre via cfe-commits <
>> cfe-commits@lists.llvm.org> wrote:
>>
>>> Author: mgehre
>>> Date: Wed Aug 21 15:08:59 2019
>>> New Revision: 369591
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=369591=rev
>>> Log:
>>> [LifetimeAnalysis] Support more STL idioms (template forward declaration
>>> and DependentNameType)
>>>
>>> Summary:
>>> This fixes inference of gsl::Pointer on std::set::iterator with
>>> libstdc++ (the typedef for iterator
>>> on the template is a DependentNameType - we can only put the
>>> gsl::Pointer attribute
>>> on the underlaying record after instantiation)
>>>
>>> inference of gsl::Pointer on std::vector::iterator with libc++ (the
>>> class was forward-declared,
>>> we added the gsl::Pointer on the canonical decl (the forward decl), and
>>> later when the
>>> template was instantiated, there was no attribute on the definition so
>>> it was not instantiated).
>>>
>>> and a duplicate gsl::Pointer on some class with libstdc++ (we first
>>> added an attribute to
>>> a incomplete instantiation, and then another was copied from the
>>> template definition
>>> when the instantiation was completed).
>>>
>>> We now add the attributes to all redeclarations to fix thos issues and
>>> make their usage easier.
>>>
>>> Reviewers: gribozavr
>>>
>>> Subscribers: Szelethus, xazax.hun, cfe-commits
>>>
>>> Tags: #clang
>>>
>>> Differential Revision: https://reviews.llvm.org/D66179
>>>
>>> Added:
>>> cfe/trunk/unittests/Sema/GslOwnerPointerInference.cpp
>>> Modified:
>>> cfe/trunk/lib/Sema/SemaAttr.cpp
>>> cfe/trunk/lib/Sema/SemaDeclAttr.cpp
>>> cfe/trunk/lib/Sema/SemaInit.cpp
>>> cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp
>>> cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer-std.cpp
>>> cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer.cpp
>>> cfe/trunk/unittests/Sema/CMakeLists.txt
>>>
>>> Modified: cfe/trunk/lib/Sema/SemaAttr.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaAttr.cpp?rev=369591=369590=369591=diff
>>>
>>> ==
>>> --- cfe/trunk/lib/Sema/SemaAttr.cpp (original)
>>> +++ cfe/trunk/lib/Sema/SemaAttr.cpp Wed Aug 21 15:08:59 2019
>>> @@ -88,13 +88,11 @@ void Sema::AddMsStructLayoutForRecord(Re
>>>  template 
>>>  static void addGslOwnerPointerAttributeIfNotExisting(ASTContext
>>> ,
>>>   CXXRecordDecl
>>> *Record) {
>>> -  CXXRecordDecl *Canonical = Record->getCanonicalDecl();
>>> -  if (Canonical->hasAttr() ||
>>> Canonical->hasAttr())
>>> +  if (Record->hasAttr() || Record->hasAttr())
>>>  return;
>>>
>>> -  Canonical->addAttr(::new (Context) Attribute(SourceRange{}, Context,
>>> -   /*DerefType*/ nullptr,
>>> -   /*Spelling=*/0));
>>> +  for (Decl *Redecl : Record->redecls())
>>> +Redecl->addAttr(Attribute::CreateImplicit(Context,
>>> /*DerefType=*/nullptr));
>>>  }
>>>
>>>  void Sema::inferGslPointerAttribute(NamedDecl *ND,
>>> @@ -189,8 +187,7 @@ void Sema::inferGslOwnerPointerAttribute
>>>
>>>// Handle classes that directly appear in std namespace.
>>>if (Record->isInStdNamespace()) {
>>> -CXXRecordDecl *Canonical = Record->getCanonicalDecl();
>>> -if (Canonical->hasAttr() ||
>>> Canonical->hasAttr())
>>> +if (Record->hasAttr() || Record->hasAttr())
>>>return;
>>>
>>>  if (StdOwners.count(Record->getName()))
>>>
>>> Modified: cfe/trunk/lib/Sema/SemaDeclAttr.cpp

Re: r369591 - [LifetimeAnalysis] Support more STL idioms (template forward declaration and DependentNameType)

2019-08-22 Thread Richard Smith via cfe-commits 
Reverted in r369677.

On Thu, 22 Aug 2019 at 10:34, Richard Smith  wrote:

> Hi Matthias,
>
> This introduces false positives into -Wreturn-stack-address for an example
> such as:
>
> #include 
>
> std::vector::iterator downcast_to(std::vector::iterator value) {
>   return *
> }
>
> 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).
>
> 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.
>
> Thanks, and sorry for the trouble.
>
> On Wed, 21 Aug 2019 at 15:07, Matthias Gehre via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>> Author: mgehre
>> Date: Wed Aug 21 15:08:59 2019
>> New Revision: 369591
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=369591=rev
>> Log:
>> [LifetimeAnalysis] Support more STL idioms (template forward declaration
>> and DependentNameType)
>>
>> Summary:
>> This fixes inference of gsl::Pointer on std::set::iterator with libstdc++
>> (the typedef for iterator
>> on the template is a DependentNameType - we can only put the gsl::Pointer
>> attribute
>> on the underlaying record after instantiation)
>>
>> inference of gsl::Pointer on std::vector::iterator with libc++ (the class
>> was forward-declared,
>> we added the gsl::Pointer on the canonical decl (the forward decl), and
>> later when the
>> template was instantiated, there was no attribute on the definition so it
>> was not instantiated).
>>
>> and a duplicate gsl::Pointer on some class with libstdc++ (we first added
>> an attribute to
>> a incomplete instantiation, and then another was copied from the template
>> definition
>> when the instantiation was completed).
>>
>> We now add the attributes to all redeclarations to fix thos issues and
>> make their usage easier.
>>
>> Reviewers: gribozavr
>>
>> Subscribers: Szelethus, xazax.hun, cfe-commits
>>
>> Tags: #clang
>>
>> Differential Revision: https://reviews.llvm.org/D66179
>>
>> Added:
>> cfe/trunk/unittests/Sema/GslOwnerPointerInference.cpp
>> Modified:
>> cfe/trunk/lib/Sema/SemaAttr.cpp
>> cfe/trunk/lib/Sema/SemaDeclAttr.cpp
>> cfe/trunk/lib/Sema/SemaInit.cpp
>> cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp
>> cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer-std.cpp
>> cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer.cpp
>> cfe/trunk/unittests/Sema/CMakeLists.txt
>>
>> Modified: cfe/trunk/lib/Sema/SemaAttr.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaAttr.cpp?rev=369591=369590=369591=diff
>>
>> ==
>> --- cfe/trunk/lib/Sema/SemaAttr.cpp (original)
>> +++ cfe/trunk/lib/Sema/SemaAttr.cpp Wed Aug 21 15:08:59 2019
>> @@ -88,13 +88,11 @@ void Sema::AddMsStructLayoutForRecord(Re
>>  template 
>>  static void addGslOwnerPointerAttributeIfNotExisting(ASTContext ,
>>   CXXRecordDecl
>> *Record) {
>> -  CXXRecordDecl *Canonical = Record->getCanonicalDecl();
>> -  if (Canonical->hasAttr() ||
>> Canonical->hasAttr())
>> +  if (Record->hasAttr() || Record->hasAttr())
>>  return;
>>
>> -  Canonical->addAttr(::new (Context) Attribute(SourceRange{}, Context,
>> -   /*DerefType*/ nullptr,
>> -   /*Spelling=*/0));
>> +  for (Decl *Redecl : Record->redecls())
>> +Redecl->addAttr(Attribute::CreateImplicit(Context,
>> /*DerefType=*/nullptr));
>>  }
>>
>>  void Sema::inferGslPointerAttribute(NamedDecl *ND,
>> @@ -189,8 +187,7 @@ void Sema::inferGslOwnerPointerAttribute
>>
>>// Handle classes that directly appear in std namespace.
>>if (Record->isInStdNamespace()) {
>> -CXXRecordDecl *Canonical = Record->getCanonicalDecl();
>> -if (Canonical->hasAttr() ||
>> Canonical->hasAttr())
>> +if (Record->hasAttr() || Record->hasAttr())
>>return;
>>
>>  if (StdOwners.count(Record->getName()))
>>
>> Modified: cfe/trunk/lib/Sema/SemaDeclAttr.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclAttr.cpp?rev=369591=369590=369591=diff
>>
>> ==
>> --- cfe/trunk/lib/Sema/SemaDeclAttr.cpp (original)
>> +++ cfe/trunk/lib/Sema/SemaDeclAttr.cpp Wed Aug 21 15:08:59 2019
>> @@ -4592,9 +4592,11 @@ static void handleLifetimeCategoryAttr(S
>>}
>>return;
>>  }
>> -D->addAttr(::new (S.Context)
>> -   OwnerAttr(AL.getRange(), S.Context, DerefTypeLoc,
>> - AL.getAttributeSpellingListIndex()));
>> +for (Decl *Redecl : D->redecls()) {
>> +  Redecl->addAttr(::new (S.Context)
>>

Re: r369591 - [LifetimeAnalysis] Support more STL idioms (template forward declaration and DependentNameType)

2019-08-22 Thread Richard Smith via cfe-commits 
Hi Matthias,

This introduces false positives into -Wreturn-stack-address for an example
such as:

#include 

std::vector::iterator downcast_to(std::vector::iterator value) {
  return *
}

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).

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.

Thanks, and sorry for the trouble.

On Wed, 21 Aug 2019 at 15:07, Matthias Gehre via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> Author: mgehre
> Date: Wed Aug 21 15:08:59 2019
> New Revision: 369591
>
> URL: http://llvm.org/viewvc/llvm-project?rev=369591=rev
> Log:
> [LifetimeAnalysis] Support more STL idioms (template forward declaration
> and DependentNameType)
>
> Summary:
> This fixes inference of gsl::Pointer on std::set::iterator with libstdc++
> (the typedef for iterator
> on the template is a DependentNameType - we can only put the gsl::Pointer
> attribute
> on the underlaying record after instantiation)
>
> inference of gsl::Pointer on std::vector::iterator with libc++ (the class
> was forward-declared,
> we added the gsl::Pointer on the canonical decl (the forward decl), and
> later when the
> template was instantiated, there was no attribute on the definition so it
> was not instantiated).
>
> and a duplicate gsl::Pointer on some class with libstdc++ (we first added
> an attribute to
> a incomplete instantiation, and then another was copied from the template
> definition
> when the instantiation was completed).
>
> We now add the attributes to all redeclarations to fix thos issues and
> make their usage easier.
>
> Reviewers: gribozavr
>
> Subscribers: Szelethus, xazax.hun, cfe-commits
>
> Tags: #clang
>
> Differential Revision: https://reviews.llvm.org/D66179
>
> Added:
> cfe/trunk/unittests/Sema/GslOwnerPointerInference.cpp
> Modified:
> cfe/trunk/lib/Sema/SemaAttr.cpp
> cfe/trunk/lib/Sema/SemaDeclAttr.cpp
> cfe/trunk/lib/Sema/SemaInit.cpp
> cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp
> cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer-std.cpp
> cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer.cpp
> cfe/trunk/unittests/Sema/CMakeLists.txt
>
> Modified: cfe/trunk/lib/Sema/SemaAttr.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaAttr.cpp?rev=369591=369590=369591=diff
>
> ==
> --- cfe/trunk/lib/Sema/SemaAttr.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaAttr.cpp Wed Aug 21 15:08:59 2019
> @@ -88,13 +88,11 @@ void Sema::AddMsStructLayoutForRecord(Re
>  template 
>  static void addGslOwnerPointerAttributeIfNotExisting(ASTContext ,
>   CXXRecordDecl
> *Record) {
> -  CXXRecordDecl *Canonical = Record->getCanonicalDecl();
> -  if (Canonical->hasAttr() ||
> Canonical->hasAttr())
> +  if (Record->hasAttr() || Record->hasAttr())
>  return;
>
> -  Canonical->addAttr(::new (Context) Attribute(SourceRange{}, Context,
> -   /*DerefType*/ nullptr,
> -   /*Spelling=*/0));
> +  for (Decl *Redecl : Record->redecls())
> +Redecl->addAttr(Attribute::CreateImplicit(Context,
> /*DerefType=*/nullptr));
>  }
>
>  void Sema::inferGslPointerAttribute(NamedDecl *ND,
> @@ -189,8 +187,7 @@ void Sema::inferGslOwnerPointerAttribute
>
>// Handle classes that directly appear in std namespace.
>if (Record->isInStdNamespace()) {
> -CXXRecordDecl *Canonical = Record->getCanonicalDecl();
> -if (Canonical->hasAttr() ||
> Canonical->hasAttr())
> +if (Record->hasAttr() || Record->hasAttr())
>return;
>
>  if (StdOwners.count(Record->getName()))
>
> Modified: cfe/trunk/lib/Sema/SemaDeclAttr.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclAttr.cpp?rev=369591=369590=369591=diff
>
> ==
> --- cfe/trunk/lib/Sema/SemaDeclAttr.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaDeclAttr.cpp Wed Aug 21 15:08:59 2019
> @@ -4592,9 +4592,11 @@ static void handleLifetimeCategoryAttr(S
>}
>return;
>  }
> -D->addAttr(::new (S.Context)
> -   OwnerAttr(AL.getRange(), S.Context, DerefTypeLoc,
> - AL.getAttributeSpellingListIndex()));
> +for (Decl *Redecl : D->redecls()) {
> +  Redecl->addAttr(::new (S.Context)
> +  OwnerAttr(AL.getRange(), S.Context,
> DerefTypeLoc,
> +AL.getAttributeSpellingListIndex()));
> +}
>} else {
>  if (checkAttrMutualExclusion(S, D,

Re: r369591 - [LifetimeAnalysis] Support more STL idioms (template forward declaration and DependentNameType)

2019-08-22 Thread Diana Picus via cfe-commits 
Hi Matthias,

It seems this is breaking some of our AArch64 bots:
http://lab.llvm.org:8011/builders/clang-cmake-aarch64-quick/builds/19691
http://lab.llvm.org:8011/builders/clang-cmake-aarch64-global-isel/builds/12022

You probably didn't notice it because something else also failed in
that build and then the bots just remained red.
I haven't looked into too much detail, but AFAICT this is only
happening with gcc-7 (our other AArch64 bots which run clang-6 seem to
be ok with it). Is this a problem with the compiler or with the code?

Thanks,
Diana

On Thu, 22 Aug 2019 at 00:07, Matthias Gehre via cfe-commits
 wrote:
>
> Author: mgehre
> Date: Wed Aug 21 15:08:59 2019
> New Revision: 369591
>
> URL: http://llvm.org/viewvc/llvm-project?rev=369591=rev
> Log:
> [LifetimeAnalysis] Support more STL idioms (template forward declaration and 
> DependentNameType)
>
> Summary:
> This fixes inference of gsl::Pointer on std::set::iterator with libstdc++ 
> (the typedef for iterator
> on the template is a DependentNameType - we can only put the gsl::Pointer 
> attribute
> on the underlaying record after instantiation)
>
> inference of gsl::Pointer on std::vector::iterator with libc++ (the class was 
> forward-declared,
> we added the gsl::Pointer on the canonical decl (the forward decl), and later 
> when the
> template was instantiated, there was no attribute on the definition so it was 
> not instantiated).
>
> and a duplicate gsl::Pointer on some class with libstdc++ (we first added an 
> attribute to
> a incomplete instantiation, and then another was copied from the template 
> definition
> when the instantiation was completed).
>
> We now add the attributes to all redeclarations to fix thos issues and make 
> their usage easier.
>
> Reviewers: gribozavr
>
> Subscribers: Szelethus, xazax.hun, cfe-commits
>
> Tags: #clang
>
> Differential Revision: https://reviews.llvm.org/D66179
>
> Added:
> cfe/trunk/unittests/Sema/GslOwnerPointerInference.cpp
> Modified:
> cfe/trunk/lib/Sema/SemaAttr.cpp
> cfe/trunk/lib/Sema/SemaDeclAttr.cpp
> cfe/trunk/lib/Sema/SemaInit.cpp
> cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp
> cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer-std.cpp
> cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer.cpp
> cfe/trunk/unittests/Sema/CMakeLists.txt
>
> Modified: cfe/trunk/lib/Sema/SemaAttr.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaAttr.cpp?rev=369591=369590=369591=diff
> ==
> --- cfe/trunk/lib/Sema/SemaAttr.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaAttr.cpp Wed Aug 21 15:08:59 2019
> @@ -88,13 +88,11 @@ void Sema::AddMsStructLayoutForRecord(Re
>  template 
>  static void addGslOwnerPointerAttributeIfNotExisting(ASTContext ,
>   CXXRecordDecl *Record) {
> -  CXXRecordDecl *Canonical = Record->getCanonicalDecl();
> -  if (Canonical->hasAttr() || Canonical->hasAttr())
> +  if (Record->hasAttr() || Record->hasAttr())
>  return;
>
> -  Canonical->addAttr(::new (Context) Attribute(SourceRange{}, Context,
> -   /*DerefType*/ nullptr,
> -   /*Spelling=*/0));
> +  for (Decl *Redecl : Record->redecls())
> +Redecl->addAttr(Attribute::CreateImplicit(Context, 
> /*DerefType=*/nullptr));
>  }
>
>  void Sema::inferGslPointerAttribute(NamedDecl *ND,
> @@ -189,8 +187,7 @@ void Sema::inferGslOwnerPointerAttribute
>
>// Handle classes that directly appear in std namespace.
>if (Record->isInStdNamespace()) {
> -CXXRecordDecl *Canonical = Record->getCanonicalDecl();
> -if (Canonical->hasAttr() || Canonical->hasAttr())
> +if (Record->hasAttr() || Record->hasAttr())
>return;
>
>  if (StdOwners.count(Record->getName()))
>
> Modified: cfe/trunk/lib/Sema/SemaDeclAttr.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclAttr.cpp?rev=369591=369590=369591=diff
> ==
> --- cfe/trunk/lib/Sema/SemaDeclAttr.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaDeclAttr.cpp Wed Aug 21 15:08:59 2019
> @@ -4592,9 +4592,11 @@ static void handleLifetimeCategoryAttr(S
>}
>return;
>  }
> -D->addAttr(::new (S.Context)
> -   OwnerAttr(AL.getRange(), S.Context, DerefTypeLoc,
> - AL.getAttributeSpellingListIndex()));
> +for (Decl *Redecl : D->redecls()) {
> +  Redecl->addAttr(::new (S.Context)
> +  OwnerAttr(AL.getRange(), S.Context, DerefTypeLoc,
> +AL.getAttributeSpellingListIndex()));
> +}
>} else {
>  if (checkAttrMutualExclusion(S, D, AL))
>return;
> @@ -4609,9 +4611,11 @@ static void handleLifetimeCategoryAttr(S
>}
>return;
>  }
> -D->addAttr(::new (S.Context)
>

r369591 - [LifetimeAnalysis] Support more STL idioms (template forward declaration and DependentNameType)

2019-08-21 Thread Matthias Gehre via cfe-commits 
Author: mgehre
Date: Wed Aug 21 15:08:59 2019
New Revision: 369591

URL: http://llvm.org/viewvc/llvm-project?rev=369591=rev
Log:
[LifetimeAnalysis] Support more STL idioms (template forward declaration and 
DependentNameType)

Summary:
This fixes inference of gsl::Pointer on std::set::iterator with libstdc++ (the 
typedef for iterator
on the template is a DependentNameType - we can only put the gsl::Pointer 
attribute
on the underlaying record after instantiation)

inference of gsl::Pointer on std::vector::iterator with libc++ (the class was 
forward-declared,
we added the gsl::Pointer on the canonical decl (the forward decl), and later 
when the
template was instantiated, there was no attribute on the definition so it was 
not instantiated).

and a duplicate gsl::Pointer on some class with libstdc++ (we first added an 
attribute to
a incomplete instantiation, and then another was copied from the template 
definition
when the instantiation was completed).

We now add the attributes to all redeclarations to fix thos issues and make 
their usage easier.

Reviewers: gribozavr

Subscribers: Szelethus, xazax.hun, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D66179

Added:
cfe/trunk/unittests/Sema/GslOwnerPointerInference.cpp
Modified:
cfe/trunk/lib/Sema/SemaAttr.cpp
cfe/trunk/lib/Sema/SemaDeclAttr.cpp
cfe/trunk/lib/Sema/SemaInit.cpp
cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp
cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer-std.cpp
cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer.cpp
cfe/trunk/unittests/Sema/CMakeLists.txt

Modified: cfe/trunk/lib/Sema/SemaAttr.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaAttr.cpp?rev=369591=369590=369591=diff
==
--- cfe/trunk/lib/Sema/SemaAttr.cpp (original)
+++ cfe/trunk/lib/Sema/SemaAttr.cpp Wed Aug 21 15:08:59 2019
@@ -88,13 +88,11 @@ void Sema::AddMsStructLayoutForRecord(Re
 template 
 static void addGslOwnerPointerAttributeIfNotExisting(ASTContext ,
  CXXRecordDecl *Record) {
-  CXXRecordDecl *Canonical = Record->getCanonicalDecl();
-  if (Canonical->hasAttr() || Canonical->hasAttr())
+  if (Record->hasAttr() || Record->hasAttr())
 return;
 
-  Canonical->addAttr(::new (Context) Attribute(SourceRange{}, Context,
-   /*DerefType*/ nullptr,
-   /*Spelling=*/0));
+  for (Decl *Redecl : Record->redecls())
+Redecl->addAttr(Attribute::CreateImplicit(Context, /*DerefType=*/nullptr));
 }
 
 void Sema::inferGslPointerAttribute(NamedDecl *ND,
@@ -189,8 +187,7 @@ void Sema::inferGslOwnerPointerAttribute
 
   // Handle classes that directly appear in std namespace.
   if (Record->isInStdNamespace()) {
-CXXRecordDecl *Canonical = Record->getCanonicalDecl();
-if (Canonical->hasAttr() || Canonical->hasAttr())
+if (Record->hasAttr() || Record->hasAttr())
   return;
 
 if (StdOwners.count(Record->getName()))

Modified: cfe/trunk/lib/Sema/SemaDeclAttr.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclAttr.cpp?rev=369591=369590=369591=diff
==
--- cfe/trunk/lib/Sema/SemaDeclAttr.cpp (original)
+++ cfe/trunk/lib/Sema/SemaDeclAttr.cpp Wed Aug 21 15:08:59 2019
@@ -4592,9 +4592,11 @@ static void handleLifetimeCategoryAttr(S
   }
   return;
 }
-D->addAttr(::new (S.Context)
-   OwnerAttr(AL.getRange(), S.Context, DerefTypeLoc,
- AL.getAttributeSpellingListIndex()));
+for (Decl *Redecl : D->redecls()) {
+  Redecl->addAttr(::new (S.Context)
+  OwnerAttr(AL.getRange(), S.Context, DerefTypeLoc,
+AL.getAttributeSpellingListIndex()));
+}
   } else {
 if (checkAttrMutualExclusion(S, D, AL))
   return;
@@ -4609,9 +4611,11 @@ static void handleLifetimeCategoryAttr(S
   }
   return;
 }
-D->addAttr(::new (S.Context)
-   PointerAttr(AL.getRange(), S.Context, DerefTypeLoc,
-   AL.getAttributeSpellingListIndex()));
+for (Decl *Redecl : D->redecls()) {
+  Redecl->addAttr(::new (S.Context)
+  PointerAttr(AL.getRange(), S.Context, DerefTypeLoc,
+  AL.getAttributeSpellingListIndex()));
+}
   }
 }
 

Modified: cfe/trunk/lib/Sema/SemaInit.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaInit.cpp?rev=369591=369590=369591=diff
==
--- cfe/trunk/lib/Sema/SemaInit.cpp (original)
+++ cfe/trunk/lib/Sema/SemaInit.cpp Wed Aug 21 15:08:59 2019
@@ -6561,7 +6561,7 @@ static void visitLocalsRetainedByReferen
 
 template  static bool