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

Build fails with clang 7: invalid operand for instruction #60

Closed
erijo opened this issue Feb 12, 2020 · 5 comments
Closed

Build fails with clang 7: invalid operand for instruction #60

erijo opened this issue Feb 12, 2020 · 5 comments

Comments

@erijo
Copy link
Contributor

erijo commented Feb 12, 2020

When trying to build the assembly variant of the latest master with clang 7 it fails:

$ make test_asm CC=clang-7
clang-7 -O3 -Wall -Wextra -std=c11 -pedantic -DBLAKE3_TESTING -fsanitize=address,undefined  blake3.c blake3_dispatch.c blake3_portable.c main.c blake3_sse41-x86_64-unix.S blake3_avx2-x86_64-unix.S blake3_avx512-x86_64-unix.S -o blake3
blake3_sse41-x86_64-unix.S:1380:9: error: invalid operand for instruction
        jne 1b
        ^
blake3_sse41-x86_64-unix.S:1636:9: error: invalid operand for instruction
        jmp 1b
...

Clang 8 works as expected.

@erijo
Copy link
Contributor Author

erijo commented Feb 12, 2020

This was seen when trying to fix #58 in ccache/ccache#519 by switching to the assembly implementation.

@sneves
Copy link
Collaborator

sneves commented Feb 12, 2020

This is this clang bug, running from version 5 to 7. It will need replacing 1 labels with something higher..

A quick fix would replace every instance of 1: 1f 1b by 9: 9f 9b respectively.

@erijo
Copy link
Contributor Author

erijo commented Feb 13, 2020

I tried the following snippet to move all labels +1:

for f in *.S; do
    awk 'match($0, /^([1-4]):/, m) {printf "%d:\n", m[1] + 1; next}; match($0, /^(.* )([1-4])([bf].*)/, m) {printf "%s%d%s\n", m[1], m[2] + 1, m[3]; next}; {print}' $f > $f.new
    mv -f $f.new $f
done

Which compiled. But then when running the test it failed with:

AddressSanitizer:DEADLYSIGNAL
=================================================================
==185329==ERROR: AddressSanitizer: SEGV on unknown address 0x00007e14ab10 (pc 0x00000050e8cc bp 0x7ffd7e14ab50 sp 0x7ffd7e14a940 T0)
==185329==The signal is caused by a READ memory access.
    #0 0x50e8cb in get_cpu_features /home/erik/source/BLAKE3/c/blake3_dispatch.c:178:11
    #1 0x60d7e5 in main /home/erik/source/BLAKE3/c/main.c:128:20
    #2 0x7fe52bcccbba in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x26bba)
    #3 0x41d329 in _start (/home/erik/source/BLAKE3/c/blake3+0x41d329)

This happens with both clang 7 and 8. Also seen when testing the asm branch. GCC works.

@sneves
Copy link
Collaborator

sneves commented Feb 13, 2020

That crash had nothing to do with the assembly, and things were working by chance everywhere. I accidentally deleted the x86_64 path for cpuid when fixing #51, and so rbx was getting truncated to ebx. In clang-7 it happened to hold a pointer instead of nothing of interest.

The fix for this is already in the asm branch.

@erijo
Copy link
Contributor Author

erijo commented Feb 13, 2020

Thanks, seems to be working!

oconnor663 added a commit that referenced this issue Feb 13, 2020
This release is motivated by a fix for a potential security
vulnerability. 421a21a fixes a bug
introduced in a1c4c4e. A truncated
pointer register led to a segfault on x86-64 under Clang 7 and 8.
Clang 9 happens to be unaffected, but the behavior is undefined in
general. See also:
#60 (comment)

The C implementation of BLAKE3 hasn't been formally packaged anywhere,
and most callers vendor code from master. This release tag is intended
to make the fix above more visible, to encourage callers to update their
vendored copies. We will continue to publish tags like this whenever
bugs in the C implementation are fixed, or if there are any incompatible
API changes.

Note that the issue above does not impact callers of the Rust `blake3`
crate. The affected file, `blake3_dispatch.c`, is not compiled by that
crate in any configuration. It does impact callers of the internal
`blake3_c_rust_bindings` crate, but that crate is not published on
crates.io and not intended for production use.
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

No branches or pull requests

2 participants