Restructuring codebase #113

AlexAltea · 2018-10-23T19:46:15Z

DISCLAIMER: These are quite deep changes to the repository, we should probably make sure everyone agrees in the proposed changes down to the finest detail.

Goal is simply make the platform-specific changes consistent with each other, and keeping the root-level and source folders clean of platform-specific files.

This builds correctly under Windows+VS2017 and macOS+Xcode. Testing on Windows+EWDK is pending.
There's no functionality changes, so there should be no run-time regressions.

Changes

Generic changes:

Combined platform-specific folders. Moved {darwin,windows}/ to platforms/{darwin,windows}/, to avoid polluting the root folder. This is meant to reduce root-folder pollution, specially if support for more platforms (e.g. Linux) is added.
Combined documentation: Moved API.md to docs/api.md, and added platform-specific usage instructions in docs/manual-{darwin,windows}.md to avoid polluting README.md, specially if support for more platforms (e.g. Linux) is added.
Reorganized .gitignore files: One root-level .gitignore file with general rules that affect the entire source tree (e.g.: .DS_Store for macOS), and platform/compiler-specific rules at platforms/*/.gitignore to deal with build-related files (e.g.: *.vcxproj.user for Visual Studio, build/ for Xcode).
Improved consistency across platforms: Before, each platform acted,
- Renamed darwin/hax_driver/com_intel_hax to darwin (consistent folder depth).
- Renamed Windows build directory from obj to build (consistent naming with macOS).

Changes on Windows:

Modified solution filters: Updated solution filters/folders to match the layout of the repository {core, platforms, tests}.
Removed WDK 7.1 makefiles: According to Added support for Linux hosts #108 (comment), these were part of the legacy WDK 7.1 build system used in earlier HAXM releases and no longer consumed. This was replaced with WDK 10, whose Nmake2MsBuild tool generated the necessary .props files.
Removed WDK 10 makefiles: There's no need of having one extra layer of indirection in our build systems: both VS and EWDK consume only .sln/.vcxproj files which are already checked in the repository. The additions to .vcxproj's are not significantly more verbose (and redundancy could still be reduced by removing duplicated statements if necessary).
Explicitly disabled JustMyCode: Debug configurations enable JustMyCode which causes a known bug in driver projects under Visual Studio 15.8.

Changes on macOS:

Removed readme: Previously at darwin\hax_driver\com_intel_hax\readme, since its description of the codebase no longer matched the state of the codebase.

- Updated solution filters/folders to match the layout of the future layout of the repository {core. platforms, tests}. - Disabled JustMyCode from Debug configurations of driver-related projects to address an issue existing in Visual Studio 15.8: https://developercommunity.visualstudio.com/content/problem/302014/dirver-build-debugmode-checkfordebuggerjustmycode.html Signed-off-by: Alexandro Sanchez Bach <[email protected]>

- Removed makefiles related to the legacy WDK 7.1 build system used in earlier HAXM releases. This has been replaced with WDK 10, whose Nmake2MsBuild tool generated the .props files. Since these files are no longer consumed by the current build system, they have been removed. - Removed `darwin\hax_driver\com_intel_hax\readme` file, since its description of the codebase no longer matches the actual state of the codebase. Signed-off-by: Alexandro Sanchez Bach <[email protected]>

raphaelning

Thanks, this is a great PR!

We'll test building the new .sln using VS2017 and EWDK after you fix the $(OutDir) setting. The build instructions will also need to be updated due to the new directory layout. If you want to help update it, that would be great, or you can leave it to us.

raphaelning · 2018-10-24T03:05:14Z

platforms/windows/haxm-windows.vcxproj

+    <OutDir>$(SolutionDir)build\out\$(DDKSpec)\</OutDir>
+    <IntDir>$(SolutionDir)temp\$(ProjectName)\$(Platform)\$(ConfigurationName)\</IntDir>
+  </PropertyGroup>
  <!-- The WrappedTaskItems label is used by the conversion tool to identify the location where items 


nit: Remove this redundant comment.

raphaelning · 2018-10-24T03:12:03Z

platforms/windows/haxm-core.vcxproj

  </ImportGroup>
  <PropertyGroup Label="UserMacros" />
+  <PropertyGroup Label="Configuration">
+    <OutDir>$(SolutionDir)build\core\$(DDKPlatform)\</OutDir>


Use separate OutDirs for Debug and Release by appending $(ConfigurationName)\

nit: Use $(Platform) instead of $(DDKPlatform). I think they differ only in the naming of 32-bit x86: Win32 vs. x86. I'm fine with either, but I'd like to be consistent with what we use for $(IntDir).

raphaelning · 2018-10-24T05:35:24Z

platforms/windows/haxm-core.vcxproj

  <PropertyGroup Label="UserMacros" />
+  <PropertyGroup Label="Configuration">
+    <OutDir>$(SolutionDir)build\core\$(DDKPlatform)\</OutDir>
+    <IntDir>$(SolutionDir)temp\$(ProjectName)\$(Platform)\$(ConfigurationName)\</IntDir>


nit: I think this is for build intermediates. Shall we also put them in build\, perhaps build\intermediates\$(ProjectName)\$(Platform)\$(ConfigurationName)\? The Darwin build does something similar:

build/{Debug, Release}/ for final product

build/intelhaxm.build/{Debug, Release}/ for intermediates

raphaelning · 2018-10-24T06:27:58Z

platforms/windows/haxm-windows.vcxproj

  </ImportGroup>
  <PropertyGroup Label="UserMacros" />
+  <PropertyGroup Label="Configuration">
+    <OutDir>$(SolutionDir)build\out\$(DDKSpec)\</OutDir>


$(DDKSpec) seems to map to win7. As with haxm-core.vcxproj, we should use separate OutDirs for Debug/Release, x64/Win32, etc.:

$(SolutionDir)build\out\$(Platform)\$(ConfigurationName)\

We don't have to include win7 in the path.

That makes much more sense, thanks for the hint! I was trying to resemble the old paths (obj\out\Win7), but I agree that build\out\$(Platform)\$(ConfigurationName is much more convenient.

raphaelning · 2018-10-24T06:29:16Z

platforms/windows/haxm-windows.vcxproj

  <PropertyGroup Label="UserMacros" />
+  <PropertyGroup Label="Configuration">
+    <OutDir>$(SolutionDir)build\out\$(DDKSpec)\</OutDir>
+    <IntDir>$(SolutionDir)temp\$(ProjectName)\$(Platform)\$(ConfigurationName)\</IntDir>


nit: Replace temp\ with build\intermediates\ (see my comment on haxm-core.vcxproj).

raphaelning · 2018-10-24T06:46:03Z

platforms/windows/version.rc

 #define VER_LEGALCOPYRIGHT_YEARS    "2013"
-#define VER_LEGALCOPYRIGHT_STR      "Copyright© " VER_LEGALCOPYRIGHT_YEARS " Intel Corporation"
-#define VER_LEGALTRADEMARKS_STR     "Copyright© 2013 Intel Corporation"
+#define VER_LEGALCOPYRIGHT_STR      "Copyrightï¿½ " VER_LEGALCOPYRIGHT_YEARS " Intel Corporation"


These two lines were probably changed by accident. The copyright symbol (U+00A9) doesn't look right now. Please revert, or maybe use an escape sequence instead (\xa9? I'm not sure).

Thanks for doing the revert. It seems this file is not encoded correctly, and opening it in VS2017 results in an error. But I don't think that has anything to do with this PR. We'll try to fix the encoding later.

raphaelning · 2018-10-24T07:02:10Z

include/hax_types.h

 #include "darwin/hax_types_mac.h"
 // Windows
-#elif defined(__WINNT__)
+#elif defined(WINNT)


Do you know where the WINNT macro is defined? Maybe we should use the more common _WIN32 macro instead:

https://stackoverflow.com/questions/142508/how-do-i-check-os-with-a-preprocessor-directive

I found it among the "inherited values" for the project's preprocessor definitions. This likely means that some internal Visual Studio .props file defines it. I just chose WINNT due to the similarity to __WINNT__ (which was by the way manually defined in sources.props).

I agree we should use the more common _WIN32 macro, so I will change it.

raphaelning · 2018-10-24T07:04:23Z

platforms/windows/.gitignore

@@ -0,0 +1,11 @@
+# Objects
+build/
+temp/


nit: Remove this if you change the $(IntDir) prefix to build\intermediates\.

raphaelning · 2018-10-24T07:08:37Z

platforms/windows/haxm-core.vcxproj

  <PropertyGroup Label="Globals">
    <ProjectGuid>{BC80D1E0-5738-4048-A742-8A20949A6587}</ProjectGuid>
    <RootNamespace>$(MSBuildProjectName)</RootNamespace>
+    <WindowsTargetPlatformVersion>$(LatestTargetPlatformVersion)</WindowsTargetPlatformVersion>


nit: Is this autogenerated by Visual Studio? I'm not sure if it implies the use of Windows 10 SDK and breaks support for Windows 7 and 8.x (maybe not, since we have <TargetVersion>Windows7</TargetVersion> below). I'd remove it.

Is this autogenerated by Visual Studio?

Yes. I'm not entirely sure about the implications of that line.

raphaelning · 2018-10-24T07:09:46Z

platforms/windows/haxm-windows.vcxproj

  <PropertyGroup Label="Globals">
    <ProjectGuid>{BA777056-A57E-492D-A821-68D08A2DACE6}</ProjectGuid>
    <RootNamespace>$(MSBuildProjectName)</RootNamespace>
+    <WindowsTargetPlatformVersion>$(LatestTargetPlatformVersion)</WindowsTargetPlatformVersion>


nit: Is this autogenerated by Visual Studio? I'm not sure if it implies the use of Windows 10 SDK and breaks support for Windows 7 and 8.x (maybe not, since we have Windows7 below). I'd remove it.

Is this autogenerated by Visual Studio?

Yes. I'm not entirely sure about the implications of that line.

- Moved `{darwin,windows}/` to `platforms/{darwin,windows}/`. This is meant to reduce root-folder pollution, specially if support for more platforms (e.g. Linux) is added. - Removed WDK 10 makefiles: There is no need of having one extra layer of indirection in our build systems: both VS and EWDK consume only .sln/.vcxproj files which are already checked in the repository. The additions to .vcxproj's are not significantly more verbose. - Reorganized .gitignore files: One root-level .gitignore file with general rules that affect the entire source tree (e.g.: .DS_Store for macOS), and platform/compiler-specific rules at platforms/*/.gitignore to deal with build-related files (e.g.: *.vcxproj.user for Visual Studio, build/ for Xcode). - Renamed `darwin/hax_driver/com_intel_hax` to `darwin` (to maintain consistent folder depth). - Renamed Windows build directory from `obj` to `build` (to maintain consistent naming with macOS). Signed-off-by: Alexandro Sanchez Bach <[email protected]>

