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

add BLAKE3 hash #13194

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/actions/verify-generated-files/action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,5 @@ runs:
ext/tokenizer/tokenizer_data_gen.php
build/gen_stub.php -f
build/gen_stub.php --generate-optimizer-info
ext/hash/blake3/fetch_upstream_blake3.sh
git add . -N && git diff --exit-code
1 change: 1 addition & 0 deletions README.REDIST.BINS
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
18. avifinfo (ext/standard/libavifinfo) see ext/standard/libavifinfo/LICENSE
19. xxHash (ext/hash/xxhash)
20. Lexbor (ext/dom/lexbor/lexbor) see ext/dom/lexbor/LICENSE
21. BLAKE3 (ext/hash/blake3) see ext/hash/blake3/upstream_blake3/LICENSE


3. pcre2lib (ext/pcre)
Expand Down
95 changes: 95 additions & 0 deletions build/php.m4
Original file line number Diff line number Diff line change
Expand Up @@ -2830,3 +2830,98 @@ AC_DEFUN([PHP_CHECK_AVX512_VBMI_SUPPORTS], [
AC_DEFINE_UNQUOTED([PHP_HAVE_AVX512_VBMI_SUPPORTS],
[$have_avx512_vbmi_supports], [Whether the compiler supports AVX512 VBMI])
])


dnl PHP_CHECK_ARM_NEON_SUPPORT
dnl check if we're compiling for ARM NEON
AC_DEFUN([PHP_CHECK_ARM_NEON_SUPPORT], [
AC_CACHE_CHECK([for ARM NEON support],ac_cv_target_arm_neon,[
AC_RUN_IFELSE([AC_LANG_SOURCE([[
int main(void) {
#if defined(__ARM_NEON__) || defined(__ARM_NEON)
return 0;
#else
return 1;
#endif
}
]])],[
ac_cv_target_arm_neon=yes
],[
ac_cv_target_arm_neon=no
],[
ac_cv_target_arm_neon=no
])])
])


dnl
dnl PHP_CHECK_X86_TARGET
dnl
dnl check if we're compiling for x86/x86_64
dnl
AC_DEFUN([PHP_CHECK_X86_TARGET], [
AC_CACHE_CHECK([for x86 target],ac_cv_target_x86,[
AC_RUN_IFELSE([AC_LANG_SOURCE([[
int main(void) {
#if defined(__x86_64__) || defined(__i386__)
return 0;
#else
return 1;
#endif
}
]])],[
ac_cv_target_x86=yes
],[
ac_cv_target_x86=no
],[
ac_cv_target_x86=no
])])
])

dnl
dnl PHP_CHECK_WINDOWS_TARGET
dnl
dnl check if we're compiling for windows
dnl
AC_DEFUN([PHP_CHECK_WINDOWS_TARGET], [
AC_CACHE_CHECK([for windows target],ac_cv_target_windows,[
AC_RUN_IFELSE([AC_LANG_SOURCE([[
int main(void) {
#if defined(_WIN32)
return 0;
#else
return 1;
#endif
}
]])],[
ac_cv_target_windows=yes
],[
ac_cv_target_windows=no
],[
ac_cv_target_windows=no
])])
])

dnl
dnl PHP_CHECK_UNIX_TARGET
dnl
dnl check if we're compiling for a unix-ish target
dnl
AC_DEFUN([PHP_CHECK_UNIX_TARGET], [
AC_CACHE_CHECK([for unix-ish target],ac_cv_target_unix,[
AC_RUN_IFELSE([AC_LANG_SOURCE([[
int main(void) {
#if defined(unix) || defined(__unix) || defined(__unix__)
return 0;
#else
return 1;
#endif
}
]])],[
ac_cv_target_unix=yes
],[
ac_cv_target_unix=no
],[
ac_cv_target_unix=no
])])
])
13 changes: 13 additions & 0 deletions configure.ac
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,19 @@ PHP_EBCDIC
dnl Check whether the system byte ordering is bigendian.
PHP_C_BIGENDIAN

dnl Check if we're targeting x86 / x86_64
PHP_CHECK_X86_TARGET

dnl Check if we're targeting ARM Neon CPUs
PHP_CHECK_ARM_NEON_SUPPORT


dnl Check if we're targeting Windows
PHP_CHECK_WINDOWS_TARGET

dnl Check whether we're targeting a unix-ish system
PHP_CHECK_UNIX_TARGET

dnl Check whether writing to stdout works.
PHP_TEST_WRITE_STDOUT

