Mark Millard
2015-Mar-15 00:36 UTC
[LLVMdev] FreeBSD's 11.0-CURRENT contrib/llvm/include/llvm/ADT/IntrusiveRefCntPtr.h's IntrusiveRefCntPtr and its use violates C++ privacy rules
When trying to build the 11.0-CURRENT clang 3.5 on powerpc64 I ran into a violation of C++ accessibility rules (for private) that stopped the compile. So not the usual defect category. (This was a bootstrapping procedure as powerpc/powerpc64 FreeBSD world’s clang has an odd status and getting from 3.4 under 10.1-STABLE to 3.5 on 11.0-CURRENT is not automatic.) Given the language rules and difficulty interpreting them I figured an open discussion area might be the better place to go until/unless someone from llvm agrees with the information. I'm not sure what priority being non-standard has for points other compilers have trouble with for the code. I have looked on the web and Revision 232289 of IntrusiveRefCntPtr.h still has the same code structure for the issue. The problem... FreeBSD 11.0-CURRENT's contrib/llvm/include/llvm/ADT/IntrusiveRefCntPtr.h has... template <typename T> class IntrusiveRefCntPtr { T* Obj; public: ... template <class X> IntrusiveRefCntPtr(IntrusiveRefCntPtr<X>&& S) : Obj(S.get()) { S.Obj = 0; } ... } To first illustrate a (partial) but-simpler-to-follow example use that would show the problem with the above: using Ta = ...; using Tb = ...;// Not the same type, more than just a name change. // Note that private members of IntrusiveRefCntPtr<Ta> // are not (should not be) accessible to // IntrusiveRefCntPtr<Tb> methods (and vice-versa). IntrusiveRefCntPtr<Ta> a{} IntrusiveRefCntPtr<Tb> b{a}; // We then would have a usage where an example of: IntrusiveRefCntPtr<Tb>::IntrusiveRefCntPtr is then trying to access an example of IntrusiveRefCntPtr<Ta>'s Obj private member. It would take a friend relationship to be established to allow the cross-type access to Obj. The code in contrib/llvm/tools/clang/lib/Frontend/ChainedIncludesSource.cpp has such a use and so makes an instance of the violation of the language rules in the actual code. The function clang::createChainedIncludesSourceIt uses classes... class ChainedIncludesSource : public ExternalSemaSource where... class ExternalSemaSource : public ExternalASTSource where... class ExternalASTSource : public RefCountedBase<ExternalASTSource> where... template <class Derived> class RefCountedBase; and it uses both of the following types... IntrusiveRefCntPtr<ExternalSemaSource> and... IntrusiveRefCntPtr<ChainedIncludesSource> In fact IntrusiveRefCntPtr<ChainedIncludesSource> is the return-expresison type for the following routine that has return type IntrusiveRefCntPtr<ExternalSemaSource>... IntrusiveRefCntPtr<ExternalSemaSource> clang::createChainedIncludesSource( CompilerInstance &CI, IntrusiveRefCntPtr<ExternalSemaSource> &Reader) { ... IntrusiveRefCntPtr<ChainedIncludesSource> source(new ChainedIncludesSource()); ... return source; } ==Mark Millard markmi at dsl-only.net
David Blaikie
2015-Mar-15 00:55 UTC
[LLVMdev] FreeBSD's 11.0-CURRENT contrib/llvm/include/llvm/ADT/IntrusiveRefCntPtr.h's IntrusiveRefCntPtr and its use violates C++ privacy rules
I assume there's a bit more to it than this - otherwise we would've discovered it earlier? Which compiler is rejecting this code? Do you have a reduced example of something that clang accepts and this other compiler rejects? On Sat, Mar 14, 2015 at 5:36 PM, Mark Millard <markmi at dsl-only.net> wrote:> When trying to build the 11.0-CURRENT clang 3.5 on powerpc64 I ran into a > violation of C++ accessibility rules (for private) that stopped the > compile. So not the usual defect category. (This was a bootstrapping > procedure as powerpc/powerpc64 FreeBSD world’s clang has an odd status and > getting from 3.4 under 10.1-STABLE to 3.5 on 11.0-CURRENT is not automatic.) > > Given the language rules and difficulty interpreting them I figured an > open discussion area might be the better place to go until/unless someone > from llvm agrees with the information. I'm not sure what priority being > non-standard has for points other compilers have trouble with for the code. > > I have looked on the web and Revision 232289 of IntrusiveRefCntPtr.h still > has the same code structure for the issue. > > > The problem... > > FreeBSD 11.0-CURRENT's contrib/llvm/include/llvm/ADT/IntrusiveRefCntPtr.h > has... > > template <typename T> > class IntrusiveRefCntPtr { > T* Obj; > > public: > ... > template <class X> > IntrusiveRefCntPtr(IntrusiveRefCntPtr<X>&& S) : Obj(S.get()) { > S.Obj = 0; > } > ... > } > > To first illustrate a (partial) but-simpler-to-follow example use that > would show the problem with the above: > > using Ta = ...; > using Tb = ...;// Not the same type, more than just a name change. > > // Note that private members of IntrusiveRefCntPtr<Ta> > // are not (should not be) accessible to > // IntrusiveRefCntPtr<Tb> methods (and vice-versa). > > IntrusiveRefCntPtr<Ta> a{} > > IntrusiveRefCntPtr<Tb> b{a}; > > // We then would have a usage where an example of: > > IntrusiveRefCntPtr<Tb>::IntrusiveRefCntPtr > > is then trying to access an example of > > IntrusiveRefCntPtr<Ta>'s Obj private member. > > It would take a friend relationship to be established to allow the > cross-type access to Obj. > > > The code in > contrib/llvm/tools/clang/lib/Frontend/ChainedIncludesSource.cpp has such a > use and so makes an instance of the violation of the language rules in the > actual code. > > The function clang::createChainedIncludesSourceIt uses classes... > > class ChainedIncludesSource : public ExternalSemaSource > where... > class ExternalSemaSource : public ExternalASTSource > where... > class ExternalASTSource : public RefCountedBase<ExternalASTSource> > where... > template <class Derived> class RefCountedBase; > > and it uses both of the following types... > > IntrusiveRefCntPtr<ExternalSemaSource> > and... > IntrusiveRefCntPtr<ChainedIncludesSource> > > In fact IntrusiveRefCntPtr<ChainedIncludesSource> is the return-expresison > type for the following routine that has return type > IntrusiveRefCntPtr<ExternalSemaSource>... > > IntrusiveRefCntPtr<ExternalSemaSource> clang::createChainedIncludesSource( > CompilerInstance &CI, IntrusiveRefCntPtr<ExternalSemaSource> &Reader) { > ... > IntrusiveRefCntPtr<ChainedIncludesSource> source(new > ChainedIncludesSource()); > ... > return source; > } > > ==> Mark Millard > markmi at dsl-only.net > > > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20150314/d4fd36a1/attachment.html>
Tim Northover
2015-Mar-15 01:25 UTC
[LLVMdev] FreeBSD's 11.0-CURRENT contrib/llvm/include/llvm/ADT/IntrusiveRefCntPtr.h's IntrusiveRefCntPtr and its use violates C++ privacy rules
On 14 March 2015 at 17:55, David Blaikie <dblaikie at gmail.com> wrote:> I assume there's a bit more to it than this - otherwise we would've > discovered it earlier? Which compiler is rejecting this code? Do you have a > reduced example of something that clang accepts and this other compiler > rejects?Probably RVO related. The problematic constructor (and it does seem pointless) is only invoked if an actual copy is needed. Clang takes advantage of RVO even at -O0 and only ever calls llvm::IntrusiveRefCntPtr<clang::ExternalSemaSource>::IntrusiveRefCntPtr(clang::ExternalSemaSource*). If so, I suspect LLVM does need to be changed to work on compilers without RVO too (the standard says implementations *may* perform that optimisation, not that they must). I'd be interested to know what this other compiler was, too. Tim.
Jonathan Roelofs
2015-Mar-15 01:34 UTC
[LLVMdev] FreeBSD's 11.0-CURRENT contrib/llvm/include/llvm/ADT/IntrusiveRefCntPtr.h's IntrusiveRefCntPtr and its use violates C++ privacy rules
On 3/14/15 6:36 PM, Mark Millard wrote:> When trying to build the 11.0-CURRENT clang 3.5 on powerpc64 I ran into a violation of C++ accessibility rules (for private) that stopped the compile. So not the usual defect category. (This was a bootstrapping procedure as powerpc/powerpc64 FreeBSD world’s clang has an odd status and getting from 3.4 under 10.1-STABLE to 3.5 on 11.0-CURRENT is not automatic.) > > Given the language rules and difficulty interpreting them I figured an open discussion area might be the better place to go until/unless someone from llvm agrees with the information. I'm not sure what priority being non-standard has for points other compilers have trouble with for the code. > > I have looked on the web and Revision 232289 of IntrusiveRefCntPtr.h still has the same code structure for the issue. > > > The problem... > > FreeBSD 11.0-CURRENT's contrib/llvm/include/llvm/ADT/IntrusiveRefCntPtr.h has... > > template <typename T> > class IntrusiveRefCntPtr { > T* Obj; > > public: > ... > template <class X> > IntrusiveRefCntPtr(IntrusiveRefCntPtr<X>&& S) : Obj(S.get()) { > S.Obj = 0; > } > ... > } > > To first illustrate a (partial) but-simpler-to-follow example use that would show the problem with the above: > > using Ta = ...; > using Tb = ...;// Not the same type, more than just a name change. > > // Note that private members of IntrusiveRefCntPtr<Ta> > // are not (should not be) accessible to > // IntrusiveRefCntPtr<Tb> methods (and vice-versa). > > IntrusiveRefCntPtr<Ta> a{} > > IntrusiveRefCntPtr<Tb> b{a}; > > // We then would have a usage where an example of: > > IntrusiveRefCntPtr<Tb>::IntrusiveRefCntPtr > > is then trying to access an example of > > IntrusiveRefCntPtr<Ta>'s Obj private member. > > It would take a friend relationship to be established to allow the cross-type access to Obj.Doesn't the: template <typename X> friend class IntrusiveRefCntPtr; here: https://github.com/llvm-mirror/llvm/blob/master/include/llvm/ADT/IntrusiveRefCntPtr.h#L202 take care of that? Jon> > > The code in contrib/llvm/tools/clang/lib/Frontend/ChainedIncludesSource.cpp has such a use and so makes an instance of the violation of the language rules in the actual code. > > The function clang::createChainedIncludesSourceIt uses classes... > > class ChainedIncludesSource : public ExternalSemaSource > where... > class ExternalSemaSource : public ExternalASTSource > where... > class ExternalASTSource : public RefCountedBase<ExternalASTSource> > where... > template <class Derived> class RefCountedBase; > > and it uses both of the following types... > > IntrusiveRefCntPtr<ExternalSemaSource> > and... > IntrusiveRefCntPtr<ChainedIncludesSource> > > In fact IntrusiveRefCntPtr<ChainedIncludesSource> is the return-expresison type for the following routine that has return type IntrusiveRefCntPtr<ExternalSemaSource>... > > IntrusiveRefCntPtr<ExternalSemaSource> clang::createChainedIncludesSource( > CompilerInstance &CI, IntrusiveRefCntPtr<ExternalSemaSource> &Reader) { > ... > IntrusiveRefCntPtr<ChainedIncludesSource> source(new ChainedIncludesSource()); > ... > return source; > } > > ==> Mark Millard > markmi at dsl-only.net > > > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >-- Jon Roelofs jonathan at codesourcery.com CodeSourcery / Mentor Embedded
Mark Millard
2015-Mar-15 01:45 UTC
[LLVMdev] FreeBSD's 11.0-CURRENT contrib/llvm/include/llvm/ADT/IntrusiveRefCntPtr.h's IntrusiveRefCntPtr and its use violates C++ privacy rules
Johnathan Roelofs is correct when he wrote:> Doesn't the: > > template <typename X> > friend class IntrusiveRefCntPtr; > > here: https://github.com/llvm-mirror/llvm/blob/master/include/llvm/ADT/IntrusiveRefCntPtr.h#L202 take care of that?I missed that difference from the FreeBSD source when I looked on the web. And it looks like it has been in place for some time. Sorry for the noise. ==Mark Millard markmi at dsl-only.net On 2015-Mar-14, at 05:55 PM, David Blaikie <dblaikie at gmail.com> wrote: I assume there's a bit more to it than this - otherwise we would've discovered it earlier? Which compiler is rejecting this code? Do you have a reduced example of something that clang accepts and this other compiler rejects? On Sat, Mar 14, 2015 at 5:36 PM, Mark Millard <markmi at dsl-only.net> wrote: When trying to build the 11.0-CURRENT clang 3.5 on powerpc64 I ran into a violation of C++ accessibility rules (for private) that stopped the compile. So not the usual defect category. (This was a bootstrapping procedure as powerpc/powerpc64 FreeBSD world’s clang has an odd status and getting from 3.4 under 10.1-STABLE to 3.5 on 11.0-CURRENT is not automatic.) Given the language rules and difficulty interpreting them I figured an open discussion area might be the better place to go until/unless someone from llvm agrees with the information. I'm not sure what priority being non-standard has for points other compilers have trouble with for the code. I have looked on the web and Revision 232289 of IntrusiveRefCntPtr.h still has the same code structure for the issue. The problem... FreeBSD 11.0-CURRENT's contrib/llvm/include/llvm/ADT/IntrusiveRefCntPtr.h has... template <typename T> class IntrusiveRefCntPtr { T* Obj; public: ... template <class X> IntrusiveRefCntPtr(IntrusiveRefCntPtr<X>&& S) : Obj(S.get()) { S.Obj = 0; } ... } To first illustrate a (partial) but-simpler-to-follow example use that would show the problem with the above: using Ta = ...; using Tb = ...;// Not the same type, more than just a name change. // Note that private members of IntrusiveRefCntPtr<Ta> // are not (should not be) accessible to // IntrusiveRefCntPtr<Tb> methods (and vice-versa). IntrusiveRefCntPtr<Ta> a{} IntrusiveRefCntPtr<Tb> b{a}; // We then would have a usage where an example of: IntrusiveRefCntPtr<Tb>::IntrusiveRefCntPtr is then trying to access an example of IntrusiveRefCntPtr<Ta>'s Obj private member. It would take a friend relationship to be established to allow the cross-type access to Obj. The code in contrib/llvm/tools/clang/lib/Frontend/ChainedIncludesSource.cpp has such a use and so makes an instance of the violation of the language rules in the actual code. The function clang::createChainedIncludesSourceIt uses classes... class ChainedIncludesSource : public ExternalSemaSource where... class ExternalSemaSource : public ExternalASTSource where... class ExternalASTSource : public RefCountedBase<ExternalASTSource> where... template <class Derived> class RefCountedBase; and it uses both of the following types... IntrusiveRefCntPtr<ExternalSemaSource> and... IntrusiveRefCntPtr<ChainedIncludesSource> In fact IntrusiveRefCntPtr<ChainedIncludesSource> is the return-expresison type for the following routine that has return type IntrusiveRefCntPtr<ExternalSemaSource>... IntrusiveRefCntPtr<ExternalSemaSource> clang::createChainedIncludesSource( CompilerInstance &CI, IntrusiveRefCntPtr<ExternalSemaSource> &Reader) { ... IntrusiveRefCntPtr<ChainedIncludesSource> source(new ChainedIncludesSource()); ... return source; } ==Mark Millard markmi at dsl-only.net _______________________________________________ LLVM Developers mailing list LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev