“missing operand” assembler warnings on Windows #28111

1. 
   hebasto commented at 10:51 am on July 20, 2023: member

   On the master branch @673acab223c0f896767b1ae784659df9f95452ae on Ubuntu 23.04:

    0$ make -C depends HOST=x86_64-w64-mingw32 NO_QT=1 DEBUG=1 NO_HARDEN=1
    1$ ./configure CONFIG_SITE=$PWD/depends/x86_64-w64-mingw32/share/config.site
    2$ make
    3...
    4  CXX      crypto/libbitcoin_crypto_base_la-aes.lo
    5  CXX      crypto/libbitcoin_crypto_base_la-chacha_poly_aead.lo
    6  CXX      crypto/libbitcoin_crypto_base_la-chacha20.lo
    7  CXX      crypto/libbitcoin_crypto_base_la-hkdf_sha256_32.lo
    8  CXX      crypto/libbitcoin_crypto_base_la-hmac_sha256.lo
    9  CXX      crypto/libbitcoin_crypto_base_la-hmac_sha512.lo
   10  CXX      crypto/libbitcoin_crypto_base_la-poly1305.lo
   11  CXX      crypto/libbitcoin_crypto_base_la-muhash.lo
   12  CXX      crypto/libbitcoin_crypto_base_la-ripemd160.lo
   13  CXX      crypto/libbitcoin_crypto_base_la-sha1.lo
   14  CXX      crypto/libbitcoin_crypto_base_la-sha256.lo
   15  CXX      crypto/libbitcoin_crypto_base_la-sha3.lo
   16  CXX      crypto/libbitcoin_crypto_base_la-sha512.lo
   17  CXX      crypto/libbitcoin_crypto_base_la-siphash.lo
   18  CXX      crypto/libbitcoin_crypto_base_la-sha256_sse4.lo
   19{standard input}: Assembler messages:
   20{standard input}:42: Warning: missing operand; zero assumed
   21{standard input}:42: Warning: missing operand; zero assumed
   22{standard input}:42: Warning: missing operand; zero assumed
   23{standard input}:42: Warning: missing operand; zero assumed
   24{standard input}:42: Warning: missing operand; zero assumed
   25{standard input}:42: Warning: missing operand; zero assumed
   26{standard input}:42: Warning: missing operand; zero assumed
   27{standard input}:42: Warning: missing operand; zero assumed
   28{standard input}:42: Warning: missing operand; zero assumed
   29{standard input}:42: Warning: missing operand; zero assumed
   30{standard input}:42: Warning: missing operand; zero assumed
   31{standard input}:42: Warning: missing operand; zero assumed
   32{standard input}:42: Warning: missing operand; zero assumed
   33{standard input}:42: Warning: missing operand; zero assumed
   34{standard input}:42: Warning: missing operand; zero assumed
   35{standard input}:42: Warning: missing operand; zero assumed
   36{standard input}:42: Warning: missing operand; zero assumed
   37{standard input}:42: Warning: missing operand; zero assumed
   38  CXXLD    crypto/libbitcoin_crypto_base.la
   39...
   

   Having a non-hardened build, the actual reason for the warnings is the absence of the -fstack-protector-all flag.

   cc @sipa

2. 
   fanquake commented at 10:57 am on July 20, 2023: member

   I don’t understand what the actual problem is, or what is broken? Can you elaborate?
3. 
   hebasto commented at 11:00 am on July 20, 2023: member

   I don’t understand what the actual problem is, or what is broken? Can you elaborate?

   I’m not an expert in assembly code, but “missing operand’ sounds scary.

   I don’t know whether this warning is safe to ignore.

4. 
   fanquake commented at 12:14 pm on July 20, 2023: member

   Probably needs some actual investigation then: i.e what code + combination of compiler flags is causing the issue, produce a minimal test case. Is this a “new” issue, or have the warnings existed in our codebase “forever”. Is it related to the fact that you’re using ubuntu mantic (devel) and/or compiler/binutils versions etc.