Moved API.md to docs/api.md, and added platform-specific usage instructions in docs/manual-{darwin,windows}.md to avoid polluting README.md, specially if support for more platforms (e.g. Linux) is added. Signed-off-by: Alexandro Sanchez Bach <[email protected]>

AlexAltea · 2018-10-24T13:22:10Z

All done!

I've also taken the opportunity to add extra changes:

Modified haxm-tests.vcxproj to use the build/intermediates/ folder for intermediates and build/tests/ for the resulting executable.
Removed MASM-related settings since MASM is no longer used.
Updated the documentation for macOS and Windows.

raphaelning · 2018-10-25T05:03:05Z

Thanks, the new source tree layout looks so neat and clean!

nit: In platforms/windows/, there are 3 .c files where some of the #include paths are not "canonical", although that doesn't break the build:

hax_win.h
hax_host_mem.c
hax_wrapper.c

We have started testing this PR, and have found one issue so far: the tests project fails the Win32 build (both Debug and Release).

  test_emulator.obj : error LNK2019: unresolved external symbol _em_decode_insn referenced in function "protected: void __thiscall EmulatorTest::assemble_decode(char c
onst *,unsigned __int64,unsigned int *,unsigned int *,enum em_status_t *)" (?assemble_decode@EmulatorTest@@IAEXPBD_KPAI2PAW4em_status_t@@@Z) [\path\to\haxm\platforms\wi
ndows\haxm-tests.vcxproj]
  test_emulator.obj : error LNK2019: unresolved external symbol _em_emulate_insn referenced in function "protected: void __thiscall EmulatorTest::run(char const *,stru
