Page MenuHomePhabricator

[AArch64] Treat x18 as callee-saved in functions with windows calling convention on non-windows OSes
ClosedPublic

Authored by mstorsjo on May 14 2019, 6:03 AM.

Details

Summary

Treat it as callee-saved, and always back it up. When windows code calls entry points in unix code, marked with the windows calling convention, that unix code can call other functions that isn't compiled with -ffixed-x18 which may clobber x18 freely. By backing it up and restoring it on return, we preserve the register across the function call, fulfilling this part of the windows calling convention on another OS.

This isn't enough for making sure that x18 is preseved when non-windows code does a callback to windows code, but is a clear improvement over the current status quo. Additionally, wine is nowadays building many modules as PE DLLs, which avoids the callback issue altogether for those DLLs.

Diff Detail

Event Timeline

mstorsjo created this revision.May 14 2019, 6:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 14 2019, 6:03 AM
mstorsjo updated this revision to Diff 264837.May 19 2020, 3:33 AM
mstorsjo retitled this revision from [RFC] [AArch64] Back up and restore X18 in the prologue/epilogue for Windows calling convention on non-Windows OSes to [AArch64] Treat x18 as callee-saved in functions with windows calling convention on non-windows OSes.
mstorsjo edited the summary of this revision. (Show Details)
mstorsjo added a subscriber: jacek.

Removing the RFC marking from this one, with the intention of getting it merged. It's a pretty non-intrusive (and cheap) fix and for maintaining x18 across wine built in functions - I've tested it in daily use for a month now and the fix does seem to behave as intended. Wine builds most DLLs as PE DLLs these days, so callbacks is no longer an issue for the built-in modules built as PE DLLs.

This comment was removed by mstorsjo.

@rnk - if you have time before you go on your leave - could you have a look at this one?

Ping - who can review AArch64 backend stuff?

efriedma added inline comments.
llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
2447

This change shouldn't be necessary. If some code in the function modifies x18, or calls some non-Win64 function, x18 will be clobbered. Frame lowering should see that, and figure out it needs to save x18 without any extra help.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
3947

Do we want to also check that the callee CC isn't Win64?

mstorsjo marked 2 inline comments as done.May 29 2020, 6:35 AM
mstorsjo added inline comments.
llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
2447

No, this is actually the core of the intent of this change.

When windows code (which expects x18 to be unmodified) calls an entry point in wine (marked with the windows calling convention), the entry point function (linux code but with a windows calling convention) itself probably won't clobber x18 (it's potentially even compiled with -ffixed-x18 to avoid doing that) - but this function can call other functions (glibc, other userland libraries), and these functions can freely clobber x18.

So this is meant to always restore x18 on return from a windows calling convention function, as we can't really know what happened meanwhile.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
3947

Hmm, indeed - in that case we could actually allow the tail call, yes. I'll see if I can add a test for that case.

mstorsjo marked an inline comment as done.May 29 2020, 11:33 AM
mstorsjo added inline comments.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
3947

Actually, we don't produce tail calls for the Win64 calling convention at all right now - it isn't in mayTailCallThisCC above at all. I guess that _should_ be fine but I'm not sure if it'd break anything, and I think adding it in this patch would be a bit out of scope. I can add that bit into the condition here though, but that case wouldn't be usable just yet at least, and not be tested here.

mstorsjo updated this revision to Diff 267313.May 29 2020, 11:42 AM

Clarified the comment about always backing up x18, added a condition about CalleeCC.

efriedma added inline comments.May 29 2020, 12:25 PM
llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
2447

Hmm...

So the functions using the Windows calling convention are compiled with -ffixed-x18, which means they don't treat x18 as allocatable at all. And potentially, Unix calling convention code in the same file is compiled the same way. But eventually, somewhere in the callstack, there might be a function that clobbers x18, so you want to save/restore it despite the fact that it's marked "fixed".

If x18 is fixed anyway, why do you need to mess with AArch64RegisterInfo::getCalleeSavedRegs? It shouldn't participate in any computation related to the calling convention.

It seems a little magical to me that functions using the Win64 calling convention are making assumptions about the behavior of x18 despite the -ffixed-x18 marking. The alternative would be to add a function attribute to specifically request this behavior, I guess, separate from the calling convention. That would separate the concerns a bit more, but realistically I'm not sure anyone other than Wine is using the Win64 convention on non-Windows targets, so maybe it's not important.

mstorsjo marked an inline comment as done.May 29 2020, 12:55 PM
mstorsjo added inline comments.
llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
2447

Well, technically, the functions don't need to be compiled with -ffixed-x18, strictly, this should patch works the same either way, just that adding that flag reduces the risk of it being clobbered while in the wine code itself as well. (Fixing that aspect properly is a different issue which is handled in a different way.)

If x18 is fixed anyway, why do you need to mess with AArch64RegisterInfo::getCalleeSavedRegs? It shouldn't participate in any computation related to the calling convention.

If x18 is fixed, we don't need that bit, but if it isn't, it's necessary.

realistically I'm not sure anyone other than Wine is using the Win64 convention on non-Windows targets, so maybe it's not important.

Yeah I don't think practically any other code base really would be using it (other than potential windows binary loaders used in other projects, i.e. doing the same - but that's probably not really prevalent for anything else than x86).

efriedma added inline comments.May 29 2020, 1:12 PM
llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
2447

Please make sure we have test coverage for both the fixed-x18 and non-fixed-x18 cases, then.

mstorsjo updated this revision to Diff 267357.May 29 2020, 1:35 PM

Added a testcase with -mattr=+reserve-x18, in addition to the previous one without it.

This revision is now accepted and ready to land.May 29 2020, 2:11 PM
This revision was automatically updated to reflect the committed changes.