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

[Alexander Zinenko]

Hello John,

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.

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.

+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?


> +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 *>(...).


>
> +  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.

>

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.

 Fixed all of the above.

>
>
John.
>
> --
Alex
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130314/f62ad292/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 13824quiet.patch
Type: application/octet-stream
Size: 5813 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130314/f62ad292/attachment.obj>