Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update MSVC projects #67

Merged
merged 3 commits into from
Nov 7, 2018
Merged

Update MSVC projects #67

merged 3 commits into from
Nov 7, 2018

Conversation

Mattiwatti
Copy link
Contributor

Hello hello, it's the MSVC guy again 😃

This PR updates the files under /msvc to make them up to date with the current master branch. I understand master is a moving target now that v3.0 is in development, so don't let this hold you back from breaking it all again in the next commit. But since the addition of the Zycore submodule is going to be the biggest change by far (project structure wise), I wanted to get that out of the way.

Refer to the first commit message for a summary of changes that should hopefully be noncontroversial. The other two commits fall under 'miscellaneous` so I separated them.

Some points to consider:

  • The Zycore project is under /msvc/dependencies/zycore, which makes a lot of the paths in the project file ../../../look/like/this. I decided that this was worth it because it preserves the property of /msvc that its layout mirrors the directory layout of the top level directory, which I would like to keep that way if possible.
  • ZycoreExportConfig.h is located under /msvc (so next to ZydisExportConfig.h). I don't really have a preference for where to put this file and I can move it to /msvc/dependencies or /msvc/dependencies/zycore if needed. The current path was mainly chosen for two reasons:
    • It avoids having to add another include path to each project, because all projects already include $(SolutionDir).
    • Hiding the file in a deep path might make it easy to overlook, despite being a pretty important header for the entire project.
  • ZYCORE_NO_LIBC and ZYAN_NO_LIBC are both defined where applicable even though they mean the same thing. This should be changed to one of the two, pending Zycore #1.
  • The Debug DLL and Release DLL configurations currently produce both Zycore.dll and Zydis.dll (same behaviour as CMake if building shared libraries is enabled). Zydis does not link against or import from Zycore, but the tools and examples do potentially import from Zycore.dll (currently only ZydisInfo and ZydisPerfTest actually require it). In my opinion Zycore is small enough that producing a separate DLL for it is questionable. It may be worth considering making Zycore a static library even for 'DLL' configurations, so that executables continue to only depend on Zydis.dll but are free to use code from Zycore if needed.

- Added Zycore project, on which all tools and examples depend just as they do on Zydis. Zydis itself and the ZydisWinKernel project do not import or link against Zycore; they only include its headers.
- Added msvc/ZycoreExportConfig.h (equivalent to the same file generated by CMake for a standard build configuration).
- Removed FormatterHooks project as this example no longer exists. Projects for the replacement samples (currently Formatter01-03.c) will be added when they are more mature and have proper names :)
- Slightly modified the include path order: Zycore headers are now always included first, followed by Zydis headers, and then headers in any other paths. This is mostly for clarity to indicate the dependency order. There should be no functional change.
- Moved ZydisFuzzIn project from /msvc/examples to /msvc/tools to mirror the same change in the top level directory.
…ON to 0x0501 and 0x0502 for 32 and 64 bits targets respectively.

This ensures that functions incompatible with Windows XP cannot be accidentally used because they will be #ifdef'd out in the Windows headers. The exception is ZydisWinKernel, which requires Windows 7 mostly due to WDK toolchain limitations.

Note that the projects still use the v141 toolset and not v141_xp as might be expected. The latter toolset is a giant hack that requires additional XP targeting support to be installed with Visual Studio, and changes the platform SDK to the ancient Windows 7 SDK as well as adding some other undesirable build hacks. In practice however the current toolchain outputs binaries that work perfectly fine on platforms from XP through 10, so long as the above preprocessor defines are correctly set to the minimum desired target version
- Disable the 'support just my code' anti-feature (removes ability to step through system code)
- Generate non-crippled PDBs by specifying /DEBUG:FULL, which was previously the default behaviour
- Add undocumented linker switch /NOVCFEATURE to remove yet another pointless debug directory entry. This will reduce the size of output binaries slightly
@flobernd
Copy link
Member

flobernd commented Nov 7, 2018

Thank you very much for your PR :) We will merge it very soon.

  1. I'm okay with the long relative pathes.
  2. Placing these two configuration headers together sounds like a good idea for me. Wouldn't like to have it hidden in a deep path either.
  3. Oh, seems like I have to look at that again. Already cleaned up that mess a bit some time ago, but I missed the LIBC thing I guess.
  4. The CMake file used to have a global option to define static/shared for all modules (and sub-modules). I did not like this solution for the same reasons you mentioned above and replaced it by a seperate ZYXXX_SHARED_LIBRARY option for each independent module (dll). This way you can choose to build Zycore as a static library and Zydis as a shared one, or the other way around, or both static, or both shared, and so on ... IMHO both of the major options (Zycore shared + Zydis shared as well as Zycore static + Zydis shared) are offering different advantages and disadvantages. Not really sure about this point at the moment, have to rethink this.

@flobernd flobernd merged commit 7c0f4e3 into zyantific:master Nov 7, 2018
@Mattiwatti
Copy link
Contributor Author

Re: 3: All actual code checks for ZYAN_NO_LIBC. CMakeLists.txt also uses option(ZYAN_NO_LIBC, ...). The only mismatch is here:

if (ZYCORE_NO_LIBC)
    target_compile_definitions("Zycore" PUBLIC "ZYCORE_NO_LIBC")
endif ()

where ZYCORE_NO_LIBC is used twice instead.

@flobernd
Copy link
Member

flobernd commented Nov 7, 2018

Haha great ... so apparently the NO_LIBC option never worked after my refactorings 😄 Thanks for reporting this. I'll fix that asap.

flobernd added a commit to zyantific/zycore-c that referenced this pull request Nov 7, 2018
@Mattiwatti
Copy link
Contributor Author

OK, cool, that means the ZYCORE_NO_LIBC defines can be removed.

Do you know if a new commit can be merged on Github if I push changes to my branch after my PR has already been merged? If so, I'll push the change to my msvc branch ASAP. Otherwise I'll make a new PR (or you can make the change if you want: just delete all occurrences of ZYCORE_NO_LIBC in all .vcxproj files).

@flobernd
Copy link
Member

flobernd commented Nov 7, 2018

Do you know if a new commit can be merged on Github if I push changes to my branch after my PR has already been merged?

Good question 😄 Not 100% sure, but you should be able to create a new PR on the existing branch, after you pushed something new.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants