Closed SamuraiOcto closed 5 months ago
Thank you for your contribution! Your PR has passed all tests, including 32-bit, ARM, ASAN, no-OMP, ...
We will give feedback soon.
Thank you for the contribution @SamuraiOcto!
The added sm3*
files lack copyright and license statements. Can you please add those, preferably using our common 0-clause BSD license:
* Redistribution and use in source and binary forms, with or without
* modification, are permitted.
*
* There's ABSOLUTELY NO WARRANTY, express or implied.
although where you use third-party SM3 code, you need to use their original license (or maybe public domain statement).
Also, at least as seen in GitHub, these files appear to use 4-space indentation? We normally use tabs, and assume tab width 8.
Finally, are the current settings for MAX_KEYS_PER_CRYPT
and OMP_SCALE
arbitrary (e.g., inherited from another file we had) or did you re-tune (--tune=auto -v=5
)? There's a likely stale comment about tuning on "Core i7" (which always looked too vague to me, but another contributor put this comment in, and it propagated between files) - please either update or drop it.
When you revise this PR, please amend the one commit and force-push. Please do not add more commits.
Oh, please also add an entry to doc/NEWS
, near the end of the section for post-1.9.0 changes, so currently on line 329, and maintain a two empty line gap before the next section.
indent
/astyle
commands from your CONTRIBUTING.md
.OMP_SCALE
was indeed copied from another file. I re-tuned with --tune=auto -v=5
which gives 4 for me, but won't the outcome depend on the number of CPU cores?MAX_KEYS_PER_CRYPT
, it was indeed copied from another file.Thanks, this looks almost ready to merge.
The format plugin file has the 0-clause BSD license.
Great. You also need to add your copyright statement to there, if you wrote that file.
OMP_SCALE
was indeed copied from another file. I re-tuned with--tune=auto -v=5
which gives 4 for me, but won't the outcome depend on the number of CPU cores?
It is per-thread, so we multiply it by the actual number of threads.
Honestly I'm not sure about
MAX_KEYS_PER_CRYPT
, it was indeed copied from another file.
Normally, it's to be tuned manually at 1 thread or in a non-OpenMP build, then once you have it at optimal value for that, you tune OMP_SCALE
for multiple threads. That said, the value of 32 looks sane for a fast hash.
I think I'll just merge this and then add a copyright line using your GitHub username on it...
OK, merged and I'm testing and preparing further changes. Added copyright statement:
+++ b/src/sm3_fmt_plug.c
@@ -1,9 +1,12 @@
-/* SM3 format plugin
+/*
+ * SM3 format plugin
*
- * Redistribution and use in source and binary forms, with or without
- * modification, are permitted.
+ * Copyright (c) 2024 SamuraiOcto
*
- * There's ABSOLUTELY NO WARRANTY, express or implied.
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted.
+ *
+ * There's ABSOLUTELY NO WARRANTY, express or implied.
*/
#if FMT_EXTERNS_H
Somehow performance with OpenMP is awful - way lower than at 1 thread. Running with -tune=auto
gets me a many orders of magnitude higher OMP_SCALE
than the 4 put in the code, and then performance becomes sane. But I'll look into tuning MAX_KEYS_PER_CRYPT
first.
Also confirmed that the auto-generated dynamic_big_crypt_generated.c
matches what I'm getting (sans the timestamp).
Reviewing the code, I see it somehow limits hash comparisons to 16 bytes - why is that? Perhaps copied from another format, and is a bug here?
OK, I've made several commits on top of yours @SamuraiOcto and pushed them into bleeding-jumbo.
You're adding several dynamic preload formats. Can you please document those in doc/DYNAMIC
?
You're adding several dynamic preload formats. Can you please document those in
doc/DYNAMIC
?
@SamuraiOcto Reminding you that this is still missing the above documentation, which I'd appreciate you adding via a separate PR. Thanks!
Thanks for merging this and sorry for the delay. #5512 adds the missing documentation
This adds the SM3 hash function that is fairly widespread, mainly in China, but which was not yet present in john. For now this is just a basic but slow-ish implementation. Perhaps I might add avx2 later. Let me know if things should be changed.