Skip to content

Commit

Permalink
Fix big endian Power system linkage stack parameter alignment
Browse files Browse the repository at this point in the history
On big endian Power, there was an issue with system linkage that was
causing parameters passed via the stack to be loaded incorrectly.
Specifically, the ABI specifies that the entire GPR's contents should be
saved on the stack, however the system linkage code was performing a
load of only the parameter size. While this works on little endian
systems, this causes the wrong bits of the argument to be loaded on big
endian systems.

This has been corrected by adjusting the stack offsets of parameters on
big endian systems so that the correct bytes of the full GPR contents
will be loaded. This should also correct #3525 on Power.

Fixes: #4765
Signed-off-by: Ben Thomas <ben@benthomas.ca>
  • Loading branch information
aviansie-ben committed Feb 4, 2020
1 parent b1ac27f commit cb0ff02
Show file tree
Hide file tree
Showing 3 changed files with 25 additions and 9 deletions.
6 changes: 4 additions & 2 deletions compiler/p/codegen/OMRLinkage.cpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*******************************************************************************
* Copyright (c) 2000, 2019 IBM Corp. and others
* Copyright (c) 2000, 2020 IBM Corp. and others
*
* This program and the accompanying materials are made available under
* the terms of the Eclipse Public License 2.0 which accompanies this
Expand Down Expand Up @@ -166,7 +166,6 @@ TR::Instruction *OMR::Power::Linkage::saveArguments(TR::Instruction *cursor, boo
TR::DataType type = paramCursor->getType();
int32_t dtype = type.getDataType();


// TODO: Is there an accurate assume to insert here ?
if (lri >= 0)
{
Expand All @@ -191,6 +190,9 @@ TR::Instruction *OMR::Power::Linkage::saveArguments(TR::Instruction *cursor, boo
{
case TR::Int8:
case TR::Int16:
if (properties.getSmallIntParmsAlignedRight())
offset &= ~3;

case TR::Int32:
op = TR::InstOpCode::stw;
length = 4;
Expand Down
15 changes: 9 additions & 6 deletions compiler/p/codegen/OMRLinkage.hpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*******************************************************************************
* Copyright (c) 2000, 2019 IBM Corp. and others
* Copyright (c) 2000, 2020 IBM Corp. and others
*
* This program and the accompanying materials are made available under
* the terms of the Eclipse Public License 2.0 which accompanies this
Expand Down Expand Up @@ -68,11 +68,12 @@ class PPCMemoryArgument


// linkage properties
#define CallerCleanup 0x01
#define RightToLeft 0x02
#define IntegersInRegisters 0x04
#define LongsInRegisters 0x08
#define FloatsInRegisters 0x10
#define CallerCleanup 0x01
#define RightToLeft 0x02
#define IntegersInRegisters 0x04
#define LongsInRegisters 0x08
#define FloatsInRegisters 0x10
#define SmallIntParmsAlignedRight 0x20

// register flags
#define Preserved 0x01
Expand Down Expand Up @@ -139,6 +140,8 @@ struct PPCLinkageProperties

uint32_t getFloatsInRegisters() const {return (_properties & FloatsInRegisters);}

uint32_t getSmallIntParmsAlignedRight() const { return (_properties & SmallIntParmsAlignedRight); }

uint32_t getRegisterFlags(TR::RealRegister::RegNum regNum) const
{
return _registerFlags[regNum];
Expand Down
13 changes: 12 additions & 1 deletion compiler/p/codegen/PPCSystemLinkage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,9 @@ TR::PPCSystemLinkage::PPCSystemLinkage(TR::CodeGenerator *cg)
{
int i = 0;
_properties._properties = IntegersInRegisters|FloatsInRegisters|RightToLeft;
if (comp()->target().cpu.isBigEndian())
_properties._properties |= SmallIntParmsAlignedRight;

_properties._registerFlags[TR::RealRegister::NoReg] = 0;
_properties._registerFlags[TR::RealRegister::gr0] = 0;
_properties._registerFlags[TR::RealRegister::gr1] = Preserved|PPC_Reserved; // system sp
Expand Down Expand Up @@ -467,7 +470,11 @@ TR::PPCSystemLinkage::mapParameters(
int32_t offset_from_top = 0;
int32_t slot_size = sizeof(uintptrj_t);

bool saveParmsInLocalArea = comp()->target().bitness() == TR::endian_little && !hasParmsPassedOnStack(parmList);
// The 64-bit ELF V2 ABI Specification makes having a parameter save area optional if all parameters can be passed in
// registers. As a result, we cannot use the caller's parameter save area on 64-bit Linux if all parameters were
// passed in registers. However, we *must* use the caller's parameter save area if one or more parameters were passed
// on the stack to load those values correctly.
bool saveParmsInLocalArea = comp()->target().isLinux() && comp()->target().is64Bit() && !hasParmsPassedOnStack(parmList);

This comment has been minimized.

Copy link
@zl-wang

zl-wang Feb 5, 2020

Contributor

Although V2 is defined for both BE and LE, it is literally only supported/used by LE (which is also 64-bit only). Such that, the previous version of deciding saveParmsInLocalArea looks more correct (i.e. no caller's save area, if it is LE and there is no on-stack parm).

So far, I have only seen one instance of V2 BE Linux (which was not a major distro to begin with -- RedHat/SuSe/Ubuntu -- which we don't support anyway). The newer version could cause problems for existing (V1) BE 64bit Linux whose driver is still built off the same code base.


if (linkage.getRightToLeft())
{
Expand All @@ -477,6 +484,10 @@ TR::PPCSystemLinkage::mapParameters(
parmCursor->setParameterOffset(offset_from_top + stackIndex);
else
parmCursor->setParameterOffset(offset_from_top + offsetToFirstParm + stackIndex);

if (linkage.getSmallIntParmsAlignedRight() && parmCursor->getType().isIntegral() && parmCursor->getSize() < slot_size)
parmCursor->setParameterOffset(parmCursor->getParameterOffset() + slot_size - parmCursor->getSize());

offset_from_top += (parmCursor->getSize() + slot_size - 1) & (~(slot_size - 1));
parmCursor = parameterIterator.getNext();
}
Expand Down

0 comments on commit cb0ff02

Please sign in to comment.