Provide a class to generate a SHA1 from a sequence of bytes, and
a convenience raw_ostream adaptor.
This is not used anywhere yet in LLVM, but I plan to base some
ThinLTO incremental scheme on this.
Details
- Reviewers
tejohnson • rafael mehdi_amini
Diff Detail
Event Timeline
I have some minor comments.
I think having a patch using this before it lands would be nice. :-)
Thank you for working on this.
include/llvm/Support/SHA1.h | ||
---|---|---|
34 | I think usually (confirmed in OpenSSL and CommonCrypto) these functions are called Init(), Update(), and Final(). | |
38 | We probably want to tell people to not reuse SHA1 objects, and might want to assert in debug mode. With this, you could even re-use the internal state for the result, but that might not be worth it :-) | |
lib/Support/SHA1.cpp | ||
29 | Nit: What does "override" mean in this context? Maybe "default" or "we assume little endian" or something else? | |
30 | In the compilers we support, do we need to resort to this? I don't know about MSVC, but IIRC, gcc and clang always define {BIG,LITTLE}_ENDIAN, no? We might not need so many tests. | |
45 | Nit: Why do the round constants get names, but the seeds don't? | |
120 | Nit: while(len--) //? | |
143 | Why here? | |
150 | Nit: I'd prefer a positive test (ifdef, like the one in line 102) |
Thanks for the review.
I don't mind waiting for the use case to be ready before landing this.
include/llvm/Support/SHA1.h | ||
---|---|---|
34 | Didn't know, I'll update. | |
38 | You mean there is no interest in supporting this: TEST(raw_sha1_ostreamTest, Intermediate) { llvm::raw_sha1_ostream Sha1Stream; Sha1Stream << "Hello"; auto Hash = toHex(Sha1Stream.sha1()); ASSERT_EQ("F7FF9E8B7BB2E09B70935A5D785E0CC5D9D0ABF0", Hash); Sha1Stream << " World!"; Hash = toHex(Sha1Stream.sha1()); ASSERT_EQ("2EF7BDE608CE5404E97D5F042F95F89F1C232871", Hash); } right? | |
lib/Support/SHA1.cpp | ||
29 | All this file is almost copy pasted (and moved to a LLVM style) : http://llvm.org/docs/doxygen/html/FuzzerSHA1_8cpp_source.html |
include/llvm/Support/SHA1.h | ||
---|---|---|
38 | As written we don't support that (you'll addUncounted one bit, and then the padding. Afterwards, adding to the stream won't do what it would need to do). I'd like that to be explicit in the documentation (and I'd like an assert or two to make it trigger in debug mode, if that's attempted). |
include/llvm/Support/SHA1.h | ||
---|---|---|
38 | I'm not sure what you mean, the InternalState is saved and restored in SHA1::result() so that we can resume as is. (this unittest is passing) |
If perfs matter, I would do it that way:
https://github.com/bitcoin/bitcoin/blob/master/src/crypto/sha1.cpp
lib/Support/SHA1.cpp | ||
---|---|---|
71 | I would do 2 loops |
Thanks for all the comments, I tried to address them.
As long as we're OK with the API, we'll still be able to improve the implementation later performance-wise if necessary.
include/llvm/Support/SHA1.h | ||
---|---|---|
38 | support both behavior through different API calls now. | |
lib/Support/SHA1.cpp | ||
71 | not sure what you mean? |
Anything else to change or can we move on and fix whatever comes up post-commit?
(this is blocking D18213 which is accepted)
No blocks on my side. Thank you.
P.S: It seems I had a comment to be submitted for a few weeks... Sorry for the lag. :-(
include/llvm/Support/SHA1.h | ||
---|---|---|
38 | Sorry, didn't notice that line before pad() (and the one that restores it). Thanks! | |
lib/Support/SHA1.cpp | ||
167 | Nit: 20 bytes (not "characters"), since it's just the raw bits. |
Hi folks,
This change breaks on Linux X86 targetting X86, when running check-all or if LLVM_BUILD_TESTS is on:
15:53:55 [ 44%] Building CXX object unittests/Support/CMakeFiles/SupportTests.dir/raw_sha1_ostream_test.cpp.o
15:53:56 [ 44%] Linking CXX executable SupportTests
15:53:56 CMakeFiles/SupportTests.dir/formatted_raw_ostream_test.cpp.o: In function `(anonymous namespace)::formatted_raw_ostreamTest_Test_Tell_Test::TestBody()':
15:53:56 formatted_raw_ostream_test.cpp:(.text._ZN12_GLOBAL__N_140formatted_raw_ostreamTest_Test_Tell_Test8TestBodyEv+0x1ae): undefined reference to `llvm::formatted_raw_ostream::current_pos() const'
15:53:56 collect2: error: ld returned 1 exit status
15:53:56 make[3]: * [unittests/Support/SupportTests] Error 1
15:53:56 make[2]: * [unittests/Support/CMakeFiles/SupportTests.dir/all] Error 2
15:53:56 make[1]: * [CMakeFiles/check-all.dir/rule] Error 2
15:53:56 make: * [check-all] Error 2
This came from a Jenkins job I'm setting up (I can reproduce it manually on a different machine too).
I've uploaded the full console log and build scripts here:
https://drive.google.com/folderview?id=0B1KcalYwX-jDOFVPSGxSV1JrWHM&usp=sharing
The main entrypoints into the scripts are build-setup.sh and test-setup.sh
The failure is strange, I suspect DLLVM_LINK_LLVM_DYLIB is playing tricks here...
Also the link failure is in formatted_raw_ostream_test.cpp, which this revision didn't touch. What makes you think this commit is responsible?
I think usually (confirmed in OpenSSL and CommonCrypto) these functions are called Init(), Update(), and Final().
I don't care about the case, but we should probably use the same words (just like MD5.h is doing).