[PATCH] D81420: Fix size for _ExtInt types with builtins

[Mott, Jeffrey T via Phabricator via cfe-commits]

jtmott-intel marked an inline comment as done.
jtmott-intel added inline comments.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:327
       return true;
     TheCall->setArg(2, Arg.get());
   }
----------------
rjmccall wrote:
> jtmott-intel wrote:
> > rjmccall wrote:
> > > I know this is existing code, but this is a broken mess.  Please change this to:
> > > 
> > > ```
> > >   ExprResult Arg = DefaultFunctionArrayLvalueConversion(TheCall->getArg(2));
> > >   if (Arg.isInvalid()) return true;
> > >   TheCall->setArg(2, Arg.get());
> > > 
> > >   QualType Ty = Arg.get()->getType();
> > >   const auto *PtrTy = Ty->getAs<PointerType>();
> > >   if (!PtrTy ||
> > >       !PtrTy->getPointeeType()->isIntegerType() ||
> > >       PtrTy->getPointeeType().isConstQualified()) {
> > >     S.Diag(Arg.get()->getBeginLoc(),
> > >            diag::err_overflow_builtin_must_be_ptr_int)
> > >       << Ty << Arg.get()->getSourceRange();
> > >     return true;
> > >   }
> > > ```
> > > 
> > > Test case would be something like (in ObjC):
> > > 
> > > ```
> > > @interface HasPointer
> > > @property int *pointer;
> > > @end
> > > 
> > > void test(HasPointer *hp) {
> > >   __builtin_add_overflow(x, y, hp.pointer);
> > > }
> > > ```
> > > 
> > > And the earlier block needs essentially the same change (but obviously checking for a different type).  You can't check types before doing placeholder conversions, and once you do l-value conversions and a type-check, you really don't need the full parameter-initialization logic.
> > I've implemented this locally but I have a quick question about the test. It passed even before I applied this code change. Is this test expected to fail before this change and pass after? Or is it just to prove that the feature (placeholder argument types?) works in general?
> Oh, we seem to handle placeholders specially, nevermind.  Well, what I said about doing conversions before checking the type still holds, but the test case is just this:
> 
> ```
> int z[1];
> __builtin_add_overflow(x, y, z);
> ```
> 
> where we should decay the argument to a pointer instead of rejecting it.
Done. Latest updated patch includes these code changes.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81420/new/

https://reviews.llvm.org/D81420