ct test_cpu_t const &,struct test_cpu_t const &)" (?run@EmulatorTest@@IAEXPBDABUtest_cpu_t@@1@Z) [\path\to\haxm\platforms\windows\haxm-tests.vcxproj]
  \path\to\haxm\platforms\windows\build\tests\Win32\\haxm-tests.exe : fatal error LNK1120: 2 unresolved externals [\path\to\haxm\platforms\windows\haxm-tests.vcxproj]

Signed-off-by: Alexandro Sanchez Bach <[email protected]>

AlexAltea · 2018-10-25T08:21:35Z

In platforms/windows/, there are 3 .c files where some of the #include paths are not "canonical", although that doesn't break the build:

By canonical you mean they are not relative to the current file? If so, I've just fixed it. The reason it worked is that the project (and the source.props file before), has several added include directories.

We have started testing this PR, and have found one issue so far: the tests project fails the Win32 build (both Debug and Release).

This was seemingly caused my the last PR, since the emulator tests include emulate.h, which relies on HAX_ARCH* macros, which are included from include/hax_types.h if HAX_TESTS is not defined, which it is.

Since hax_types.h is meant to be arch/compiler/platform-agnostic, I feel the correct choice should be making it available both in tests and kernel code. The only obstacle that prevents it are platform-specific headers (e.g. windows/hax_types_windows.h) that include headers not available for userspace projects. Consequently I've filtered those out with the HAX_TESTS macro.

