[cfe-commits] [PATCH] PR13824 warn if reinterpret_cast used for up/downcast

[John McCall]

On Mar 12, 2013, at 10:01 AM, Jordan Rose <jordan_rose at apple.com> wrote:
> Pinging this myself, adding Richard Smith as another reviewer. I don't quite feel qualified to give the final word on it, but I'm fixing an analyzer bug that's basically "don't crash when the user does this in exactly the right wrong way", and it'd be nice to have a warning so that they fix their code.

+def ReinterpretUpDownCastZeroAdjustment :
+  DiagGroup<"reinterpret-updown-zero-adjustment">;
+def ReinterpretUpDownCast : DiagGroup<"reinterpret-updown",

How about -Wreinterpret-base and -Wreinterpret-primary-base?

+def warn_reinterpret_wrong_subobject : Warning<
+  "'reinterpret_cast' might return a pointer to wrong subobject">,

How about "reinterpret_cast %select{from|to}3 class %0 %select{to|from}3 %select{virtual base|base at non-zero offset}2 %1 behaves differently from static_cast"?

+static void DiagnoseReinterpretUpDownCast(Sema &Self, ExprResult &SrcExpr,
+                                          QualType DestType,
+                                          const SourceRange &OpRange) {

You should take SrcExpr as a const Expr * and OpRange as a SourceRange.

+  const Type *SrcUnqualType = SrcType.getTypePtr();
+  const Type *DestUnqualType = DestType.getTypePtr();

This is unnecessary and also doesn't necessarily actually give you an unqualified type.

+  // When casting from pointer or reference, get pointee type; use original 
+  // type otherwise.
+  const CXXRecordDecl *SrcPointeeRD = SrcUnqualType->getPointeeCXXRecordDecl();
+  const CXXRecordDecl *SrcRD =
+    SrcPointeeRD ? SrcPointeeRD : SrcUnqualType->getAsCXXRecordDecl();

It'd be good to remember the original non-pointer sugared types here for diagnostic purposes.

Also, at this point in semantic analysis you can differentiate between the pointer and reference cases by just checking SrcExpr->isRValue().

+  CXXBasePaths SrcBP, DestBP;
+
...
+  const ASTRecordLayout &SrcLayout = Self.Context.getASTRecordLayout(SrcRD);
+  const ASTRecordLayout &DestLayout = Self.Context.getASTRecordLayout(DestRD);

1.  Please sink this down to the point where you need it.
2.  Please only actually get the layout for the derived class.
3.  You can re-use the paths object if the first lookup failed.

+  if (SrcBP.getDetectedVirtual() || DestBP.getDetectedVirtual()) {

You are ignoring the possibility that there are subobjects at multiple paths.  We should not warn if *any* of the paths is a non-virtually-derived base at offset zero.

+  CharUnits Offset = CharUnits::Zero();
+  if (ReinterpretKind == ReinterpretUpcast)
+    Offset = SrcLayout.getBaseClassOffset(DestRD);
+  else if (ReinterpretKind == ReinterpretDowncast)
+    Offset = DestLayout.getBaseClassOffset(SrcRD);

This won't work for indirect base classes.

+  DiagnoseReinterpretUpDownCast(Self, SrcExpr, DestType, OpRange);
+

Please sink this until we've decided that the cast succeeds.

John.