[cfe-commits] [PATCH] Add _Atomic specifier for atomic types

[Jeffrey Yasskin]

[cc Cary because of the dwarf question.]

On Mon, Oct 3, 2011 at 7:53 PM, Eli Friedman <eli.friedman at gmail.com> wrote:
> Per subject; after some discussions with John, I decided that we
> really want to be prepared for C1x atomics, and being able to use
> _Atomic in the implementation of <atomic> will make the implementation
> of <atomic> more flexible.  This patch follows the syntax from the C1x
> draft which will hopefully stick: _Atomic(int*) specifies an atomic
> pointer to an integer, etc.

The C1x draft makes _Atomic(type) a type specifier and plain _Atomic a
type qualifier. It looks like your patch only implements the type
specifier. Was that intentional?

> After this patch is committed, I plan to update my other patch (for
> the __atomic_* builtins) to use these atomic types, which will make
> the implementation of <atomic> completely straightforward (although
> the implementation might want to use a bit of trickery to make sure
> errors mentioning _Atomic don't leak out).  I'm still not quite sure
> what the runtime support is going to look like, so the initial
> implementation will probably error out on non-lock-free atomics.

Sounds good to me. Comments inline.

> I don't plan on working on <stdatomic.h> at the moment, but it should
> be relatively easy to implement.
>
> Any comments would be appreciated.


> Index: test/Sema/atomic-type.c
> ===================================================================
> --- test/Sema/atomic-type.c	(revision 0)
> +++ test/Sema/atomic-type.c	(revision 0)
> @@ -0,0 +1,12 @@
> +// RUN: %clang_cc1 %s -verify -fsyntax-only
> +
> +// Basic parsing/Sema tests for _Atomic
> +// (No operations are actually supported on objects of this type yet.)
> +_Atomic(int) t1;
> +_Atomic(int) *t2 = &t1;
> +void testf(void*);
> +void f(void) {
> +  _Atomic(_Atomic(int)*) t3;
> +  _Atomic(_Atomic(int)*) *t4[2] = { &t3, 0 };

If you meant to implement the type qualifier, you should add tests for it too.

> +  testf(t4);
> +}
> \ No newline at end of file

^ Newlines!

> Index: include/clang/AST/Type.h
> ===================================================================
> --- include/clang/AST/Type.h	(revision 141016)
> +++ include/clang/AST/Type.h	(working copy)
> @@ -4352,6 +4353,37 @@
>    static bool classof(const ObjCObjectPointerType *) { return true; }
>  };
>
> +class AtomicType : public Type, public llvm::FoldingSetNode {
> +  QualType ValueType;
> +
> +  AtomicType(QualType ValTy, QualType Canonical)
> +    : Type(Atomic, Canonical, ValTy->isDependentType(),
> +           ValTy->isInstantiationDependentType(),
> +           ValTy->isVariablyModifiedType(),
> +           ValTy->containsUnexpandedParameterPack()),
> +      ValueType(ValTy) {}
> +  friend class ASTContext;  // ASTContext creates these.
> +
> +  public:
> +  /// getValueType - Gets the type contained by this atomic type, i.e.
> +  /// the type returned by performing an atomic load of this atomic type.
> +  QualType getValueType() const { return ValueType; }
> +
> +  bool isSugared() const { return false; }
> +  QualType desugar() const { return QualType(this, 0); }
> +
> +  void Profile(llvm::FoldingSetNodeID &ID) {
> +    Profile(ID, getValueType());

Could be getValueType().Profile(ID) to eliminate the need for the
static function.

> +  }
> +  static void Profile(llvm::FoldingSetNodeID &ID, QualType T) {
> +    ID.AddPointer(T.getAsOpaquePtr());
> +  }
> +  static bool classof(const Type *T) {
> +    return T->getTypeClass() == Atomic;
> +  }
> +  static bool classof(const AtomicType *) { return true; }
> +};
> +
>  /// A qualifier set is used to build a set of qualifiers.
>  class QualifierCollector : public Qualifiers {
>  public:
> Index: lib/Sema/TreeTransform.h
> ===================================================================
> --- lib/Sema/TreeTransform.h	(revision 141016)
> +++ lib/Sema/TreeTransform.h	(working copy)
> @@ -905,6 +905,12 @@
>                                          NumExpansions);
>    }
>
> +  /// \brief Build a new atomic type given its value type.
> +  ///
> +  /// By default, performs semantic analysis when building the pointer type.

Mis-copied comment.

