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

Detect FMT_SPLIT_UNIFIES_CASE error #1736

Merged

Conversation

loverszhaokai
Copy link
Contributor

No description provided.

magnumripper added a commit that referenced this pull request Sep 8, 2015
@magnumripper magnumripper merged commit 349e5c3 into openwall:bleeding-jumbo Sep 8, 2015
@loverszhaokai
Copy link
Contributor Author

@jfoug

Thanks for your patches for detecting FMT_SPLIT error. Should I change the "PASS" to "FAILED" when
there is FMT_SPLIT warning ?

@magnumripper
Copy link
Member

If this had been the normal self-test it should definitely be just a non-fatal warning. But maybe nothing in -test-full should be mere warnings? I'm not sure.

@magnumripper
Copy link
Member

We're not counting/summarizing warnings. That is an argument for actually give FAILED for any problem in -test-full. An alternative would be to start counting and summarizing warnings but that may be overkill.

@loverszhaokai
Copy link
Contributor Author

@magnumripper Thanks. I think I should change it to "FAILED".

@loverszhaokai
Copy link
Contributor Author

@jfoug @magnumripper

When there is a FMT_SPLIT warning or error, change to "FAILED" by: #1741

@jfoug
Copy link
Collaborator

jfoug commented Sep 10, 2015

Good, I agree that in -test=full, it should not be a warning, but an error. In that mode, the user is certainly doing an expensive test. I really think however that -test-full probably should be removed from the normal options, and placed into the 'hidden' set.

@magnumripper
Copy link
Member

I think you are now failing but still printing "Warning" for the two latter cases? I didn't try it though.

@loverszhaokai
Copy link
Contributor Author

@magnumripper

I think you are now failing but still printing "Warning" for the two latter cases? I didn't try it though.

Yes.

[...]
Testing: dynamic=md5($p) [256/256 AVX2 8x3]... FAILED (Warning: dynamic=md5($p) should not set FMT_SPLIT_UNIFIES_CASE)
[...]
1 out of 335 tests have FAILED

@magnumripper
Copy link
Member

I really think however that -test-full probably should be removed from the normal options, and placed into the 'hidden' set.

I also think we should put most of this in #if !JTR_RELEASE_BUILD or someting like that. Maybe Solar disagrees, and for that matter this is not a lot of code.

@magnumripper
Copy link
Member

I think you are now failing but still printing "Warning" for the two latter cases? I didn't try it though.

Yes.

And actually I think for the first case, FAILED will end up printed twice, won't it? I think you need to fix all three. Just give a description of the problem and let the caller add FAILED to it.

@loverszhaokai
Copy link
Contributor Author

@magnumripper

And actually I think for the first case, FAILED will end up printed twice, won't it? I think you need to fix all three. Just give a description of the problem and let the caller add FAILED to it.

OK.

@magnumripper
Copy link
Member

Now that you've got my attention, it's like this:

Benchmarking: dynamic=md5($p) [128/128 AVX 4x3]... FAILED (dynamic=md5($p) should not set FMT_SPLIT_UNIFIES_CASE)

I think the repetition of the format name is redundant and should be dropped. This probably affects most of your fails.

@magnumripper
Copy link
Member

I think it should be

Benchmarking: dynamic=md5($p) [128/128 AVX 4x3]... FAILED (should not set FMT_SPLIT_UNIFIES_CASE)

@loverszhaokai
Copy link
Contributor Author

@magnumripper

I will change it soon. : )

@jfoug
Copy link
Collaborator

jfoug commented Sep 10, 2015

It's kind of funny. These have been problems for a LONG time, and no one has complained a bit. Toss out a warning, and they get fixed in hours.

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.

3 participants