Page MenuHomePhabricator

[ABI] Implement Windows ABI for x86_64
ClosedPublic

Authored by kusmour on May 21 2019, 11:26 AM.

Details

Summary

Implement the ABI for WIndows-x86_64 including register info and calling convention.
Handled nested struct returned in register (SysV doesn't have it supported)

Diff Detail

Repository
rL LLVM

Event Timeline

kusmour created this revision.May 21 2019, 11:26 AM
compnerd added inline comments.May 21 2019, 2:54 PM
lldb/source/Plugins/ABI/SysV-x86_64/ABISysV_x86_64.cpp
1094 ↗(On Diff #200546)

This really isn't correct. There is no reason to assume that everything on x86_64 uses the SysV ABI. Can you convert this to a switch over the OS type and return it from the known OSes (Linux, BSD, and Darwin do conform to SysV, the others can return ABISP().

lldb/source/Plugins/ABI/Windows-x86_64/ABIWindows_x86_64.cpp
1078 ↗(On Diff #200546)

Ugh, this really feels like copy and paste. Perhaps we should have a .inc file that we can use to replicate the reigster names across the targets.

1104 ↗(On Diff #200546)

Nit: the indentation seems off.

1203 ↗(On Diff #200546)

Mind using CHAR_BIT instead of 8?

1359 ↗(On Diff #200546)

FP80 is not supported on Windows, this should be a hard error.

1466 ↗(On Diff #200546)

long double is the same as double on Windows.

1492 ↗(On Diff #200546)

lmao, is this a copy paste thing? I think that xmm_reg is better than altivec_reg as ALTIVEC is PPC specific.

1573 ↗(On Diff #200546)

Mind using CHAR_BIT instead of 8 please?

lldb/source/Plugins/ABI/Windows-x86_64/ABIWindows_x86_64.h
55 ↗(On Diff #200546)

This comment doesn't make sense on Windows - signal handlers don't really apply to Windows.

59 ↗(On Diff #200546)

This should be made strict as per the Windows ABI.

63 ↗(On Diff #200546)

Can we add an additional test please? In particular, leaf frames are permitted to have unaligned stacks.

xiaobai added inline comments.May 21 2019, 3:26 PM
lldb/source/Plugins/ABI/Windows-x86_64/ABIWindows_x86_64.cpp
1257–1259 ↗(On Diff #200546)

I don't see any references to clang in the below code. Is this still accurate?

1388 ↗(On Diff #200546)

Why is this commented out? If it's unneeded, please remove it

1811 ↗(On Diff #200546)

nit: Windows-x86_64 doesn't use rbp

kusmour updated this revision to Diff 200622.May 21 2019, 6:10 PM

update a new version based on comment.
cleaned some useless comment
change to use CHAR_BIT for readability

kusmour marked 16 inline comments as done.May 21 2019, 6:29 PM
kusmour added inline comments.
lldb/source/Plugins/ABI/Windows-x86_64/ABIWindows_x86_64.cpp
1257–1259 ↗(On Diff #200546)

not relevant, deleted

1359 ↗(On Diff #200546)

I changed it to provide a correct error message. Is it better to put an assertion or return here?

1466 ↗(On Diff #200546)

got it thanks

1811 ↗(On Diff #200546)

thanks

lldb/source/Plugins/ABI/Windows-x86_64/ABIWindows_x86_64.h
59 ↗(On Diff #200546)

windows is 16-byte aligned, updated.

63 ↗(On Diff #200546)

can you provide more information about this? On windows a leaf function doesn't require a stack frame. But a frame function that doesn't call other function is not required to align the stack

jasonmolenda added inline comments.
lldb/source/Plugins/ABI/Windows-x86_64/ABIWindows_x86_64.h
63 ↗(On Diff #200546)

I'm not sure if the Phabracator comments are pointing to the correct bits of sourcecode - you're talking about CallFrameAddressIsValid right? This is checking the stack frame's Canonical Frame Address -- it is a check that the caller's $rsp was 16-byte aligned before it CALLed a (possibly leaf). It isn't a check of the $rsp value in the callee. The unwinder uses this as a sanity check to detect when the backtrace has bogus values as it walks up the stack.

Also, not sure where this comment thread is :) but re. CodeAddressIsValid - return true because this is the x86 ISA where there is no instruction alignment restrictions. This method is for architectures with fixed width instructions (okay, where"fixed width" is greater than 1 :) to help detect backtrace mistakes (again). e.g. aarch64/arm64 instructions are always 4-byte aligned.

kusmour marked 3 inline comments as done.May 22 2019, 10:59 AM
compnerd accepted this revision.May 22 2019, 4:33 PM
compnerd added inline comments.
lldb/source/Plugins/ABI/SysV-x86_64/ABISysV_x86_64.cpp
1094 ↗(On Diff #200546)

Nit: a switch is clearer and easier to extend.

lldb/source/Plugins/ABI/Windows-x86_64/ABIWindows_x86_64.cpp
1096 ↗(On Diff #200622)

Nit: I think that arch.GetTriple().isOSWindows() is nicer than the explicit check of Win32.

lldb/source/Plugins/ABI/Windows-x86_64/ABIWindows_x86_64.h
47 ↗(On Diff #200622)

The comment seems wrong

This revision is now accepted and ready to land.May 22 2019, 4:33 PM
kusmour marked an inline comment as done.May 22 2019, 5:18 PM
kusmour added inline comments.
lldb/source/Plugins/ABI/Windows-x86_64/ABIWindows_x86_64.cpp
1096 ↗(On Diff #200622)

got it

kusmour updated this revision to Diff 200844.May 22 2019, 5:21 PM

update nit

kusmour updated this revision to Diff 201377.May 24 2019, 6:27 PM

update the CreateDefaultUnwindPlan.
return false or give empty Unwind Plan may cause lldb to crash and break tests on windows.
So for now, copy the SysV-x86_64

kusmour updated this revision to Diff 203061.Jun 4 2019, 7:54 PM

Update the GetReturnValueObjectImpl using function CanPassInRegisters to explicitly check

NOTE: There's NO register context info about registers beyond general purpose registers So floating point return type is not supported since it uses XMM0 to return need to update register context for windows x64
kusmour updated this revision to Diff 204695.Jun 13 2019, 10:02 PM

Added support for XMM registers.
Now the step out on a function returns floating point number should have the right value.
To play with it, define a function

float getFloat(float value) {
    return value;
}

set a break point and step out
LLDB will print out the right value if debugging on Windows-x86_64

kusmour updated this revision to Diff 204885.Jun 14 2019, 5:15 PM

complete the code for floating point registers and cleaned up
the support for floating point registers fixed TestRegistersIterator update the testcased

kusmour updated this revision to Diff 205390.Jun 18 2019, 10:35 AM

the OS check in SysV x64 ABI broke the test SymbolFile/DWARF/debug_loc.s
added a restriction lld to #REQUIRES

labath added a subscriber: labath.Jun 18 2019, 10:55 AM

the OS check in SysV x64 ABI broke the test SymbolFile/DWARF/debug_loc.s

Can you elaborate on that? How exactly did it break this test?

added a restriction lld to #REQUIRES

That definitely is not the right solution here. If you add lld to your checkout on windows, then "REQUIRES: lld" will be satisfied, but the test will (I guess) still fail. "XFAIL: system-windows" is probably what you want here, but it would be good to understand why this starts to fail now. Is it because we fail to detect that the file in question there is a linux file, and so the triple comes out like "x86_64--" or something? Maybe we should make the SysV abi plugin match unknown oss too... The only format where we have trouble determining the OS is ELF, and it's a pretty good bet that any elf file will be using the "SysV" ABI...

@labath Hi, you're right about the lld, it didn't make the test pass, instead it became unsupported.

Can you elaborate on that? How exactly did it break this test?

So the problem here is that, llvm-mc constructs the Triple correctly, but lldb does not preserve all the information from it, at least it does not know about the OS type.

Maybe we should make the SysV abi plugin match unknown oss too...

I'm not sure this is the right thing to do, but this should fix the problem.

The only format where we have trouble determining the OS is ELF, and it's a pretty good bet that any elf file will be using the "SysV" ABI...

Can you provide more information about this? It sounds like a bug

Maybe we should make the SysV abi plugin match unknown oss too...

I'm not sure this is the right thing to do, but this should fix the problem.

It would at least preserve status quo. Not much, but it is something...

The only format where we have trouble determining the OS is ELF, and it's a pretty good bet that any elf file will be using the "SysV" ABI...

Can you provide more information about this? It sounds like a bug

Well.. you could consider it a bug, but it's quite hard to say where that bug is (it's definitely not a bug in lldb or llvm-mc). A sad fact of life is that the ELF files don't usually carry enough information to properly identify the os/environment they were intended to run on. If you run readelf -e on the file generated in the test, you'll get something like

ELF Header:
  Magic:   7f 45 4c 46 02 01 01 00 00 00 00 00 00 00 00 00 
  Class:                             ELF64
  Data:                              2's complement, little endian
  Version:                           1 (current)
  OS/ABI:                            UNIX - System V
  ABI Version:                       0
  Type:                              REL (Relocatable file)
  Machine:                           Advanced Micro Devices X86-64
  Version:                           0x1
  Entry point address:               0x0
  Start of program headers:          0 (bytes into file)
  Start of section headers:          648 (bytes into file)
  Flags:                             0x0
  Size of this header:               64 (bytes)
  Size of program headers:           0 (bytes)
  Number of program headers:         0
  Size of section headers:           64 (bytes)
  Number of section headers:         9
  Section header string table index: 1

Section Headers:
  [Nr] Name              Type             Address           Offset
       Size              EntSize          Flags  Link  Info  Align
  [ 0]                   NULL             0000000000000000  00000000
       0000000000000000  0000000000000000           0     0     0
  [ 1] .strtab           STRTAB           0000000000000000  00000238
       000000000000004e  0000000000000000           0     0     1
  [ 2] .text             PROGBITS         0000000000000000  00000040
       0000000000000003  0000000000000000  AX       0     0     4
  [ 3] .debug_str        PROGBITS         0000000000000000  00000043
       000000000000001b  0000000000000001  MS       0     0     1
  [ 4] .debug_loc        PROGBITS         0000000000000000  0000005e
       0000000000000036  0000000000000000           0     0     1
  [ 5] .debug_abbrev     PROGBITS         0000000000000000  00000094
       000000000000002d  0000000000000000           0     0     1
  [ 6] .debug_info       PROGBITS         0000000000000000  000000c1
       000000000000003d  0000000000000000           0     0     1
  [ 7] .rela.debug_info  RELA             0000000000000000  00000190
       00000000000000a8  0000000000000018           8     6     8
  [ 8] .symtab           SYMTAB           0000000000000000  00000100
       0000000000000090  0000000000000018           1     6     8
Key to Flags:
  W (write), A (alloc), X (execute), M (merge), S (strings), I (info),
  L (link order), O (extra OS processing required), G (group), T (TLS),
  C (compressed), x (unknown), o (OS specific), E (exclude),
  l (large), p (processor specific)

There are no program headers in this file.

The elf header has this OS/ABI field which could be used to specify linux, but here it's just set to the generic "none" value (which gets printed as "unix sysv"). There is a value for ELFOSABI_LINUX/ELFOSABI_GNU, which could be used to say "linux", but it seems nobody uses it (I'm not sure what's the background there). I guess that usually wasn't a problem, because tools would just assume they are dealing with binaries for the host platform, but lldb has a strong desire to know what platform is a binary intended for without assuming anything about the environment it's being run it. That desire is understandable, but it kind of collides with reality. Over time, lldb has developed a bunch of heuristics to determine the right OS for a file, but these are just heuristics, and they fail on extremely simple files like the ones used in this test, because they have nothing to go on.

kusmour updated this revision to Diff 205640.Jun 19 2019, 10:56 AM

allow SysV_x86_64 ABI to handle unknown OS type
this should fix the test SymbolFile/DWARF/debug_loc.s

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 24 2019, 11:23 AM
mgorny added inline comments.Jun 27 2019, 12:06 AM
lldb/trunk/source/Plugins/ABI/SysV-x86_64/ABISysV_x86_64.cpp
231

There's NetBSD missing here. This broke our buildbots. I'll fix it but please be more careful in the future.

asmith added a subscriber: asmith.Jul 29 2019, 5:37 PM

I think this change has introduced a dependence on x64.

I tried building with the 32b Visual Studio compiler and there are a number of undefined registers in registercontextwindows_x64.cpp.
This is because the version of _CONTEXT included by winnt.h is for x32 and not x64.

i.e., llvm-project\lldb\source\plugins\process\windows\common\x64\registercontextwindows_x64.cpp(297): error C2039: 'Rax': is not a member of '_CONTEXT'

Thoughts? Maybe this doesn't need to be included when building x32 LLDB?

I think this change has introduced a dependence on x64.

I tried building with the 32b Visual Studio compiler and there are a number of undefined registers in registercontextwindows_x64.cpp.
This is because the version of _CONTEXT included by winnt.h is for x32 and not x64.

i.e., llvm-project\lldb\source\plugins\process\windows\common\x64\registercontextwindows_x64.cpp(297): error C2039: 'Rax': is not a member of '_CONTEXT'

This seems kind of strange to me. I didn't think you could actually build RegisterContextWindows_x64.cpp on an x86 machine. From the CMake logic, it looks like it's guarded by CMAKE_SYSTEM_PROCESSOR matching either "x86_64" or "AMD64". Do you have any local changes or a specific CMake setup that caused you to run into this?

Thoughts? Maybe this doesn't need to be included when building x32 LLDB?

I would rather be able to build this all on x86 if possible, but I don't think it's the end of the world if there's no nice way to do this. There is plenty of code in LLDB guarded by what platform you're building on/for, this would just be another instance of it.

I can confirm this is reproducible with the master branch of llvm-project cloned from GitHub.

You can reproduce the build failure from an x64 command prompt with this cmake command line:

cmake path-to-llvm-project/llvm -Thost=x64 -DLLVM_ENABLE_PROJECTS="clang;lld;lldb" -DCLANG_DEFAULT_LINKER=lld -DLLVM_ENABLE_ASSERTIONS=ON

And building with:

msbuild ALL_BUILD.vcxproj /m

In this configuration CMAKE_SYSTEM_PROCESSOR is going to report AMD64 and this is going to use the x64 hosted Visual Studio compiler targeting 32 bit Windows. Probably some of these files need to be wrapped in #if defined(_M_X64).

lldb/trunk/source/Plugins/ABI/Windows-x86_64/ABIWindows_x86_64.h
1

Nit; number of dashes '----' is wrong in the headers of the files added

You might want to have a look at https://reviews.llvm.org/D65409 which might fix (some of?) the issues mentioned here.

I don't know if this is relevant or not, but I thought I'd say that on linux we also effectively aren't able to debug a 64-bit process with a 32-bit lldb(-server). (effectively = that's not entirely true, and the situation is a bit more subtle, but the overall effect is the same).

asmith added a comment.Aug 6 2019, 6:43 AM

Do you need some help fixing the build?