5. fanquake added the label Windows on Jul 20, 2023
6. 
   sipa commented at 3:47 pm on July 20, 2023: member

   Try this:

    0--- a/src/crypto/sha256_sse4.cpp
    1+++ b/src/crypto/sha256_sse4.cpp
    2@@ -949,7 +949,7 @@ void Transform(uint32_t* s, const unsigned char* chunk, size_t blocks)
    3 
    4         "Ldone_hash_%=:"
    5 
    6-        : "+r"(s), "+r"(chunk), "+r"(blocks), "=r"(a), "=r"(b), "=r"(c), "=r"(d), /* e = chunk */ "=r"(f), "=r"(g), "=r"(h), "=r"(y0), "=r"(y1), "=r"(y2), "=r"(tbl), "+m"(inp_end), "+m"(inp), "+m"(xfer)
    7+        : "+r"(s), "+r"(chunk), "+r"(blocks), "=r"(a), "=r"(b), "=r"(c), "=r"(d), /* e = chunk */ "=r"(f), "=r"(g), "=r"(h), "=r"(y0), "=r"(y1), "=r"(y2), "=r"(tbl), "+m"(inp_end), "+m"(inp), "+o"(xfer)
    8         : "m"(K256), "m"(FLIP_MASK), "m"(SHUF_00BA), "m"(SHUF_DC00)
    9         : "cc", "memory", "xmm0", "xmm1", "xmm2", "xmm3", "xmm4", "xmm5", "xmm6", "xmm7", "xmm8", "xmm9", "xmm10", "xmm11", "xmm12"
   10    );
   

   (replace “+m” with “+o” at the end of the line)

7. 
   fanquake commented at 4:03 pm on July 20, 2023: member

   Try this: (replace “+m” with “+o” at the end of the line)

   The same warnings occur with that diff:

    0# git diff
    1diff --git a/src/crypto/sha256_sse4.cpp b/src/crypto/sha256_sse4.cpp
    2index f4557291c..ebc99aa64 100644
    3--- a/src/crypto/sha256_sse4.cpp
    4+++ b/src/crypto/sha256_sse4.cpp
    5@@ -949,7 +949,7 @@ void Transform(uint32_t* s, const unsigned char* chunk, size_t blocks)
    6 
    7         "Ldone_hash_%=:"
    8 
    9-        : "+r"(s), "+r"(chunk), "+r"(blocks), "=r"(a), "=r"(b), "=r"(c), "=r"(d), /* e = chunk */ "=r"(f), "=r"(g), "=r"(h), "=r"(y0), "=r"(y1), "=r"(y2), "=r"(tbl), "+m"(inp_end), "+m"(inp), "+m"(xfer)
   10+        : "+r"(s), "+r"(chunk), "+r"(blocks), "=r"(a), "=r"(b), "=r"(c), "=r"(d), /* e = chunk */ "=r"(f), "=r"(g), "=r"(h), "=r"(y0), "=r"(y1), "=r"(y2), "=r"(tbl), "+m"(inp_end), "+m"(inp), "+o"(xfer)
   11         : "m"(K256), "m"(FLIP_MASK), "m"(SHUF_00BA), "m"(SHUF_DC00)
   12         : "cc", "memory", "xmm0", "xmm1", "xmm2", "xmm3", "xmm4", "xmm5", "xmm6", "xmm7", "xmm8", "xmm9", "xmm10", "xmm11", "xmm12"
   13    );
   14
   15# make -C src crypto/libbitcoin_crypto_base_la-sha256_sse4.lo
   16make: Entering directory '/bitcoin/src'
   17  CXX      crypto/libbitcoin_crypto_base_la-sha256_sse4.lo
   18{standard input}: Assembler messages:
   19{standard input}:42: Warning: missing operand; zero assumed
   20{standard input}:42: Warning: missing operand; zero assumed
   21{standard input}:42: Warning: missing operand; zero assumed
   22{standard input}:42: Warning: missing operand; zero assumed
   23{standard input}:42: Warning: missing operand; zero assumed
   24{standard input}:42: Warning: missing operand; zero assumed
   25{standard input}:42: Warning: missing operand; zero assumed
   26{standard input}:42: Warning: missing operand; zero assumed
   27{standard input}:42: Warning: missing operand; zero assumed
   28{standard input}:42: Warning: missing operand; zero assumed
   29{standard input}:42: Warning: missing operand; zero assumed
   30{standard input}:42: Warning: missing operand; zero assumed
   31{standard input}:42: Warning: missing operand; zero assumed
   32{standard input}:42: Warning: missing operand; zero assumed
   33{standard input}:42: Warning: missing operand; zero assumed
   34{standard input}:42: Warning: missing operand; zero assumed
   35{standard input}:42: Warning: missing operand; zero assumed
   36{standard input}:42: Warning: missing operand; zero assumed
   37make: Leaving directory '/bitcoin/src'
   

8. 
   sipa commented at 4:35 pm on July 20, 2023: member

   Can someone compile with -save-temps and get me the sha256_sse4.s file?
