Page MenuHomePhabricator

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

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
4713

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.Nov 4 2020, 6:21 PM
clang/lib/CodeGen/TargetInfo.cpp
4713

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.Nov 5 2020, 12:09 PM
clang/lib/CodeGen/TargetInfo.cpp
4713

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.Nov 14 2020, 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.

efriedma accepted this revision.Dec 15 2020, 1:03 PM

LGTM with one minor comment.

Do you have commit access? If not, I can merge for you; please specify your preferred git "Author" line.

clang/test/CodeGenCXX/ppc32-varargs-method.cpp
16

Not sure referring to numUsedRegs like this will work in a non-Asserts build. Please verify.

This revision is now accepted and ready to land.Dec 15 2020, 1:03 PM

Hi, Eli. I'm missing emails from Phabricator, so I didn't know about your recent post. I will respond to your question about numUsedRegs when I find time.

nemanjai accepted this revision.Dec 21 2020, 12:15 PM

LGTM aside from a minor nit.

clang/lib/CodeGen/TargetInfo.cpp
4712

nit (feel free to ignore): seems like a better name might be something like isSingleGPR since it would appear that this is for types that go into a (single) general purpose register.

I forgot about this diff for a month.

clang/lib/CodeGen/TargetInfo.cpp
4712

Hi. isInt is also true for a 64-bit integer, which goes into a pair of GPRs, because each register has 32 bits.

clang/test/CodeGenCXX/ppc32-varargs-method.cpp
16

I did a build with cmake -DLLVM_ENABLE_ASSERTIONS=OFF and verified that the ppc32*varargs* tests still pass. %numUsedRegs is still in the llvm output.

%numUsedRegs does not appear in the output of clang -S -emit-llvm ... (not cc1), but %numUsedRegs does appear in the output of clang -cc1 .... The test runs cc1 and sees the %numUsedRegs. I am confused by how llvm sometimes erases the name of %numUsedRegs and sometimes keeps the name, but I observe that turning LLVM_ENABLE_ASSERTIONS on or off doesn't affect the test.

This revision was automatically updated to reflect the committed changes.