Page MenuHomePhabricator

[PowerPC] Fix va_arg in C++, Objective-C on 32-bit ELF targets
Needs ReviewPublic

Authored by kernigh on Oct 28 2020, 12:00 PM.

Details

Summary

In the PPC32 SVR4 ABI, a va_list has copies of registers from the function call. va_arg looked in the wrong registers for (the pointer representation of) an object in Objective-C, and for some types in C++. Fix va_arg to look in the general-purpose registers, not the floating-point registers. Also fix va_arg for some C++ types, like a member function pointer, that are aggregates for the ABI.

Anthony Richardby found the problem in Objective-C. Eli Friedman suggested part of this fix.

Fixes https://bugs.llvm.org/show_bug.cgi?id=47921

Diff Detail

Event Timeline

kernigh created this revision.Oct 28 2020, 12:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 28 2020, 12:00 PM
kernigh requested review of this revision.Oct 28 2020, 12:00 PM

I'm using this TargetInfo.cpp diff in clang 10.0.1 on OpenBSD/macppc to build GNUstep. For me, it fixes the build of gnustep-gui. I rebuilt libobjc2, gnustep-make, and gnustep-base before building gnustep-gui.

This diff doesn't change 64-bit PowerPC, because 64-bit does va_list in a different way.

efriedma added inline comments.Oct 29 2020, 12:13 PM
clang/lib/CodeGen/TargetInfo.cpp
4723

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();?

kernigh added inline comments.Wed, Nov 4, 6:21 PM
clang/lib/CodeGen/TargetInfo.cpp
4723

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.

kernigh added inline comments.Thu, Nov 5, 12:09 PM
clang/lib/CodeGen/TargetInfo.cpp
4723

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.

kernigh updated this revision to Diff 305335.Sat, Nov 14, 6:08 PM
kernigh retitled this revision from [PowerPC] Fix va_arg in Objective-C on 32-bit ELF targets to [PowerPC] Fix va_arg in C++, Objective-C on 32-bit ELF targets.
kernigh edited the summary of this revision. (Show Details)

I have updated the diff to use bool isInt = !Ty->isFloatingType(); and bool isIndirect = isAggregateTypeForABI(Ty);. This removes both calls to Ty->isAggregateType(). This fixes some C++ types, because aren't aggregates in the C++ language, but do become aggregates in the ABI. The new test ppc32-varargs-method.cpp checks a pointer to a member function, and passes only with both the isInt and isIndirect changes.