9. 
   hebasto commented at 5:43 pm on July 20, 2023: member

   Can someone compile with -save-temps and get me the sha256_sse4.s file?

   For the master branch:

   • https://github.com/hebasto/artefacts/blob/master/issue-28111/asm-master/sha256_sse4.s

   Master branch + the patch:

   • https://github.com/hebasto/artefacts/blob/master/issue-28111/asm-patched/sha256_sse4.s

   (both files are the same actually)

10. 
    sipa commented at 6:14 pm on July 20, 2023: member

    Ok so what seems to be happening is that in your build, %16 gets mapped to (%rsp) (aka the memory address stored in the stack pointer), which in a few places gets used as e.g. 8+%16, which results in 8+(%rsp), and that appears to be invalid asm code.

    On my system (and presumably ~every build we have since this code was introduced), %16 is mapped to 16(%rsp) (16 bytes further than the memory address stored in the stack pointer), and thus 8+%16 becomes 8+16(%rsp), which is 24 bytes further than the memory address stored in the stack pointer.

    One solution is perhaps replacing all the +%16 with +0%16 so that 8+0%16 becomes 8+0(%rsp), but that feels a bit icky - I’m not familiar with asm enough to know if this is always acceptable (as for normal builds it’d mean 8+016(%rsp)). I’d expect some kind of modifier to exist to make it output the 0, but I can’t find any such thing in the documentation.

    So… can you try if that (substitute +%16 everywhere with +0%16) works?

11. 
    hebasto commented at 6:19 pm on July 20, 2023: member

    So… can you try if that (substitute +%16 everywhere with +0%16) works?

    It compiles. Going to run tests.

12. 
    hebasto commented at 6:43 pm on July 20, 2023: member

    The crypto_tests test suit passes as well.

    To ensure that the SSE4 implementation is being tested, ENABLE_X86_SHANI, ENABLE_SSE41 and ENABLE_AVX2 were disabled manually, that was confirmed in debug.log:

    02023-07-20T18:38:31Z Using the 'sse4(1way)' SHA256 implementation
    

13. 
    hebasto commented at 6:50 pm on July 20, 2023: member

    @sipa

    Ok so what seems to be happening is that in your build, %16 gets mapped to (%rsp) (aka the memory address stored in the stack pointer), which in a few places gets used as e.g. 8+%16, which results in 8+(%rsp), and that appears to be invalid asm code.

    On my system (and presumably ~every build we have since this code was introduced), %16 is mapped to 16(%rsp) (16 bytes further than the memory address stored in the stack pointer), and thus 8+%16 becomes 8+16(%rsp), which is 24 bytes further than the memory address stored in the stack pointer.

    Is there any reasonable explanation why the -fstack-protector-all compiler flag affects that mapping?

14. 
    sipa commented at 6:52 pm on July 20, 2023: member

    Yes, indirectly. Having that flag or not affects the stack layout. The issue is triggered by the “xfer” variable being allocated at the top of the stack.
15. 
    sipa commented at 7:32 pm on July 20, 2023: member

    FWIW, I assume this issue has existed since this code was introduced, but isn’t harmful apart from preventing compilation, as far as I can tell.
16. 
    fanquake commented at 7:34 pm on July 20, 2023: member

    I don’t actually think it even prevents compilation, and only warns? Unclear from the issue.
17. 
    hebasto commented at 7:35 pm on July 20, 2023: member

    I don’t actually think it even prevents compilation, and only warns? Unclear from the issue.

    Only warnings. Compilation succeeds.

18. 
    sipa commented at 7:36 pm on July 20, 2023: member

    Errr, if it doesn’t prevent compilation it’s possible that the result is wrong (when the warning is emitted). Do tests pass?
19. 
    hebasto commented at 7:36 pm on July 20, 2023: member

    Errr, if it doesn’t prevent compilation it’s possible that the result is wrong (when the warning is emitted). Do tests pass?

    They do.

20. 
    sipa commented at 8:29 pm on July 20, 2023: member

    In that case it seems to be doing the right thing despite the warning.
21. 
    fanquake commented at 1:47 pm on July 21, 2023: member

    Ok, going to close this for now then. Looks like next steps are to open an issue upstream with binutils, and report the (potential) false positive.
22. fanquake closed this on Jul 21, 2023

    ---

23. 
    fanquake commented at 8:45 am on August 3, 2023: member

    @hebasto did you open an issue to track this upstream? Can you link it here?
24. bitcoin locked this on Aug 2, 2024

Contributors
hebasto fanquake sipa

Labels
Windows