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

[John McCall]

On Mar 14, 2013, at 8:47 AM, Alexander Zinenko <ftynse at gmail.com> wrote:
> Thanks for review!
> 
> I attached a quiet version of a patch that doesn't warn if any of the paths is a non-virtually-derived base at offset zero.  
> During first iteration, Jordan proposed to make a separate switch to enable a noisy version that warns even if the cast is safe, though. If it is still required, it can be easily added back.
> Processing multiple base paths made checking a bit heavier, but I see no other approach.

+  if(!SrcRD || !SrcRD->hasDefinition())
+    return;
+
+  const CXXRecordDecl *DestRD = DestType->getPointeeCXXRecordDecl();
+
+  if(!DestRD || !DestRD->hasDefinition())
+    return;

Please add a comment saying that we're deliberately not instantiating
templates here because we're not allowed to.

Also, I think this needs to use isCompleteDefinition().

> On 12 March 2013 20:14, John McCall <rjmccall at apple.com> wrote:
> 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?
> We only need reinterpret-primary-base now.

Let's call it -Wreinterpret-base-class, actually.

> +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"?
> Fixed. Shouldn't we provide some details on what causes the difference?

Well, my suggestion mentions that it's a virtual base or a base at a non-zero
offset, but if you want something more or different, feel free.
 
> +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.
> Fixed.
> 
> +  const Type *SrcUnqualType = SrcType.getTypePtr();
> +  const Type *DestUnqualType = DestType.getTypePtr();
> 
> This is unnecessary and also doesn't necessarily actually give you an unqualified type.
> Deleted. 
> 
> +  // 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(). 
> Warning "wrong cast to T*" looks clearer to me than "wrong cast to T" if the code is reinterpret_cast<T *>(...).

That's fine.

> 
> +  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.
> We actually need the layout of any derived class in the pass to process indirect base offsets correctly.

Good point.  But we definitely don't need the layout of the base class. :)

+  enum {
+    ReinterpretNormalCast = -1,
+    ReinterpretUpcast,
+    ReinterpretDowncast
+  } ReinterpretKind;
+  ReinterpretKind = ReinterpretNormalCast;

You don't need ReinterpretNormalCast.  Just leave it uninitialized;
any reasonable -Wuninitialized will be able to figure out that it's initialized
along every path.

Otherwise, this looks great.  Feel free to check in with these changes.

John.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130319/2c8c54b0/attachment.html>