[PATCH] D41039: Add support for attribute "trivial_abi"

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In https://reviews.llvm.org/D41039#951807, @rjmccall wrote:

> In https://reviews.llvm.org/D41039#951648, @ahatanak wrote:
>
> > I had a discussion with Duncan today and he pointed out that perhaps we shouldn't allow users to annotate a struct with "trivial_abi" if one of its subobjects is non-trivial and is not annotated with "trivial_abi" since that gives users too much power.
> >
> > Should we error out or drop "trivial_abi" from struct Outer when the following code is compiled?
> >
> >   struct Inner1 {
> >     ~Inner1(); // non-trivial
> >     int x;
> >   };
> >  
> >   struct __attribute__((trivial_abi)) Outer {
> >     ~Outer();
> >     Inner1 x;
> >   };
> >
> >
> > The current patch doesn't error out or drop the attribute, but the patch would probably be much simpler if we didn't allow it.
>
>
> I think it makes sense to emit an error if there is provably a non-trivial-ABI component.  However, for class temploids I think that diagnostic should only fire on the definition, not on instantiations; for example:
>
>   template <class T> struct __attribute__((trivial_abi)) holder {
>      T value;
>      ~holder() {}
>   };
>   holder<std::string> hs; // this instantiation should be legal despite the fact that holder<std::string> cannot be trivial-ABI.
>   
>
> But we should still be able to emit the diagnostic in template definitions, e.g.:
>
>   template <class T> struct __attribute__((trivial_abi)) named_holder {
>      std::string name; // there are no instantiations of this template that could ever be trivial-ABI
>      T value;
>      ~named_holder() {}
>   };
>   
>
> The wording should be something akin to the standard template rule that a template is ill-formed if it has no valid instantiations, no diagnostic required.
>
> I would definitely like to open the conversation about the name of the attribute.  I don't think we've used "abi" in an existing attribute name; usually it's more descriptive.  And "trivial" is a weighty word in the standard.  I'm not sure I have a great counter-proposal off the top of my head, though.






================
Comment at: include/clang/AST/DeclCXX.h:443
+    /// SMF_MoveConstructor, and SMF_Destructor are meaningful here.
+    unsigned HasTrivialSpecialMembersForCall : 6;
+
----------------
I think you could probably get away with only three bits here (and below) if you reorder those three values to be the first three.  I don't know if that's important in terms of packing DefinitionData at all.  Should be okay to not do for now.


================
Comment at: include/clang/AST/DeclCXX.h:478
+    /// ill-formed if it is annotated with "trivial_abi".
+    unsigned DropTrivialABI : 1;
+
----------------
So is it actually interesting to drop the attribute from the AST?  And why is this bit necessary vs., say, just checking whether the attribute is present but the class is not trivial-for-calls?


================
Comment at: include/clang/AST/DeclCXX.h:1380
+    return data().DeclaredNonTrivialSpecialMembersForCall &
+           SMF_CopyConstructor || !hasTrivialCopyConstructorForCall();
+  }
----------------
Please parenthesize, even if it isn't strictly required.


================
Comment at: include/clang/AST/DeclCXX.h:1502
+    return data().CanPassInRegisters && hasNonTrivialDestructor();
+  }
+
----------------
These method names read as if they're actions, not predicates.  Please use "is..." or "can..." or "should..." or something that makes it clear that they're predicates.


================
Comment at: include/clang/Basic/AttrDocs.td:2234
+when the type is considered non-trivial for the purpose of calls according to
+the C++ ABI. Non-trivial destructors or copy/move constructors of classes
+annotated with ``trivial_abi`` do not cause the corresponding special functions
----------------
You should say "when the type would otherwise be considered..." here, because this attribute actually changes the definition of non-trivial for the purposes of calls.

I would suggest rephrasing the bit about non-trivial special members like so: "A class annotated with ``trivial_abi`` can have non-trivial destructors or copy/move constructors without automatically becoming non-trivial for the purposes of calls."


================
Comment at: include/clang/Basic/AttrDocs.td:2251
+      A a;
+    };
+
----------------
You should state explicitly in this example that A is itself trivial for the purposes of calls.


================
Comment at: include/clang/Basic/AttrDocs.td:2255
+callee is responsible for destroying the object regardless of how it is passed
+under the C ABI.
+
----------------
"If a type is trivial for the purposes of calls, has a non-trivial destructor, and is passed as an argument by value, the convention is that the callee will destroy the object before returning."


================
Comment at: include/clang/Basic/AttrDocs.td:2257
+
+Attribute ``trivial_abi`` will be dropped from a class in the following cases:
+
----------------
Don't phrase it as being "dropped", phrase it as having no effect.


================
Comment at: include/clang/Basic/AttrDocs.td:2261
+- The class or its subobjects have Objective-C pointer type members and ARC is
+  enabled.
+  }];
----------------
I think the right list of exceptions is:

  - The class directly declares a virtual base or virtual methods.
  - The class has a base class that is non-trivial for the purposes of calls.
  - The class has a non-static data member whose type is non-trivial for the purposes of calls, which includes:
    - classes that are non-trivial for the purposes of calls
    - __weak-qualified types in Objective-C++
    - arrays of any of the above

I don't see why __strong types would be affected.  We've talked about changing the C++ ABI for structs containing __strong members as part of the __strong-pointers-in-structs feature, but that's not even implicated here because there's an attribute which did not previously exist, so there's no established ABI.


================
Comment at: lib/CodeGen/CGCall.cpp:3509
+    return;
+  }
+
----------------
You do have to push a cleanup here in case there's an exception thrown before the call is made.  Fortunately, you should be able to just use the logic immediately below, since it's exactly the same treatment that the Microsoft C++ ABI uses.


================
Comment at: lib/CodeGen/CodeGenFunction.cpp:1150
+      EmitAutoVarCleanups(Emission);
+    }
   }
----------------
We must already have a place that does this for the Microsoft ABI.


https://reviews.llvm.org/D41039