[cfe-commits] [PATCH] Type safety attributes (was: Compile-time MPI_Datatype checking)
Dmitri Gribenko
gribozavr at gmail.com
Sat Aug 4 22:42:24 PDT 2012
On Tue, Jun 12, 2012 at 10:46 AM, Jordan Rose <jordan_rose at apple.com> wrote:
> 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.
Thank you for the review! Finally, I got back to this.
> + 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.
Done & added tests.
> + 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?
Makes sense, simplifies code. And this will halve the number of
specific_attr_iterator's, so checking function calls should be faster.
> + 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.
If tag is value-dependent, we will not get to this, because we will
not be able to find a matching C type a few lines earlier. Added a
test, 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; };
I don't think they should be layout-compatible according to standard.
> 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.
Usually, in the context of this attribute, char type is used for
character strings. But I think that allowing only the matching
signed/unsigned char to be used in place of char should be enough to
silence most false positives.
> Finally, that giant condition can probably go in a helper function, especially if we're doing the same thing somewhere else in Sema.
Extracted a function. We don't do anything like this elsewhere is 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.
Done.
> + 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.
Done.
> + 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.)
We can. But there are usually very few of these structs created --
one per declared type tag. Do you think it is worth it?
> 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".
Done, but cloned the warn_nonnull_pointers_only to err_attribute_pointers_only.
> +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?
I would better make this an error. What do you think? The attribute
is used by API developer and the compiler should not be forgiving in
attribute misuse.
> +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"?
I don't think it is helpful to print internal identifiers: the message
still says that "you shouldn't use this tag here". Can change
message, though, if you think it would be an improvement.
> +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.
Quotes: done.
Regarding 'pointee' word: we use it in our sources and documentation,
but I agree that it might sound funny. Merged both "mismatch"
warnings into one.
Regarding rewording: in my proposed messages I print the argument type
and underline the tag. It is useful to print the type because it
might not be obvious from the source. Printing back the type tag is
not helpful, in my opinion, since we can underline it.
> +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.)
Changed wording to your suggestion.
I don't think typo-correction is worth it (someone will add attributes
to the API decls just once).
> 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.
Would love to hear.
Dmitri
--
main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
(j){printf("%d\n",i);}}} /*Dmitri Gribenko <gribozavr at gmail.com>*/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: type-safety-attrs-v8.patch
Type: application/octet-stream
Size: 72359 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120804/ca09f9a3/attachment.obj>
More information about the cfe-commits
mailing list