Page MenuHomePhabricator

[PowerPC][ELF]make sure local variable space does not overlap with parameter save area
ClosedPublic

Authored by shchenz on Jul 1 2021, 2:50 AM.

Details

Summary

This is a bug fix for byval parameter passing in the caller site.

Currently in the caller site, byval parameter which size is not smaller than 8 will be stored into parameter save area.

See:

LowerCall_64SVR4  -> createMemcpyOutsideCallSeq()

But in the previous HasParameterArea checking, it is not set to true. So this will cause the stack memory for local variable space and parameter save area overlap.

Diff Detail

Event Timeline

shchenz created this revision.Jul 1 2021, 2:50 AM
shchenz requested review of this revision.Jul 1 2021, 2:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 1 2021, 2:50 AM
shchenz updated this revision to Diff 355810.Jul 1 2021, 2:55 AM

delete the auto gen comments in testcase.

jsji added inline comments.Jul 7 2021, 9:21 AM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
6034

This doesn't look like a correct fix to me.
I think we rely on CalculateStackSlotUsed for non FastCall ELFV2ABI.
Do we know why CalculateStackSlotUsed is returning false for this case?

shchenz added inline comments.Jul 7 2021, 6:15 PM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
6034

Thanks, I will do more investigation. Actually, I find the stack size is unnecessary bigger than needed after the fix.

shchenz planned changes to this revision.Jul 7 2021, 6:52 PM
shchenz updated this revision to Diff 357154.EditedJul 8 2021, 1:05 AM

1: the unexpected size is caused by: there is a minimal parameter save area requirement which is 64 bytes. It is bigger than the needed 36 bytes.

2: Because the parameter size is 36 bytes, it can be passed by registers, so current CalculateStackSlotUsed return false. But because the byval parameter may be assumed in caller's parameter save area in the callee in other compiler, so we must also store the byval parameter in parameter save area of the caller function. Maybe we need a revisit for this case. For my understanding, there is no need to store byval parameter in the parameter save area in the caller site if the parameters can be passed in registers. Callee site should copy the byval parameter to its own stack memory, so any operation will not impact the caller site. Not sure which gcc version requires this and from the later comments, seems this is not implemented? https://github.com/llvm/llvm-project/blob/21fd8759529707b0f4430ebe8f27a01edc7f655e/llvm/lib/Target/PowerPC/PPCISelLowering.cpp#L6172-L6181 But this should be another improvement issue from this functionality one.

jsji added a comment.Jul 9 2021, 11:38 AM
This comment was removed by jsji.
jsji added inline comments.Jul 9 2021, 11:42 AM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
3923–3929

Should we check the ELFV2ABI here?

Also type check?

jsji added inline comments.Jul 9 2021, 11:44 AM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
3924

comment are referring to the opposite? Should we describe the ABI about size >=8?

shchenz updated this revision to Diff 357839.Jul 11 2021, 10:11 PM

1: add explicitly check for abi

Thanks for the review @jsji. Updated accordingly.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
3923–3929

I added ABI check, I think we don't need the type check, as here the type is byval and it is always a pointer to a structure or a scalar type, we just need to know its size.

3924

I think saving the byval parameter (size>=8bytes) into the stack is not required by ABI. This is for compatibility with GCC. See https://github.com/llvm/llvm-project/blob/21fd8759529707b0f4430ebe8f27a01edc7f655e/llvm/lib/Target/PowerPC/PPCISelLowering.cpp#L6172-L6181.
I think we need to reinvestigate this requirement for byval parameter on Linux. Saving byval parameter in caller's stack frame is not necessary IMO, for example, we don't do this on AIX. And from the latter comments of the above codes, GCC seems also does not require this in its implementation

gentle ping

gentle ping

jsji accepted this revision as: jsji.Aug 26 2021, 7:25 AM

LGTM. Sorry for missing pings .

This revision is now accepted and ready to land.Aug 26 2021, 7:25 AM
This revision was landed with ongoing or failed builds.Aug 26 2021, 6:59 PM
This revision was automatically updated to reflect the committed changes.