> +  /// Subclasses may override this routine to provide different behavior.
> +  QualType RebuildAtomicType(QualType ValueType, SourceLocation KWLoc);
> +
>    /// \brief Build a new template name given a nested name specifier, a flag
>    /// indicating whether the "template" keyword was provided, and the template
>    /// that the template name refers to.
> Index: lib/Sema/SemaTemplateDeduction.cpp
> ===================================================================
> --- lib/Sema/SemaTemplateDeduction.cpp	(revision 141016)
> +++ lib/Sema/SemaTemplateDeduction.cpp	(working copy)
> @@ -1116,7 +1116,17 @@
>                                         Info, Deduced, TDF);
>
>        return Sema::TDK_NonDeducedMismatch;
> -
> +
> +    //     _Atomic T   [extension]

Is this _Atomic T or _Atomic(T) or both?

> +    case Type::Atomic:
> +      if (const AtomicType *AtomicArg = Arg->getAs<AtomicType>())
> +        return DeduceTemplateArguments(S, TemplateParams,
> +                                       cast<AtomicType>(Param)->getValueType(),
> +                                       AtomicArg->getValueType(),
> +                                       Info, Deduced, TDF);
> +
> +      return Sema::TDK_NonDeducedMismatch;
> +
>      //     T *
>      case Type::Pointer: {
>        QualType PointeeType;
> Index: lib/Sema/SemaType.cpp
> ===================================================================
> --- lib/Sema/SemaType.cpp	(revision 141016)
> +++ lib/Sema/SemaType.cpp	(working copy)

> @@ -2872,6 +2882,10 @@
>      void VisitTagTypeLoc(TagTypeLoc TL) {
>        TL.setNameLoc(DS.getTypeSpecTypeNameLoc());
>      }
> +    void VisitAtomicTypeLoc(AtomicTypeLoc TL) {
> +      TL.setKWLoc(DS.getTypeSpecTypeLoc());
> +      TL.setParensRange(DS.getTypeofParensRange());

Is getTypeofParensRange() named like that because at one point the
only thing that had a parens range was typeof()?  It may be time to
change the name.

> +    }
>
>      void VisitTypeLoc(TypeLoc TL) {
>        // FIXME: add other typespec types and change this to an assert.
> @@ -4183,3 +4197,16 @@
>    }
>    llvm_unreachable("unknown unary transform type");
>  }
> +
> +QualType Sema::BuildAtomicType(QualType T, SourceLocation Loc) {
> +  if (!T->isDependentType() &&
> +      (T->isIncompleteType() || !T.isTriviallyCopyableType(Context))) {
> +    Diag(Loc, diag::err_atomic_specifier_bad_type) << T;

I'd appreciate a better message here. "cannot be specified" doesn't
tell the user what they did wrong. Instead, split it into two
messages, for incomplete vs non-trivially copyable types, and say
something like "_Atomic specifier cannot apply to incomplete type" and
"atomic types must be trivially copyable".  I suspect the library will
use static_assert(is_trivially_copyable<T>::value, "…") so that C++
users never see this particular diagnostic, but we still shouldn't
confuse C users.

Also, C1x says that the _Atomic specifier can't be applied to "an
array type, a function type, an atomic type, or a qualified type.",
while the _Atomic qualifier can't be applied to "an array type or a
function type." Are those covered by the trivially-copyable
restriction? This probably deserves a comment.

> +    return QualType();
> +  }
> +
> +  // FIXME: Do we need any handling for ARC here?
> +
> +  // Build the pointer type.
> +  return Context.getAtomicType(T);
> +}
> Index: lib/Sema/SemaExpr.cpp
> ===================================================================
> --- lib/Sema/SemaExpr.cpp	(revision 141016)
> +++ lib/Sema/SemaExpr.cpp	(working copy)
> @@ -342,6 +342,10 @@
>    QualType T = E->getType();
>    assert(!T.isNull() && "r-value conversion on typeless expression?");
>
> +  // We can't do lvalue-to-rvalue on atomics yet.

According to the C1x draft I have, it's not "yet". The only way to get
an rvalue from an atomic lvalue is to call atomic_load().

