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

Added cmake script for cross-platform build + solved build errors when building with VS #467

Closed
wants to merge 1 commit into from

Conversation

KOLANICH
Copy link

Added some shims for Visual Studio
Since this commit you won't need to download opencl headers manually - they are git submodule
Made compatibility with VS: fixed build and link errors

Refactored the source during the work:

  • moved sources to multiple files to increase readability and allow VS compiler not to crash
  • replaced macrodefinitions with inline functions and enums to prevent potential errors
  • fixed some warnings
  • replaced uints with enums types and ladders of elseifs with switches
  • moved some constants into config generated with cmake
  • fixed the source to make it also buildable with C++ compiler
  • fixed BUILD.md

Fixed incorrect calling convention for AMD Display Library to _cdecl from _stdcall

@KOLANICH KOLANICH force-pushed the fix-windows-build branch 15 times, most recently from 1f3232e to 85d5262 Compare August 21, 2016 17:01
Added some shims for Visual Studio
Since this commit you won't need to download opencl headers manually - they are git submodule
Made compatibility with VS: fixed build and link errors

Refactored the source during the work:
* moved sources to multiple files to increase readability and allow VS compiler not to crash
* replaced macrodefinitions with inline functions and enums to prevent potential errors
* fixed some warnings
* replaced uints with enums types and ladders of elseifs with switches
* moved some constants into config generated with cmake
* fixed the source to make it also buildable with C++ compiler
* fixed BUILD.md

Fixed incorrect calling convention for AMD Display Library to _cdecl from _stdcall
@jsteube
Copy link
Member

jsteube commented Aug 21, 2016

There's a lot of potential in this, I like it. From a quick overview i found the following stuff very nice:

  • pragma once
  • config.h
  • have many small files

Before thinking about merging, I have to check this out in detail and you need to fix some stuff first:

  • Sync your branch with hashcat master, I got tons with conflicts. We're are hashcat v3.10 already, while you're at v3.00
  • Is it possible to make this without using cmake?
  • Does not compile on linux anymore:

root@ht:~/xy/hashcat/build# make
Scanning dependencies of target hashcat
[ 3%] Building C object CMakeFiles/hashcat.dir/src/status_display.c.o
In file included from /root/xy/hashcat/include/hc_device_param_t.h:5:0,
from /root/xy/hashcat/include/shared.h:31,
from /root/xy/hashcat/src/status_display.c:3:
/root/xy/hashcat/include/ext_OpenCL.h:76:3: error: unknown type name ‘OCL_LIB’

  • What is a windows shim?
  • How can I cross-compile the windows binaries from Linux. Users expect that from us
  • What advantage is it to have those enum types over macro definitions?
  • Why did you add type naming after: typedef struct XXXX_t_ { ... } XXXX_t; ? Anonymous structs are fine aren't them?
  • The filenames have no real convention, for example: ext_OpenCL.h <- original, filenames_generators <- new type 1, hc_concurrency.h <- new type 2. So the question is, why having hc_* in front for some and for others not?

@KOLANICH
Copy link
Author

KOLANICH commented Aug 21, 2016

Sync your branch with hashcat master, I got tons with conflicts. We're are hashcat v3.10 already, while you're at v3.00

You can see I have already done that, the version was rebased upon master and conflicts are solved semi-manually. As you see, the version number now is parsed from the nearest tag from git and the version number v3.00 in CMakeLists.txt is just a fallback.

Merge conflicts are not solved automatically (in theory they could be solved completely automatically, but it would require some advanced tools merging ASTs and keeping history of coder's actions in IDE) because a lot of code is changed and put into another files. To merge you need to solve conflict by simply picking my versions.

Is it possible to make this without using cmake?

Both yes and no. Yes because you can pregenerate files with cmake. No because cmake gens system-specific files without flexibility in them. But don't worry, cmake is already de-facto standard way to build cross-platform software, lots of well-known projects use it: opencv, libnfc, websocketpp for example.
Of course you can use another build system, but I don't think the users will be happy to download install python or cygwin just to build hashcat. Cmake is both lighthweight and powerful solution for this.

What is a windows shim?

it is *nix shim for windows. It is some headers missing in MSVC I found over internet and put together into a git repo for convenience.

How can I cross-compile the windows binaries from Linux. Users expect that from us

cmake allows you to provide the toolchain you want to use, including cross-compilers.

What advantage is it to have those enum types over macro definitions?

Macros is a very dirty way to do things, they transform the source code in unobivious way, which can cause very hard-to-find errors. You see the code, but in fact it is another code. Please, don't use them except the case of extreme need. I have replaced macros with enums and inlines to be sure that the compilation errors I was getting were not caused by them.

Why did you add type naming after: typedef struct XXXX_t_ { ... } XXXX_t; ? Anonymous structs are fine aren't them?

They are fine, but Visual Studio writes the struct type in tooltips, not the typedef one, which looks not fine if the structs are anonymous.

The filenames have no real convention, for example: ext_OpenCL.h <- original, filenames_generators <- new type 1, hc_concurrency.h <- new type 2. So the question is, why having hc_* in front for some and for others not?

Because some symbols in them have hc_ prefix :) In fact developing naming convention is not my target for now, the primary target is to make it be built without errors with msvc, gcc, g++ and clang.

@jsteube
Copy link
Member

jsteube commented Aug 21, 2016

I've decide to reject this, even there's tons of good stuff in here.

Major reason is that, for me, it's too much changes at once to recognize all the implication.
Minor reason is that it's not conflict free PR. A good PR's should be free of conflict.

Anyway I want to use some of those changes. So what I'm asking here is to do the changes, but slowly. I'd suggest to create dedicated PR's where you only change one thing at a time. That would help me to understand the changes in detail and in depth.

I know it's not always possible, especially when it comes to includes and depencies, but we could start with the easy changes first, for example moving the MACRO constants into the ENUM types. Since you made all the changes, you can evaluate the steps much better than I can. Ideally create a migration plan first.

About cmake: If possible I'd like to not use cmake, because I'm not used to it. If there's really no way around it, then we do it. But in that case we should do it at the end of the migration process.

About .gitmodules: The idea seems nice, however there also some problem with it. If there's any code included automatically from a different repository, than this repository could be used to inject code without any changes to hashcat. There created some concern from the more security aware developers and I can see their point. We should avoid this.

@jsteube jsteube closed this Aug 21, 2016
@KOLANICH
Copy link
Author

The idea seems nice, however there also some problem with it. If there's any code included automatically from a different repository, than this repository could be used to inject code without any changes to hashcat

Yes, it can be used, if one manages to find a collision for sha1 ;).

@roycewilliams
Copy link
Member

This is not as far-fetched as it might appear. https://sites.google.com/site/itstheshappening/

jsteube added a commit that referenced this pull request Sep 2, 2016
jsteube added a commit that referenced this pull request Sep 2, 2016
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.

None yet

3 participants