This is an archive of the discontinued LLVM Phabricator instance.

[ABI] Fix SystemV ABI to handle nested aggregate type returned in register
ClosedPublic

Authored by kusmour on May 30 2019, 1:53 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

kusmour created this revision.May 30 2019, 1:53 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 30 2019, 1:53 PM

There should be a test to go along with this.

@jingham working on the unit test rn. will upload soon

compnerd added inline comments.May 30 2019, 4:35 PM
lldb/source/Plugins/ABI/SysV-x86_64/ABISysV_x86_64.cpp
1563 ↗(On Diff #202294)

NIT: faltten -> flatten

1568 ↗(On Diff #202294)

I really wish that we could just use CGFunctionInfo::getReturnInfo().getKind()

1678 ↗(On Diff #202294)

Hmm, dead code?

1810 ↗(On Diff #202294)

The braces are extraneous

1811 ↗(On Diff #202294)

The braces are extraneous

kusmour marked an inline comment as done.May 30 2019, 4:51 PM
kusmour added inline comments.
lldb/source/Plugins/ABI/SysV-x86_64/ABISysV_x86_64.cpp
1568 ↗(On Diff #202294)

that's clang. I can try simplify it using ·GetFieldAtIndex·. this just incase we need to check child_is_base_class

kusmour updated this revision to Diff 202340.May 30 2019, 5:41 PM

simplify the method in 'FlattenAggregateType'
added test for nested struct returned in registers

compnerd accepted this revision.May 30 2019, 6:50 PM

Thanks, this looks much better.

This revision is now accepted and ready to land.May 30 2019, 6:50 PM
compnerd requested changes to this revision.May 31 2019, 2:41 PM

Actually, I think that we should extend CompilerType and TypeSystem to expose Clang's knowledge of whether a type is passed in a register by means of using clang::RecordDecl::isPassInRegisters

This revision now requires changes to proceed.May 31 2019, 2:41 PM

Actually, I think that we should extend CompilerType and TypeSystem to expose Clang's knowledge of whether a type is passed in a register by means of using clang::RecordDecl::isPassInRegisters

Can't agree more. Thanks!!

kusmour updated this revision to Diff 202517.May 31 2019, 5:57 PM

added a virtual function CanPassInRegister in TypeSystem class
to provide info about type can be passed in register or not
ClangASTContext will refer to clang::RecordDecl::canPassInRegister for this info
and later SwiftASTContext can provide the equivalent
updated the test to test on C++
and provided class test, including base class, subclass, abstract class (this one must be in memory)

kusmour marked 3 inline comments as done.Jun 1 2019, 12:44 AM
compnerd added inline comments.Jun 1 2019, 10:02 AM
lldb/source/Symbol/ClangASTContext.cpp
3915 ↗(On Diff #202517)

I think that using auto instead of clang::RecordDecl here is fine as you are already spelling that out in the ClangASTContext::GetAsRecordDecl.

kusmour marked 2 inline comments as done.Jun 1 2019, 4:52 PM
kusmour added inline comments.
lldb/source/Symbol/ClangASTContext.cpp
3915 ↗(On Diff #202517)

got it

kusmour updated this revision to Diff 202574.Jun 1 2019, 4:52 PM
kusmour marked an inline comment as done.

small update :)

kusmour updated this revision to Diff 202824.Jun 3 2019, 5:03 PM
  1. Limit the TestReturnValue for nested struct and class (cpp support) to only x86_64
  2. This patch somehow fix the bug: pr36870 for Systerm V ABI (windows is waiting for this change to go in). So I change the test to allow passing on SysV-x86_64
This revision was not accepted when it landed; it landed in state Needs Review.Jun 4 2019, 12:27 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 4 2019, 12:27 PM
lldb/trunk/packages/Python/lldbsuite/test/functionalities/return-value/TestReturnValue.py