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

[Alexander Zinenko]

Hello, John!

Thanks for your comments and sorry that it's taking so long to polish my
code.
Please find the updated version attached.

On 20 March 2013 07:02, John McCall <rjmccall at apple.com> wrote:

> 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.
>
Actually, we do not call CheckReinterpretCast if the cast is
type-dependent. The reason behind these checks is the following case with
forward declaration
class A;
class B {};
void foo(A *a) {
  reinterpret_cast<B *>(a);
}
in which we can't examine the definition of A since we don't have it yet.
I added a comment and mentioned templates at the end though.

--
Alex


>
> 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/20130321/26da6c29/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 13824quiet.patch
Type: application/octet-stream
Size: 16934 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130321/26da6c29/attachment.obj>