-
Notifications
You must be signed in to change notification settings - Fork 2k
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
bleeding-jumbo fails to build on Fedora Rawhide (F23) #1093
Comments
Is this about using gcc v5 or something like that, or did it work before some recent commit? Latest code works fine here using gcc 4.9.2 |
Something is up when I use GCC 5 to compile JtR. I haven't done any further investigations to see what exactly causes the problem. |
MD5_thread_for_body is in MD5_std.c It gets 'used' by dynamic under certain conditions (which used to be when it was compiled for md5crypt). I will check to see if these have changed somehow. Using those (MD5_body and MD5_thread_for_body) can be faster than oSSL, that is why they are used. |
can you do a make clean then make, just to be sure. |
This might be the 'MAYBE_INLINE' working differently. @magnumripper how would we easily disable that? How about adding these lines to the very bottom of common.h (NOTE, this is only to 'test' that this is what is happening). #undef MAYBE_INLINE Put that at the bottom of common.h make clean and make and see if link problem goes away. |
I see nothing in the release notes about changed syntax for inline. |
It would have NOTHING to do with syntax. It would have everything to do with implementation. This is a link error, meaning that the compiler is not emitting that function to the object code. That is all. To me it sounds like implementation differences. I am not able to test, just guess. @kholia will have to tell whether that guess is right or not. Once we know, then we can see what is needed to 'fix' it. |
@jfoug I started with a clean tree. After the build failed, I ran "make clean" again, just to be sure. |
Please try to remove the MAYBE_INLINE and see if problem goes away. I really think this is what is happening. That now, with 5.0 nothing gets output into the object file, thus the link fails, where before, even if it was being done inline, it still ended up in the .o file, so that dyna could use it. If this is the case, then we have 1 of 3 choices. 1, remove the inline. 2. someone compile the function twice, once with inline, and once without. 3. remove its usage from dyna, and fall back to oSSL (for gcc 5). I would really like to find out just how big a deal the 'inline' really is within cryptmd5. It really WAS nice to be able to share that code, but often times the code in JtR is poorly written to allow code share (static functions, inlines, etc), all for often just an insignificant gain. |
I would regard that as a compiler bug. As long as the function is not static, a non-inlined copy should be emitted too. |
Seems you are right, and it's about C11 vs C89: http://www.gnu.org/software/gcc/gcc-5/porting_to.html |
So one obvious solution is adding |
Also:
So we 'may' be able to use extern, still get the inline for md5crypt, but force the compiler to jam it into the .o file also. |
But since I do not have this version, @kholia may have to do the experimentation on getting this working. I can propose things, but it will be his testing that is required. |
I made these changes: $ git diff
diff --git a/src/MD5_std.c b/src/MD5_std.c
index 40bf79e..3c1f4ca 100644
--- a/src/MD5_std.c
+++ b/src/MD5_std.c
@@ -480,7 +480,8 @@ extern void MD5_body(MD5_word x[15], MD5_word out[4]);
* is large enough.
*/
#ifdef __x86_64__
-#define MAYBE_INLINE_BODY MAYBE_INLINE
+//#define MAYBE_INLINE_BODY MAYBE_INLINE
+#define MAYBE_INLINE_BODY
#else
#define MAYBE_INLINE_BODY
#endif Here were timings with inline
Here were timings without
In other words, a bunch of problems for nothing. There are WAY too many of these, they end up meaning nothing but a huge JtR image. No gain in speed, and makes things more difficult for code sharing. Inlines ARE very important, but they have to be the right ones. Inlining a monster like body gains little to nothing, and may at times lose performance, due to a larger working set, and caches being blown. |
Please try the patch in the message I sent just prior to this (that clears out the MAYBE_INLINE_BODY), and see if that change alone is enough to make this work. IF that is the case, and from what I have seen, there is almost NO gain trying to inline the body, then I would propose to either remove the inlining, OR remove the inlining of body if GCC 5 is detected (or C99 semantics are being used). |
The patch fixes the build problem 👍 |
Ok, if we now KNOW the problem, lets look at a solution. Again, the solution 'I' would prefer, is to scrap the MAYBE_INLINE_BODY fully. IT DOES NOT provide any benefits of speed (possibly less than 1%, but it seems to be within the margin of fluctuation). But there are other options. With the inline, it appears that gcc5 is not happy to provide the extern function any more for us. |
@jfoug I will be online for next 2 hours or so (before I hit the bed). I can try alternate patches too :) |
This is core stuff so we could opt to just hand it over to Solar. He will have to fix it in core asap and I'd prefer to just go with whatever fix he picks. @kholia could you try checking out the master branch and build it without any fix? Do you get similar errors? |
@jfoug your tests were done with AVX, I'm not sure this inline macro is involved at all then? I think it's not. |
@magnumripper "make clean linux-x86-64" on |
OK, then we can leave Solar out of this discussion. |
@magnumripper XOP also had zero gain (well, about 1% also). |
Also SSSE4.1 had about a 1% loss (VM). Inlining of those GIANT functions is not going to gain speed, or just a super tiny amount. The total amount of time creating and tearing down the stack frame is simply meaningless, compared with amount of time used in the function. there is WAY too much inline requests in JtR where they should not be. This is just one example. Yes, there ARE times where it is hugely helpful, and other times where it is used to make fake functions to put into a header. But simply jamming an |
My point was that this MAYBE_INLINE_BODY is not used at all for SIMD. You are just seeing random variations from unaltered code. |
Here's a relevant test that is actually affected by that macro (this is CORE john without SIMD)
This is still less than one percent so your reasoning holds true anyway. This is on core i7 mobile. |
My bad. You are 100% right. |
So when is this code used in Jumbo on an x86-64? It's only in a few Dynamic formats that need large input, right? |
I'm still kind of fond of the idea to just add |
BTW I can't build gcc-5 on OSX yet. Well I probably could, but I'm too lazy... I'll just wait till my packager has it nicely wrapped up. |
For SIMD builds, yes, that is correct. But it IS used, and this is required at link time. |
using gcc 5 without --std=gnu89 (although I kept the latter for now). See also #1250.
I mentioned that the source provided at openwall.com/john/ for john-1.8.0-jumbo include this issue. |
I just hit this too (on NixOS 16.03). Please make a release with the fix. |
We are in no position to release right now. It's likely several months away. |
Fixes this build error: dynamic_fmt.o: In function `DynamicFunc__crypt_md5_to_input_raw_Overwrite_NoLen': .../john-1.8.0-jumbo-1/src/dynamic_fmt.c:4989: undefined reference to `MD5_body_for_thread' Upstream issue: openwall/john#1093
Fixes this build error: dynamic_fmt.o: In function `DynamicFunc__crypt_md5_to_input_raw_Overwrite_NoLen': .../john-1.8.0-jumbo-1/src/dynamic_fmt.c:4989: undefined reference to `MD5_body_for_thread' Upstream issue: openwall/john#1093 (cherry picked from commit d565687)
hello earth to planet ripper the year down here is 2017 staring 2018 in the face Santa with his sleigh is the only thing in the way :p where is that patch , i see the issue is closed , make[1]: Leaving directory '/home/mpiuser/john-1.8.0-jumbo-1/src/escrypt' warning "_BSD_SOURCE and _SVID_SOURCE are deprecated, use _DEFAULT_SOURCE" ... maybe i'll take it up with him, Santa this one here compiled |
@marxenegls, have you tried to apply the patch seen in #2743?
Why don't you use the one that works |
This one is still working on Xubuntu 18.04. Thank you! |
Apparently commit e2e868d fixed the remaining issues with gcc 5.x. See also openwall#1093, openwall@e2e868d, and openwall@f310326.
Apparently commit e2e868d fixed the remaining issues with gcc 5.x. See also openwall#1093, openwall@e2e868d, and openwall@f310326.
Apparently commit e2e868d fixed the remaining issues with gcc 5.x. See also openwall#1093, openwall@e2e868d, and openwall@f310326.
Fixes this build error: dynamic_fmt.o: In function `DynamicFunc__crypt_md5_to_input_raw_Overwrite_NoLen': .../john-1.8.0-jumbo-1/src/dynamic_fmt.c:4989: undefined reference to `MD5_body_for_thread' Upstream issue: openwall/john#1093 (cherry picked from commit d565687)
The text was updated successfully, but these errors were encountered: