Page MenuHomePhabricator

Fix compilation faliures for x64 visual studio 2013 build
ClosedPublic

Authored by ADodds on Dec 17 2014, 6:59 AM.

Details

Summary

The visual studio 2013 x64 build is failing due to some type differences between 32bit and 64bit builds.

The CONTEXT structure used in RegisterContextWindows_x86 changes its fields based on the build.
Currently I have used #ifdefs to alternate between the different registers for each build, is this an acceptable solution?

It seems however that HostThreadWindows is largely 32bit specific, so should there rather be a dedicated x64 version?

In HostThreadWindows its seems that ULONG_PTR also changes its size based on the machine type, however the argument for ExitThread() doesnt change in a simmilar way.
I have created a proxy function to wrap ExitThread() so it can again be used as a function pointer.

If these changes are acceptable, would someone be able to commit these fixes since I dont currently have commit access.

Thanks,
Aidan

Diff Detail

Repository
rL LLVM

Event Timeline

ADodds updated this revision to Diff 17392.Dec 17 2014, 6:59 AM
ADodds retitled this revision from to Fix compilation faliures for x64 visual studio 2013 build.
ADodds updated this object.
ADodds edited the test plan for this revision. (Show Details)
ADodds added reviewers: zturner, clayborg, emaste, domipheus.
ADodds added a subscriber: Unknown Object (MLST).
zturner edited edge metadata.Dec 17 2014, 7:18 AM

I'll take a look later today.

I don't think this is the right fix. For x64 we should implement an entirely different RegisterContext class in a file called RegisterContext_x86_64. At some point we're going to want support debugging x86 targets with an x64 build of LLDB, but that has its own set of issues (notably that we will need to use Wow64GetThreadContext and a PWOW64_CONTEXT structure). So that will probably also require its own RegisterContext implementation. So the correct fix for this is to simply not compile this file on x64. You can do that at the CMake level by editing lldb\source\Plugins\Process\Windows\CMakeLists.txt and conditionally adding RegisterContext_x86.cpp only for builds where CMAKE_SIZEOF_VOID_P EQUAL 4.

Then, in ProcessWindows.cpp you'll need to wrap any lines that #include "RegisterContext_x86.h" or allocate a RegisterContext_x86 object in a similar #ifdef.

ADodds planned changes to this revision.Dec 17 2014, 9:59 AM

Thanks for taking a look at this.
I had an idea that the correct fix may be just what you have mentioned.
I will revise this patch to bypass this file for x64 builds as you advise.

Selectively including .cpp files in CMake may be harder then it first looks since the LLVM cmake automaticaly throw an error when it finds unincluded .cpp files.
When I tried to bypass RegisterContextWindows_x86.cpp for x64 targets I get the following error:

CMake Error at cmake/modules/LLVMProcessSources.cmake:70 (message):
  Found unknown source file
  D:/tiplldb/llvm/tools/lldb/source/Plugins/Process/Windows/RegisterContextWindows_x86.cpp


  Please update
  D:/tiplldb/llvm/tools/lldb/source/Plugins/Process/Windows/CMakeLists.txt

Call Stack (most recent call first):
  cmake/modules/LLVMProcessSources.cmake:44 (llvm_check_source_file_list)
  tools/lldb/CMakeLists.txt:230 (llvm_process_sources)
  tools/lldb/source/Plugins/Process/Windows/CMakeLists.txt:20 (add_lldb_library)

For reference, here was my modifications to lldb/source/plugins/process/windows/cmakelists.txt:

set(LLVM_NO_RTTI 1)

include_directories(.)
include_directories(../Utility)

set(lldbPluginProcessWindowsSources
  DebuggerThread.cpp
  DynamicLoaderWindows.cpp
  LocalDebugDelegate.cpp
  ProcessWindows.cpp
  TargetThreadWindows.cpp
  )

if (CMAKE_SIZEOF_VOID_P EQUAL 4)
  list(lldbPluginProcessWindowsSources APPEND
    RegisterContextWindows_x86.cpp
    )
endif()

add_lldb_library(lldbPluginProcessWindows
  ${lldbPluginProcessWindowsSources}
  )

Do you know any way to work around this?
Or do you have any suggestions how we can fix the build for x64 targets in the short term?

Thanks

Easiest way is to Make a subfolder called x86 and move the files there.
Then selectively include them from the subfolder. Host does something
similar if you want an example

ADodds updated this revision to Diff 17447.Dec 18 2014, 9:05 AM
ADodds edited edge metadata.

I have reworked my initial patch with the feedback from Zachary.
I have tested this patch on both x86 and x64 windows builds and successfully build lldb on both.

Both RegisterContextWindows_x86.cpp and .h were not modified only modev, however the diff is a little verbose in this respect.

Thanks

Looks good after addressing these comments.

source/Plugins/Process/Windows/CMakeLists.txt
11 ↗(On Diff #17447)

Change "lldbPluginProcessWindows" to "common". This controls how it will show up in Solution Explorer but otherwise doesn't mean anything, so it should be descriptive of the category.

20 ↗(On Diff #17447)

Change "lldbPluginProcessWindows" to "x86".

source/Plugins/Process/Windows/TargetThreadWindows.cpp
20 ↗(On Diff #17447)

I don't think we will ever build for itanium, and a better check for 64-bitness is to use _WIN64. So to be consistent with other places in the Windows codepath where we will need to check for 64-bitness, I would change this to

#if !defined(_WIN64)

78 ↗(On Diff #17447)

Same here, change to

#if defined(_WIN64)

Actually since you don't have commit access, I will go ahead and make these fixes and then commit.

zturner added inline comments.Dec 18 2014, 10:05 AM
source/Plugins/Process/Windows/TargetThreadWindows.cpp
79–82 ↗(On Diff #17447)

Also, just as a heads up, I made one more change to the comment here. if the triple is x86 and the host architecture is x64, that's a Wow64 process, not an x86_64 process. That would occur if both the triple and the host architecture were x64. So I added an empty case to the switch to handle llvm::Triple::x64 and moved your comment there, and changed this comment to refer to a RegisterContextWindows_Wow64.

Thanks for the insightful feedback and for commiting this change Zachary.

This revision was automatically updated to reflect the committed changes.