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

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:327
       return true;
     TheCall->setArg(2, Arg.get());
   }
----------------
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.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:337
+      QualType Ty =
+          I < 2 ? Arg.get()->getType()
+                : Arg.get()->getType()->getAs<PointerType>()->getPointeeType();
----------------
jtmott-intel wrote:
> erichkeane wrote:
> > Try:
> > 
> > Type *Ty = ArgExpr->getType()->getPointeeOrArrayElementType();
> > 
> > instead of the ternary.
> A complication I'm seeing is that `getPointeeOrArrayElementType` returns a `Type` but getIntWidth wants a `QualType`. Some options that occurred to me:
> 
> - Is it acceptable to just wrap a `Type` in a `QualType`?
> - I might be able to add a `getIntWidth(const Type*)` overload.
> - Looks like I can skip the `getAs<PointerType>()` and just call `getPointeeType` directly. The line would be shorter but I would still need the ternary.
* Is it acceptable to just wrap a Type in a QualType?

In general, no, this can lose qualifiers on the array element.  But `getIntWidth` shouldn't ever care about that, so in the abstract, adding the overload should be fine.  However, in this case I think the ternary is actually better code.


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

https://reviews.llvm.org/D81420