[cfe-commits] [PATCH] Type safety attributes (was: Compile-time MPI_Datatype checking)

[Jordan Rose]

Hi, Dmitri. I'd like for this patch not to get lost with your new comment-parsing work, though it might be too late for that.


+    case Stmt::BinaryConditionalOperatorClass:
+    case Stmt::ConditionalOperatorClass: {
+      const AbstractConditionalOperator *ACO =
+          cast<AbstractConditionalOperator>(TypeExpr);
+      bool Result;
+      if (ACO->getCond()->EvaluateAsBooleanCondition(Result, Ctx)) {
+        if (Result)
+          TypeExpr = ACO->getTrueExpr();
+        else
+          TypeExpr = ACO->getFalseExpr();
+        continue;
+      }
+    }

Unintended fallthrough here? If we can't evaluate the condition at compile-time, we probably failed (return false).


+    case Stmt::BinaryOperatorClass: {
+      const BinaryOperator *BO = cast<BinaryOperator>(TypeExpr);
+      if (BO->getOpcode() == BO_Comma) {
+        TypeExpr = BO->getRHS();
+        continue;
+      }
+    }

Ditto, although this time the fallthrough is to a "return false" so it would work as intended.


+  if (const ArgumentWithTypeTagAttr *A =
+          dyn_cast<ArgumentWithTypeTagAttr>(Attr)) {
+    ArgumentKind = A->getArgumentKind();
+    TypeTagIdx = A->getTypeTagIdx();
+    ArgumentIdx = A->getArgumentIdx();
+    IsPointerAttr = false;
+  } else if (const PointerWithTypeTagAttr *A =
+          dyn_cast<PointerWithTypeTagAttr>(Attr)) {
+    ArgumentKind = A->getPointerKind();
+    TypeTagIdx = A->getTypeTagIdx();
+    ArgumentIdx = A->getPointerIdx();
+    IsPointerAttr = true;

There's a few places with code like this. Why not just use one attribute kind for both ArgumentWithTypeTagAttr and PointerWithTypeTagAttr, and have the attribute have an ExpectsPointer flag?


+    if (!ArgumentExpr->isNullPointerConstant(Context,
+                                             Expr::NPC_ValueDependentIsNotNull)) {

If the type tag being passed is also value-dependent, this might be a spurious warning (although that's a kind of scary way to code). I think you'd also get the warning again when the template is instantiated, so I would consider using NPC_ValueDependentIsNull. You should probably have a test case for this, though.


+    // C++11 [basic.fundamental] p1:
+    // Plain char, signed char, and unsigned char are three distinct types.
+    //
+    // But we treat plain `char' as equivalent to `signed char' or `unsigned
+    // char' depending on the current char signedness mode.
+    if(mismatch &&
+       isa<BuiltinType>(ArgumentType) && isa<BuiltinType>(RequiredType)) {
+      BuiltinType::Kind PointeeKind = cast<BuiltinType>(ArgumentType)->getKind();
+      BuiltinType::Kind RequiredKind = cast<BuiltinType>(RequiredType)->getKind();
+      if((PointeeKind == BuiltinType::SChar  && RequiredKind == BuiltinType::Char_S) ||
+         (PointeeKind == BuiltinType::UChar  && RequiredKind == BuiltinType::Char_U) ||
+         (PointeeKind == BuiltinType::Char_U && RequiredKind == BuiltinType::UChar) ||
+         (PointeeKind == BuiltinType::Char_S && RequiredKind == BuiltinType::SChar))
+        mismatch = false;

You don't do this for isLayoutCompatible, though. Are these two structs layout-compatible under -fsigned-char?

struct A { char x; };
struct B { signed char x; };

To me, using isa<> and then cast<> is less pretty than using dyn_cast, even though you'd have to do both dyn_casts before the condition.

Do we want to treat a required type of 'char' as equivalent to /any/ char type? Usually someone using a type_tag for 'char' would be passing either raw bytes or ASCII (or UTF-8) characters, neither of which really have inherent signedness. OTOH, if the signedness is in the required type than I think being strict is correct.

Finally, that giant condition can probably go in a helper function, especially if we're doing the same thing somewhere else in Sema.


+  if (!Attr.getParameterName()) {
+    S.Diag(Attr.getLoc(), diag::err_attribute_argument_n_not_identifier)
+      << Attr.getName() << 1;
+    return;
+  }
+
+  if (Attr.getNumArgs() != 2) {
+    S.Diag(Attr.getLoc(), diag::err_attribute_wrong_number_arguments) << 3;
+    return;
+  }

Please describe in an inline comment what the magic numbers 1 and 3 indicate for these diagnostics.


+  if (Pointer)
+    D->addAttr(::new (S.Context) PointerWithTypeTagAttr(Attr.getRange(),
+                                                        S.Context,
+                                                        PointerKind,
+                                                        PointerIdx,
+                                                        TypeTagIdx));
+  else
+    D->addAttr(::new (S.Context) ArgumentWithTypeTagAttr(Attr.getRange(),
+                                                         S.Context,
+                                                         PointerKind,
+                                                         PointerIdx,
+                                                         TypeTagIdx));

Again, I think it's a good idea to unify these.


+  struct TypeTagForDatatypeData {
+    ParsedType *MatchingCType;
+    unsigned LayoutCompatible : 1;
+    unsigned MustBeNull : 1;
+  };

This is a little silly, but can we cram these into a PointerIntPair? Right now we're spending two words where we could get away with 1. (Or, even if we pass the data around in struct format, we could be /storing/ it in a single word.)


And finally, some warning wording suggestions:

+def err_pointer_with_type_tag_not_pointer : Error<
+  "pointer argument is not of a pointer type">;

I would actually use warn_nonnull_pointers_only, and change it to take the name of the attribute, so that you can say "'pointer_with_type_tag attribute only applies to pointer arguments".


+def warn_type_tag_for_datatype_not_ice : Warning<
+  "'type_tag_for_datatype' attribute requires the initializer to be "
+  "an %select{integer|integral}0 constant expression; "
+  "initializer ignored by attribute">, InGroup<TypeSafety>;

This one's mostly okay, but "initializer ignored by attribute" is making me wonder if it really means "initializer /discarded/ by attribute", which would affect correctness. Perhaps the last clause should just be removed?


+def warn_type_tag_for_datatype_wrong_kind : Warning<
+  "this type tag was not designed to be used with this function">,
+  InGroup<TypeSafety>;

Let's be helpful here and say something like "type tag 'D_tag' is associated with SYSTEM 'd', but function/method 'C_func' uses SYSTEM 'c'". Clearly this isn't perfect; I'm not sure what to say instead of SYSTEM. "Type schema"? "Type family"?

+def warn_type_safety_type_mismatch : Warning<
+  "argument type %0 doesn't match specified %1 type tag "
+  "%select{that requires %3|}2">, InGroup<TypeSafety>;
+def warn_type_safety_pointee_type_mismatch : Warning<
+  "pointee type %0 doesn't match specified %1 type tag "
+  "%select{that requires %3|}2">, InGroup<TypeSafety>;
+def warn_type_safety_null_pointer_required : Warning<
+  "specified %0 type tag requires a null pointer">, InGroup<TypeSafety>;
+

I like the last one here, though maybe it should have single quotes around the type family name. The other two could be more descriptive, though:

"argument is required to %select{have|be layout-compatible with}2 type '%1' by type tag '%0'"
"argument is required to be a pointer to type '%1' by type tag '%0'"
"argument is required to be null by type tag '%0'"

I think having the name of the type tag in the warning makes things clearer, especially if you accidentally put the wrong type tag. (Since most type tags have a prefix, the type family would be redundant) And none of our other diagnostics use the word "pointee", which sounds funny to me.

My weird argument numbering is for consistency among the three warnings, but that might be more confusing than it's worth.


+def err_type_safety_unknown_flag : Error<
+  "%0 is not a type comparison flag; use 'layout_compatible' or 'must_be_null'">;
+

Bike-shedding suggestion: "invalid type comparison flag '%0'". I'm not sure if the suggestions for valid flags is necessary, but this is a new annotation that people will not be familiar with.

(If you want to get fancy here, add typo detection.)

--

I think this is a really useful addition to a compiler. Off the top of my head I can think of one more major potential user of these annotations: OpenGL. I'd really like to see this go in.

I have a couple comments on your docs but I think this is quite enough for now.

Jordan