Page MenuHomePhabricator

[Hexagon] Add support for Linux/Musl ABI.
ClosedPublic

Authored by sidneym on Jan 14 2020, 6:37 AM.

Details

Summary

The patch adds a new option -mllvm enable-linux-abi. The patch
primary deals with the way variable arguments are passed.

If a callee function has a variable argument list, it must perform the
following operations to set up its function prologue:

  1. Determine the number of registers which could have been used for passing unnamed arguments. This can be calculated by counting the number of registers used for passing named arguments. For example, if the callee function is as follows:

    int foo(int a, ...){ ... }

    ... then register R0 is used to access the argument ' a '. The registers available for passing unnamed arguments are R1, R2, R3, R4, and R5.
  2. Determine the number and size of the named arguments on the stack.
  3. If the callee has named arguments on the stack, it should copy all of these arguments to a location below the current position on the stack, and the difference should be the size of the register-saved area plus padding (if any is necessary).

    The register-saved area constitutes all the registers that could have been used to pass unnamed arguments. If the number of registers forming the register-saved area is odd, it requires 4 bytes of padding; if the number is even, no padding is required. This is done to ensure an 8-byte alignment on the stack. For example, if the callee is as follows:

    int foo(int a, ...){ ... }

    ... then the named arguments should be copied to the following location:

    current_position - 5 (for R1-R5) * 4 (bytes) - 4 (bytes of padding)

    If the callee is as follows:

    int foo(int a, int b, ...){ ... }

    ... then the named arguments should be copied to the following location:

    current_position - 4 (for R2-R5) * 4 (bytes) - 0 (bytes of padding)
  4. After any named arguments have been copied, copy all the registers that could have been used to pass unnamed arguments on the stack. If the number of registers is odd, leave 4 bytes of padding and then start copying them on the stack; if the number is even, no padding is required. This constitutes the register-saved area. If padding is required, ensure that the start location of padding is 8-byte aligned. If no padding is required, ensure that the start location of the on-stack copy of the first register which might have a variable argument is 8-byte aligned.
  5. Decrement the stack pointer by the size of register saved area plus the padding. For example, if the callee is as follows:

    int foo(int a, ...){ ... } ;

    ... then the decrement value should be the following:

    5 (for R1-R5) * 4 (bytes) + 4 (bytes of padding) = 24 bytes

    The decrement should be performed before the allocframe instruction. Increment the stack-pointer back by the same amount before returning from the function.

Diff Detail

Event Timeline

sidneym created this revision.Jan 14 2020, 6:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 14 2020, 6:37 AM
Herald added a subscriber: hiraditya. · View Herald Transcript

LGTM. @kparzysz what do you think?

pcc added a subscriber: pcc.Jan 15 2020, 12:07 PM

I know almost nothing about Hexagon, but shouldn't this be determined by the target triple, or at least a target-feature?

In D72701#1822556, @pcc wrote:

I know almost nothing about Hexagon, but shouldn't this be determined by the target triple, or at least a target-feature?

When the target is Linux the clang driver will add this option.

pcc added a comment.Jan 15 2020, 3:28 PM
In D72701#1822556, @pcc wrote:

I know almost nothing about Hexagon, but shouldn't this be determined by the target triple, or at least a target-feature?

When the target is Linux the clang driver will add this option.

So shouldn't the code here be checking whether the target is Linux using Triple::isOSLinux() instead of adding a new option?

sidneym updated this revision to Diff 238816.Jan 17 2020, 10:13 AM
sidneym retitled this revision from [Hexagon] Add support for Linux ABI. to [Hexagon] Add support for Linux/Musl ABI..

Update code to check for the Musl environment.

Checking OS was too vague and could result in this ABI being unexpectedly enabled if the triple was not set and the host tools were built for Linux. Musl is the only C-library currently used by Hexagon Linux so
that is the check I'm using to enable the ABI.

This revision is now accepted and ready to land.Jan 17 2020, 10:45 AM
This revision was automatically updated to reflect the committed changes.
dalias added a subscriber: dalias.Jan 20 2020, 9:49 AM

Forgive my jumping into this without knowing all the details, but is there a public intended-for-upstreamability hexagon arch definition for musl? If not, are there assumptions about musl and the libc-defined part of the ABI in this changeset that might depend on or conflict with what's eventually added upstream?

Forgive my jumping into this without knowing all the details, but is there a public intended-for-upstreamability hexagon arch definition for musl? If not, are there assumptions about musl and the libc-defined part of the ABI in this changeset that might depend on or conflict with what's eventually added upstream?

Supporting MUSL didn't require any ABI changes but moving to a Linux OS allowed us to fix some issues in the original ABI. Just checking for the OS could cause this ABI to be accidentally selected so I used the environment, musl to limit when it could be enabled.
I'm working on getting permission for a public github to host the bits we needed to add to MUSL to support Hexagon. I thought having a "working" reference to examine and then posting a series of patches for review to the musl-mailing list would be the easiest way to go. I admit I have not yet reached out the musl mailing list to confirm this because I wanted to have the github repo up first, but how does this plan sound to you?

Thanks