> +  if (T->getAs<AtomicType>())
> +    return Owned(E);
> +
>    // Create a load out of an ObjCProperty l-value, if necessary.
>    if (E->getObjectKind() == OK_ObjCProperty) {
>      ExprResult Res = ConvertPropertyForRValue(E);
> @@ -5388,6 +5392,10 @@
>    LHSType = Context.getCanonicalType(LHSType).getUnqualifiedType();
>    RHSType = Context.getCanonicalType(RHSType).getUnqualifiedType();
>
> +  // We can't do assignment from/to atomics yet.

Again, the only way to assign an atomic is with atomic_store().

> +  if (LHSType->isAtomicType())
> +    return Incompatible;
> +
>    // Common case: no conversion required.
>    if (LHSType == RHSType) {
>      Kind = CK_NoOp;
> Index: lib/AST/TypePrinter.cpp
> ===================================================================
> --- lib/AST/TypePrinter.cpp	(revision 141016)
> +++ lib/AST/TypePrinter.cpp	(working copy)
> @@ -581,6 +582,16 @@
>    }
>  }
>
> +void TypePrinter::printAtomic(const AtomicType *T, std::string &S) {
> +  if (!S.empty())
> +    S = ' ' + S;
> +  std::string Str;
> +  IncludeStrongLifetimeRAII Strong(Policy);
> +  print(T->getValueType(), Str);
> +
> +  S = "_Atomic(" + Str + ")" + S;

Wow, this is inefficient.  I wouldn't comment, but printAtomic takes
an out parameter for the result, and the only reason to do that is to
avoid copies, but this code copies out the wazoo.

> +}
> +
>  /// Appends the given scope to the end of a string.
>  void TypePrinter::AppendScope(DeclContext *DC, std::string &Buffer) {
>    if (DC->isTranslationUnit()) return;
> Index: lib/AST/ItaniumMangle.cpp
> ===================================================================
> --- lib/AST/ItaniumMangle.cpp	(revision 141016)
> +++ lib/AST/ItaniumMangle.cpp	(working copy)
> @@ -2111,6 +2112,11 @@
>      mangleType(D);
>  }
>
> +void CXXNameMangler::mangleType(const AtomicType *T) {

The other mangleType definitions have a grammar fragment in a comment.
 It looks like the official ABI doesn't mention atomics yet, so can
you describe where you got the v17 prefix?

> +  Out << "v17_Atomic";
> +  mangleType(T->getValueType());
> +}
> +
>  void CXXNameMangler::mangleIntegerLiteral(QualType T,
>                                            const llvm::APSInt &Value) {
>    //  <expr-primary> ::= L <type> <value number> E # integer literal
> Index: lib/AST/ASTContext.cpp
> ===================================================================
> --- lib/AST/ASTContext.cpp	(revision 141016)
> +++ lib/AST/ASTContext.cpp	(working copy)
> @@ -1063,8 +1063,13 @@
>        return getTypeInfo(getCanonicalType(T));
>    }
>
> +  case Type::Atomic: {
> +    // FIXME: The alignment needs to be "fixed".

What do you mean "fixed"? Do you mean rounded up to the next power of
2 over the size, or changed to match the target platform?

> +    return getTypeInfo(cast<AtomicType>(T)->getValueType());
>    }
>
> +  }
> +
>    assert(Align && (Align & (Align-1)) == 0 && "Alignment must be power of 2");
>    return std::make_pair(Width, Align);
>  }
> @@ -1707,6 +1712,10 @@
>      break;
>    }
>
> +  case Type::Atomic: {
> +    // FIXME: Implement!
> +  }

Does "The type name in an atomic type specifier shall not refer to an
array type" from C1x 6.7.2.4 imply this should go in the "type should
never be variably-modified" section?

