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;
}

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.

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

Johnathan Roelofs is correct when he wrote:

Doesn't the:

   template <typename X>
   friend class IntrusiveRefCntPtr;

here: llvm/IntrusiveRefCntPtr.h at master · llvm-mirror/llvm · GitHub 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.

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

Clang is pretty good about warning about the technical use of those functions even if nrvo kicks in and they’re never actually needed - older versions of gcc used to not do this checking, but to I believe now even gcc catches this.

There may be some holes here, of course…