[PATCH] D90329: [PowerPC] Fix va_arg in Objective-C on 32-bit ELF targets

[George Koehler via Phabricator via cfe-commits]

kernigh added inline comments.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:4723
+  bool isInt = Ty->isIntegerType() || Ty->hasPointerRepresentation() ||
+               Ty->isAggregateType();
   bool isF64 = Ty->isFloatingType() && getContext().getTypeSize(Ty) == 64;
----------------
kernigh wrote:
> efriedma wrote:
> > I suspect this code doesn't handle C++ member pointers correctly.  But maybe we can leave that for a followup.
> > 
> > Could we simplify this to `bool isInt = !Ty->isFloatingType();`?
> Yes, C++ member pointers are broken. I declared a struct animal, tried `typedef void (animal::*noise)(); ... va_arg(ap, noise)`, and it segfaulted. In the disassembly, I saw 2 problems: (1) va_arg was looking at the floating-point registers, and (2) va_arg was not indirecting through a pointer. The caller had passed a pointer to a `noise` in a general-purpose register.
> 
> I'm not sure about `bool isInt = !Ty->isFloatingType();`, because I haven't checked whether it would break other types.
`bool isInt = !Ty->isFloatingType();` is working for me. I'm doing more checks; if it continues to work, I will update this diff.

Another change in `bool isIndirect = Ty->isAggregateType();` to instead call `isAggregateTypeForABI(Ty)` is fixing va_arg of a C++ member function pointer. I may want to put this change in the same diff because I will be running both changes together.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90329