> +
>    case Type::ConstantArray: {
>      const ConstantArrayType *cat = cast<ConstantArrayType>(ty);
>      result = getConstantArrayType(
> @@ -2904,6 +2913,34 @@
>    return QualType(AT, 0);
>  }
>
> +/// getAtomicType - Return the uniqued reference to the atomic type for
> +/// the given value type.
> +QualType ASTContext::getAtomicType(QualType T) const {
> +  // Unique pointers, to guarantee there is only one pointer of a particular
> +  // structure.
> +  llvm::FoldingSetNodeID ID;
> +  AtomicType::Profile(ID, T);
> +
> +  void *InsertPos = 0;
> +  if (AtomicType *AT = AtomicTypes.FindNodeOrInsertPos(ID, InsertPos))
> +    return QualType(AT, 0);
> +
> +  // If the pointee type isn't canonical, this won't be a canonical type either,

s/pointee/atomic/

> +  // so fill in the canonical type field.
> +  QualType Canonical;
> +  if (!T.isCanonical()) {
> +    Canonical = getAtomicType(getCanonicalType(T));
> +
> +    // Get the new insert position for the node we care about.
> +    AtomicType *NewIP = AtomicTypes.FindNodeOrInsertPos(ID, InsertPos);
> +    assert(NewIP == 0 && "Shouldn't be in the map!"); (void)NewIP;
> +  }
> +  AtomicType *New = new (*this, TypeAlignment) AtomicType(T, Canonical);
> +  Types.push_back(New);
> +  AtomicTypes.InsertNode(New, InsertPos);
> +  return QualType(New, 0);
> +}
> +
>  /// getAutoDeductType - Get type pattern for deducing against 'auto'.
>  QualType ASTContext::getAutoDeductType() const {
>    if (AutoDeductTy.isNull())
> @@ -5802,6 +5839,24 @@
>        return RHS;
>      return getBlockPointerType(ResultType);
>    }
> +  case Type::Atomic:
> +  {
> +    // Merge two pointer types, while trying to preserve typedef info
> +    QualType LHSValue = LHS->getAs<AtomicType>()->getValueType();
> +    QualType RHSValue = RHS->getAs<AtomicType>()->getValueType();
> +    if (Unqualified) {
> +      LHSValue = LHSValue.getUnqualifiedType();
> +      RHSValue = RHSValue.getUnqualifiedType();
> +    }
> +    QualType ResultType = mergeTypes(LHSValue, RHSValue, false,
> +                                     Unqualified);
> +    if (ResultType.isNull()) return QualType();
> +    if (getCanonicalType(LHSValue) == getCanonicalType(ResultType))
> +      return LHS;
> +    if (getCanonicalType(RHSValue) == getCanonicalType(ResultType))
> +      return RHS;
> +    return getPointerType(ResultType);

You mean getAtomicType?  If getPointerType is wrong here, please add a
test that fails because of it.

> +  }
>    case Type::ConstantArray:
>    {
>      const ConstantArrayType* LCAT = getAsConstantArrayType(LHS);
> Index: lib/CodeGen/CodeGenFunction.cpp
> ===================================================================
> --- lib/CodeGen/CodeGenFunction.cpp	(revision 141016)
> +++ lib/CodeGen/CodeGenFunction.cpp	(working copy)
> @@ -86,6 +86,10 @@
>    case Type::ObjCObject:
>    case Type::ObjCInterface:
>      return true;
> +
> +  // In IRGen, atomic types are just the underlying type

Does the extra alignment that's sometimes required imply wrapping an
aggregate around some atomics' value types?

> +  case Type::Atomic:
> +    return hasAggregateLLVMType(type->getAs<AtomicType>()->getValueType());
>    }
>    llvm_unreachable("unknown type kind!");
>  }
> @@ -978,6 +982,10 @@
>      case Type::FunctionNoProto:
>        type = cast<FunctionType>(ty)->getResultType();
>        break;
> +
> +    case Type::Atomic:
> +      type = cast<AtomicType>(ty)->getValueType();

Similarly here, it's possible atomics are never variably modified.

> +      break;
>      }
>    } while (type->isVariablyModifiedType());
>  }
> Index: lib/CodeGen/CGRTTI.cpp
> ===================================================================
> --- lib/CodeGen/CGRTTI.cpp	(revision 141016)
> +++ lib/CodeGen/CGRTTI.cpp	(working copy)
> @@ -474,6 +474,10 @@
>      // abi::__pointer_to_member_type_info.
>      VTableName = "_ZTVN10__cxxabiv129__pointer_to_member_type_infoE";
>      break;
> +
> +  case Type::Atomic:
> +    // Pretend this is a class; not sure what else we can do here.

FIXME: ask the C++ ABI folks?  Complex and vector get to be
fundamental types, so maybe atomic should be too?

> +    VTableName = ClassTypeInfo;
>    }
>
>    llvm::Constant *VTable =
> Index: lib/CodeGen/CGDebugInfo.cpp
> ===================================================================
> --- lib/CodeGen/CGDebugInfo.cpp	(revision 141016)
> +++ lib/CodeGen/CGDebugInfo.cpp	(working copy)
> @@ -1418,6 +1418,13 @@
>                                     0, 0, Elements);
>  }
>
> +llvm::DIType CGDebugInfo::CreateType(const AtomicType *Ty,
> +                                     llvm::DIFile U) {
> +  // Ignore the atomic wrapping
> +  // FIXME: What is the correct representation?

Cary Coutant suggests adding a DW_TAG_atomic_type node with a child
node pointing at the value type, similar to DW_TAG_const_type, etc.
Then extra alignment, size, etc. information can be attached to the
DW_TAG_atomic_type node. He's going to bring this up with the Dwarf
committee and let us know what they say.

> +  return getOrCreateType(Ty->getValueType(), U);
> +}
> +
>  /// CreateEnumType - get enumeration type.
>  llvm::DIType CGDebugInfo::CreateEnumType(const EnumDecl *ED) {
>    llvm::DIFile Unit = getOrCreateFile(ED->getLocation());