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

[Eli Friedman]

On Tue, Oct 4, 2011 at 11:47 AM, Jeffrey Yasskin <jyasskin at google.com> wrote:
> [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?

Yes; that's all we need, for the moment. The "qualifier" is more
complicated; we can add it later.

>> 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!

Yes. :)

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

The static function has another user.

>> +  }
>> +  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.

Yes.

>> +  /// 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?

This is Sema; the difference shouldn't matter.

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

Yes, probably.  I'll do that as a followup.

>> +    }
>>
>>      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.

Okay, will do.  (Although, "trivially copyable" is likely to confuse C
users; I'll see what I can come up with, though.)

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

Function types and atomic types are not trivially-copyable; arrays and
qualified types are.  I'll make the necessary adjustments (although
I'm not entirely comfortable banning arrays in C when they are legal
in C++; I might mark it an extension for the moment).

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

Okay, cool. :)

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

K.

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

Yes, the whole thing is ridiculous; no, I don't plan to rewrite it at
the moment.

>> +}
>> +
>>  /// 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?

It's a vendor-specific prefix, since there isn't any standardized
mangling at the moment.  I'll put in a comment to that effect.
(Granted, it usually won't matter unless someone tries to use _Atomic
in C++ directly.)

>> +  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?

Some combination of the two.  We need to increase the alignment of,
for example, long long on ARM, but we don't want to use some crazy
alignment if someone puts _Atomic on a 32-byte struct.  And actually,
I hadn't considered increasing the size of the type before, but it's a
possibility.

>> +    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?

_Atomic(int (*)[var]) is a variably-modified atomic type.

>> +
>>    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/

K.

>> +  // 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.

Yes, thanks for catching that.  And I'm pretty sure you can hit that
case pretty easily with atomic function pointers.

>> +  }
>>    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?

If you're increasing the size, yes... I wasn't really considering that
case when I wrote this.

>> +  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?

K.

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

Okay; please keep me up-to-date.

-Eli