In the future we probably should make a clear cut between kernel and userspace: maybe by defining macros HAX_KERNEL and HAX_USER (instead of HAX_TESTS) that lets us separate code better.

raphaelning

Thanks!

raphaelning · 2018-10-25T09:10:46Z

By canonical you mean they are not relative to the current file? If so, I've just fixed it. The reason it worked is that the project (and the source.props file before), has several added include directories.

Right. I noticed that you had updated most relative #include paths, so I just thought you had missed a few.

This was seemingly caused my the last PR, since the emulator tests include emulate.h, which relies on HAX_ARCH* macros, which are included from include/hax_types.h if HAX_TESTS is not defined, which it is.

I see. I thought we had tested the Win32 build before merging #110, but perhaps we overlooked the tests build failure.

In the future we probably should make a clear cut between kernel and userspace: maybe by defining macros HAX_KERNEL and HAX_USER (instead of HAX_TESTS) that lets us separate code better.

I agree. Maybe this can help us solve another problem (#9): that hax_interface.h is actually not shared between HAXM (kernel) and QEMU (user space). There are a few issues to fix, but making hax_types.h portable would be a key step.

AlexAltea · 2018-10-25T09:11:56Z

Maybe this can help us solve another problem (#9): that hax_interface.h is actually not shared between HAXM (kernel) and QEMU (user space).

Yes, my thoughts exactly. :-)

AlexAltea added 2 commits October 23, 2018 12:55

AlexAltea force-pushed the reorganization branch 2 times, most recently from 784efbf to 86ebde2 Compare October 24, 2018 02:53

raphaelning suggested changes Oct 24, 2018

View reviewed changes

AlexAltea added 2 commits October 24, 2018 15:06

Combined documentation

89857de

Moved API.md to docs/api.md, and added platform-specific usage instructions in docs/manual-{darwin,windows}.md to avoid polluting README.md, specially if support for more platforms (e.g. Linux) is added. Signed-off-by: Alexandro Sanchez Bach <[email protected]>

AlexAltea force-pushed the reorganization branch 2 times, most recently from 35bf3d3 to 89857de Compare October 24, 2018 13:19

AlexAltea mentioned this pull request Oct 24, 2018

Added TravisCI scripts #115

Merged

Fixed tests and include paths in Windows sources

3e21ff4

Signed-off-by: Alexandro Sanchez Bach <[email protected]>

raphaelning approved these changes Oct 25, 2018

View reviewed changes

wcwang merged commit 43b3e4f into intel:master Oct 25, 2018

AlexAltea deleted the reorganization branch October 25, 2018 09:20

Restructuring codebase #113

Restructuring codebase #113

Uh oh!

Conversation

AlexAltea commented Oct 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Uh oh!

raphaelning left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexAltea Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexAltea commented Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

raphaelning commented Oct 25, 2018

Uh oh!

AlexAltea commented Oct 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

raphaelning left a comment

Choose a reason for hiding this comment

Uh oh!

raphaelning commented Oct 25, 2018

Uh oh!

AlexAltea commented Oct 25, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

AlexAltea commented Oct 23, 2018 •

edited

Loading

AlexAltea Oct 24, 2018 •

edited

Loading

AlexAltea commented Oct 24, 2018 •

edited

Loading

AlexAltea commented Oct 25, 2018 •

edited

Loading