Expand Down
18 changes: 18 additions & 0 deletions ext/hash/blake3/fetch_upstream_blake3.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
#!/bin/sh
divinity76 marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a personal opinion, but pcre2 nor opcache's JIT code are cloned. I do not see a particular reason to make an exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@devnexen this script makes it easy to see exactly how blake3 1.5.0 was cloned, and should make it easy to update to future versions (like 1.6.0) in the future - IMO it's worthwhile to keep this script. idk if it should be part of .github/actions/verify-generated-files/action.yml tho (there's a discussion on that topic here #13194 (comment) )

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not see the appeal nor the necessity. Couple of counter arguments from the top of my head :

  • does not make build easier, CI included.
  • if this blake3 release contains bugs, the upgrade can be done in the same way the internal pcre2 library does. There is no necessity to upgrade versions if it brings nothing to php or goes against php's needs otherwise.
  • if this contains bugs we are able to fix quickly but would take a long time (or worse getting refused) upstream, in your case we would need to patch it on the fly or forking the repo.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@divinity76 The point of verify-generated-files is to avoid sneaking malicious code into generated files that are collapsed during code reviews that people don't generally look at. This risk doesn't exist here. IMO, if the intent is to avoid people mistakenly editing these files, the same issue already exists for all other copied projects, and should be solved universally (if at all).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iluuu1994

The point of verify-generated-files is to avoid sneaking malicious code into generated files that are collapsed during code reviews that people don't generally look at. This risk doesn't exist here.

Interesting, thanks for the explanation!

Maybe I snuck some malicious code into one of the upstream_blake3/* files? If I did, this script would actually revert it, and then verify-generated-files would notice it!

@devnexen

I do not see the appeal

My main appeal is that if I want to update the bundled blake3 in the future, i can just change the 1 line git clone --branch '1.5.0' with git clone --branch '1.5.1' and the script should do the rest.

does not make build easier, CI included.

fair

if this blake3 release contains bugs, the upgrade can be done in the same way the internal pcre2 library does.

How does upstream pcre2 fixes get applied to the internal pcre2 library? i don't know

There is no necessity to upgrade versions if it brings nothing to php or goes against php's needs otherwise.

There's good updates sometimes, for example the latest v1.5.0 fixed a arm big-endian compile issue, v1.4.1 increased ARM performance by ~10-30%, 1.3.3 fixed a GCC6.1 AVX512 debug corruption issue.

if this contains bugs we are able to fix quickly but would take a long time (or worse getting refused) upstream, in your case we would need to patch it on the fly or forking the repo.

Yeah that's a good point. If we get to that point, this script is useless. Anecdotal evidence, the only time I made a PR to BLAKE3, it was accepted in 11 hours ( BLAKE3-team/BLAKE3#131 ), but it was a trivial one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's good updates sometimes, for example the latest v1.5.0 fixed a arm big-endian compile issue, v1.4.1 increased ARM performance by ~10-30%, 1.3.3 fixed a GCC6.1 AVX512 debug corruption issue.

Those are valid reasons to upgrade but again upgrades can be handled in same manner at the aforementioned php components. Note that I m not against having blake3 feature, I see it as a "nice to have", just the way of the dependency being handled.

# afaik the PHP project doesn't allow git submodules, so we do this fetcher script instead.
cd "$(dirname "$0")"
rm -rf upstream_blake3
# fancy way of fetching only the files we want
git clone --branch '1.5.0' -n --depth=1 --filter=tree:0 'https://github.com/BLAKE3-team/BLAKE3.git' 'upstream_blake3'
cd upstream_blake3
git sparse-checkout set --no-cone LICENSE c/blake3.c c/blake3.h c/blake3_avx2.c c/blake3_avx2_x86-64_unix.S \
c/blake3_avx2_x86-64_windows_gnu.S c/blake3_avx2_x86-64_windows_msvc.asm c/blake3_avx512.c \
c/blake3_avx512_x86-64_unix.S c/blake3_avx512_x86-64_windows_gnu.S c/blake3_avx512_x86-64_windows_msvc.asm \
c/blake3_dispatch.c c/blake3_impl.h c/blake3_neon.c c/blake3_portable.c c/blake3_sse2.c \
c/blake3_sse2_x86-64_unix.S c/blake3_sse2_x86-64_windows_gnu.S c/blake3_sse2_x86-64_windows_msvc.asm \
c/blake3_sse41.c c/blake3_sse41_x86-64_unix.S c/blake3_sse41_x86-64_windows_gnu.S \
c/blake3_sse41_x86-64_windows_msvc.asm
git checkout
rm -rf .git
cd ..
git apply patches.diff
80 changes: 80 additions & 0 deletions ext/hash/blake3/patches.diff
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
diff --git a/ext/hash/blake3/upstream_blake3/c/blake3.c b/ext/hash/blake3/upstream_blake3/c/blake3.c
index 692f4b0216..3591ba245a 100644
--- a/ext/hash/blake3/upstream_blake3/c/blake3.c
+++ b/ext/hash/blake3/upstream_blake3/c/blake3.c
@@ -341,21 +341,23 @@ INLINE void compress_subtree_to_parent_node(
size_t num_cvs = blake3_compress_subtree_wide(input, input_len, key,
chunk_counter, flags, cv_array);
assert(num_cvs <= MAX_SIMD_DEGREE_OR_2);
-
- // If MAX_SIMD_DEGREE is greater than 2 and there's enough input,
- // compress_subtree_wide() returns more than 2 chaining values. Condense
- // them into 2 by forming parent nodes repeatedly.
- uint8_t out_array[MAX_SIMD_DEGREE_OR_2 * BLAKE3_OUT_LEN / 2];
- // The second half of this loop condition is always true, and we just
+ // https://github.com/BLAKE3-team/BLAKE3/pull/380
+ // This condition is always true, and we just
// asserted it above. But GCC can't tell that it's always true, and if NDEBUG
// is set on platforms where MAX_SIMD_DEGREE_OR_2 == 2, GCC emits spurious
// warnings here. GCC 8.5 is particularly sensitive, so if you're changing
// this code, test it against that version.
- while (num_cvs > 2 && num_cvs <= MAX_SIMD_DEGREE_OR_2) {
+#if MAX_SIMD_DEGREE_OR_2 > 2
+ // If MAX_SIMD_DEGREE_OR_2 is greater than 2 and there's enough input,
+ // compress_subtree_wide() returns more than 2 chaining values. Condense
+ // them into 2 by forming parent nodes repeatedly.
+ uint8_t out_array[MAX_SIMD_DEGREE_OR_2 * BLAKE3_OUT_LEN / 2];
+ while (num_cvs > 2) {
num_cvs =
compress_parents_parallel(cv_array, num_cvs, key, flags, out_array);
memcpy(cv_array, out_array, num_cvs * BLAKE3_OUT_LEN);
}
+#endif
memcpy(out, cv_array, 2 * BLAKE3_OUT_LEN);
}

diff --git a/ext/hash/blake3/upstream_blake3/c/blake3_dispatch.c b/ext/hash/blake3/upstream_blake3/c/blake3_dispatch.c
index af6c3dadc7..af3bf17bbe 100644
--- a/ext/hash/blake3/upstream_blake3/c/blake3_dispatch.c
+++ b/ext/hash/blake3/upstream_blake3/c/blake3_dispatch.c
@@ -86,7 +86,6 @@ static void cpuidex(uint32_t out[4], uint32_t id, uint32_t sid) {
#endif
}

-#endif

enum cpu_feature {
SSE2 = 1 << 0,
@@ -161,6 +160,8 @@ static
#endif
}
}
+// https://github.com/BLAKE3-team/BLAKE3/pull/382
+#endif

void blake3_compress_in_place(uint32_t cv[8],
const uint8_t block[BLAKE3_BLOCK_LEN],
diff --git a/ext/hash/blake3/upstream_blake3/c/blake3_neon.c b/ext/hash/blake3/upstream_blake3/c/blake3_neon.c
index 8a818fc78f..c4b4548edf 100644
--- a/ext/hash/blake3/upstream_blake3/c/blake3_neon.c
+++ b/ext/hash/blake3/upstream_blake3/c/blake3_neon.c
@@ -9,15 +9,13 @@
#endif

INLINE uint32x4_t loadu_128(const uint8_t src[16]) {
- // vld1q_u32 has alignment requirements. Don't use it.
- uint32x4_t x;
- memcpy(&x, src, 16);
- return x;
+ // https://github.com/BLAKE3-team/BLAKE3/pull/384
+ return vreinterpretq_u32_u8(vld1q_u8(src));
}

INLINE void storeu_128(uint32x4_t src, uint8_t dest[16]) {
- // vst1q_u32 has alignment requirements. Don't use it.
- memcpy(dest, &src, 16);
+ // https://github.com/BLAKE3-team/BLAKE3/pull/384
+ vst1q_u8(dest, vreinterpretq_u8_u32(src));
}

INLINE uint32x4_t add_128(uint32x4_t a, uint32x4_t b) {