msan: notate more variable assignments from assembly code #1512

pull theuni wants to merge 2 commits into bitcoin-core:master from theuni:x86-asm-clean-msan2 changing 1 files +22 −19
  1. theuni commented at 3:25 pm on March 27, 2024: contributor

    This was missed in 31ba40494428dcbf2eb5eb6f2328eca91b0b0746 because older versions of clang did not complain about it. But clang-17, at least, does.

    The array-as-a-param makes this annoying because sizeof(l) is not helpful. I’d be happy to change the size calculation if there are any better suggestions or strong preferences.

  2. theuni commented at 3:33 pm on March 27, 2024: contributor
    Should fix #1511
  3. fanquake commented at 3:51 pm on March 27, 2024: member
    Nice. This fixes #1511 for me, under Clang 17 (Ubuntu clang version 17.0.6 (5build1)) and 18 (Ubuntu clang version 18.1.0 (rc2-4)). I’ve also cherry-picked it into https://github.com/bitcoin/bitcoin/pull/29742 to run through our CI.
  4. real-or-random approved
  5. real-or-random commented at 1:35 pm on March 28, 2024: contributor
    utACK 241a2145d15f5b9e2978c3a76d4fec9eefbc96f8
  6. real-or-random commented at 1:48 pm on March 28, 2024: contributor

    The array-as-a-param makes this annoying because sizeof(l) is not helpful.

    Hm, yeah, idk, the uint64_t l[8] parameter declaration is a bit strange, or at least unusual for this code base. I’d ACK a patch that changes this a pointer declaration, which we usually do in the codebase. If you do, maybe also rename to l8.

    For others: The issue is that, despite the declaration, l will still decay to a pointer, i.e., l won’t be of array type, but of type uint64_t*. And the [8] has no significance. That means that sizeof(l) == sizeof(uint64_t*), which, for maximum confusion, happens to be 8 coincidentally.

    https://github.com/bitcoin-core/secp256k1/blob/05bfab69aef3622f77f754cfb01220108a109c91/src/scalar_4x64_impl.h#L684

  7. theuni commented at 2:50 pm on March 29, 2024: contributor

    TIL of the weird-looking

    0void func(uint64_t l[static 8])
    

    syntax in c99: https://stackoverflow.com/questions/3430315/what-is-the-purpose-of-static-keyword-in-array-parameter-of-function-like-char

    Sadly it doesn’t do anything to help us here, just thought I’d share. Yet another totally unrelated thing that static can mean in c/c++ code :)

    I’ll add a commit to change this to a pointer as suggested.

  8. theuni force-pushed on Mar 29, 2024
  9. theuni force-pushed on Mar 29, 2024
  10. in src/scalar_4x64_impl.h:825 in fa86ffa415 outdated
    821 #else
    822     /* 160 bit accumulator. */
    823     uint64_t c0 = 0, c1 = 0;
    824     uint32_t c2 = 0;
    825 
    826     /* l[0..7] = a[0..3] * b[0..3]. */
    


    real-or-random commented at 0:40 am on March 30, 2024:
    0    /* l8[0..7] = a[0..3] * b[0..3]. */
    

    And there are some other comments that may require updating… E.g., Extract l7. I admit that these are confusing now that the variable name is l8. But I guess it should anyway be Extract l8[7]


    theuni commented at 3:55 pm on April 3, 2024:
    Ah, I figured comments like l6 still made sense conceptually, but sure, I’ll update them to match.
  11. real-or-random commented at 0:45 am on March 30, 2024: contributor
    0void func(uint64_t l[static 8])
    

    Haha okay, I totally wasn’t aware of this, even though I happen to look at the standards text from time to time… And yeah, static is a bit cursed.

  12. real-or-random added the label assurance on Apr 2, 2024
  13. real-or-random added the label refactor/smell on Apr 2, 2024
  14. change inconsistent array param to pointer
    The behavior is identical, but the former syntax suggests guarantees that
    don't actually exist.
    a61339149f
  15. msan: notate more variable assignments from assembly code
    This was missed in 31ba40494428dcbf2eb5eb6f2328eca91b0b0746 because older
    versions of clang did not complain about it. But clang-17, at least, does.
    f7f0184ba1
  16. theuni force-pushed on Apr 3, 2024
  17. real-or-random approved
  18. real-or-random commented at 4:59 pm on April 3, 2024: contributor
    ACK f7f0184ba1358cb8659607d2d0105628cb130e4f tests work fine with clang 17 and ./configure CFLAGS="-fsanitize=memory -fsanitize-memory-track-origins=2 -fno-omit-frame-pointer -g -O1 -fno-optimize-sibling-calls" CC=clang
  19. sipa commented at 5:00 pm on April 3, 2024: contributor
    utACK f7f0184ba1358cb8659607d2d0105628cb130e4f
  20. real-or-random merged this on Apr 3, 2024
  21. real-or-random closed this on Apr 3, 2024

  22. fanquake referenced this in commit 5354807b00 on Apr 4, 2024
  23. fanquake referenced this in commit 53eec53dca on Apr 4, 2024
  24. fanquake referenced this in commit b5d21182e5 on Apr 6, 2024
  25. real-or-random commented at 2:02 pm on April 9, 2024: contributor

    For posterity, the reason why clang 15 was happy with just the annotation in https://github.com/bitcoin-core/secp256k1/commit/31ba40494428dcbf2eb5eb6f2328eca91b0b0746 is this:

    The default of what is considered a “use” of uninitialized memory was changed in clang 16. Returning an uninitialized variables from a function, or passing uninitialized values to a function as a parameter is now considered, and MSan will report it by default. See the Clang 16.0.09 Release Notes:

    -fsanitize-memory-param-retval is turned on by default. With -fsanitize=memory, passing uninitialized variables to functions and returning uninitialized variables from functions is more aggressively reported. -fno-sanitize-memory-param-retval restores the previous behavior.


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/secp256k1. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-02 14:15 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me