tests: Switch one of the Travis jobs to an unsigned char environment (-funsigned-char) #15134

pull practicalswift wants to merge 1 commits into bitcoin:master from practicalswift:unsigned-char changing 1 files +2 −2
  1. practicalswift commented at 7:05 AM on January 9, 2019: contributor

    Switch one of the Travis jobs to an unsigned char environment (-funsigned-char).

    This will help us catch errors due to code written under the assumption that char has the same value range as signed char.

    The signedness of char is implementation-defined.

    Example:

    $ uname -a
    Linux […] x86_64 x86_64 x86_64 GNU/Linux
    $ cat foo.cpp
    #include <iostream>
    
    int main() {
        char c;
        std::cin >> c;
        int i = (unsigned char)c;
        std::cout << i << "\n";
    }
    $ clang++ -o foo foo.cpp
    $ echo -e "\xff" | ./foo
    255
    $ clang++ -fsigned-char -o foo foo.cpp
    $ echo -e "\xff" | ./foo
    255
    $ clang++ -funsigned-char -o foo foo.cpp
    $ echo -e "\xff" | ./foo
    255
    $ cat bar.cpp
    #include <iostream>
    
    int main() {
        char c;
        std::cin >> c;
        int i = c;
        std::cout << i << "\n";
    }
    $ clang++ -o bar bar.cpp
    $ echo -e "\xff" | ./bar
    -1
    $ clang++ -fsigned-char -o bar bar.cpp
    $ echo -e "\xff" | ./bar
    -1
    $ clang++ -funsigned-char -o bar bar.cpp
    $ echo -e "\xff" | ./bar
    255
    

    gcc chars:

    • signed: alpha, hppa, ia64, m68k, mips, sh, sparc, x86
    • unsigned: arm, powerpc, s390

    About -funsigned-char:

    Let the type "char" be unsigned, like "unsigned char".

    Each kind of machine has a default for what "char" should be. It is either like "unsigned char" by default or like "signed char" by default.

    Ideally, a portable program should always use "signed char" or "unsigned char" when it depends on the signedness of an object. But many programs have been written to use plain "char" and expect it to be signed, or expect it to be unsigned, depending on the machines they were written for.

    This option, and its inverse, let you make such a program work with the opposite default. The type "char" is always a distinct type from each of "signed char" or "unsigned char", even though its behavior is always just like one of those two.

  2. fanquake added the label Tests on Jan 9, 2019
  3. laanwj commented at 2:41 PM on January 9, 2019: member

    Concept ACK, mistakes with char are quite common in C use, without this I guess things suddenly might start to break on ARM or RISC-V without Travis noticing.

  4. practicalswift renamed this:
    build: Add a Travis ASan/LSan/UBSan job testing in a unsigned char environment (-funsigned-char)
    tests: Add a Travis ASan/LSan/UBSan job testing in a unsigned char environment (-funsigned-char)
    on Jan 9, 2019
  5. laanwj commented at 2:52 PM on January 16, 2019: member

    @theuni can you take a look at this please

  6. laanwj assigned theuni on Jan 16, 2019
  7. DrahtBot commented at 10:53 PM on January 30, 2019: member

    <!--e57a25ab6845829454e8d69fc972939a-->

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #16320 (ci: Add Travis check to make sure unit test coverage reports stay deterministic by practicalswift)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  8. Rspigler commented at 9:55 PM on February 1, 2019: contributor

    Would this cover Power ISA? (https://github.com/bitcoin/bitcoin/pull/14066)

  9. DrahtBot added the label Needs rebase on Feb 14, 2019
  10. DrahtBot removed the label Needs rebase on Feb 14, 2019
  11. DrahtBot added the label Needs rebase on Feb 14, 2019
  12. practicalswift force-pushed on Feb 15, 2019
  13. DrahtBot removed the label Needs rebase on Feb 15, 2019
  14. laanwj commented at 11:39 PM on February 16, 2019: member

    Would this cover Power ISA? (#14066)

    PowerPC has unsigned chars afaik so in a sense, yea.

  15. practicalswift commented at 8:25 PM on March 8, 2019: contributor

    Can we move forward with this one? :-)

    Should hopefully be trivial to review since it only touches .travis.yml.

    Perhaps you could take a look too @MarcoFalke? :-)

  16. in .travis.yml:154 in 9542b8391a outdated
     149 | +      name: 'x86_64 Linux  [GOAL: install]  [bionic]  [no depends, only system libs, sanitizers: address/leak (ASan + LSan) + undefined (UBSan) + integer] [unsigned char]'
     150 | +      env: >-
     151 | +        HOST=x86_64-unknown-linux-gnu
     152 | +        PACKAGES="clang llvm python3-zmq qtbase5-dev qttools5-dev-tools libssl1.0-dev libevent-dev bsdmainutils libboost-system-dev libboost-filesystem-dev libboost-chrono-dev libboost-test-dev libboost-thread-dev libdb5.3++-dev libminiupnpc-dev libzmq3-dev libprotobuf-dev protobuf-compiler libqrencode-dev"
     153 | +        NO_DEPENDS=1
     154 | +        FUNCTIONAL_TESTS_CONFIG="--exclude wallet_multiwallet.py" # Temporarily suppress ASan heap-use-after-free (see issue #14163)
    


    MarcoFalke commented at 8:40 PM on March 8, 2019:

    FUNCTIONAL_TESTS_CONFIG was removed long ago

  17. practicalswift force-pushed on Mar 9, 2019
  18. practicalswift commented at 6:03 AM on March 9, 2019: contributor

    @MarcoFalke Thanks for reviewing! Now rebased and updated. Please re-review :-)

  19. in .travis.yml:143 in 47af5ccb2b outdated
     138 | +      env: >-
     139 | +        HOST=x86_64-unknown-linux-gnu
     140 | +        PACKAGES="clang llvm python3-zmq qtbase5-dev qttools5-dev-tools libssl1.0-dev libevent-dev bsdmainutils libboost-system-dev libboost-filesystem-dev libboost-chrono-dev libboost-test-dev libboost-thread-dev libdb5.3++-dev libminiupnpc-dev libzmq3-dev libprotobuf-dev protobuf-compiler libqrencode-dev"
     141 | +        NO_DEPENDS=1
     142 | +        GOAL="install"
     143 | +        BITCOIN_CONFIG="--enable-zmq --with-incompatible-bdb --with-gui=qt5 CFLAGS='-funsigned-char' CPPFLAGS='-DDEBUG_LOCKORDER -funsigned-char' --with-sanitizers=address,integer,undefined CC=clang CXX=clang++"
    


    MarcoFalke commented at 5:22 PM on March 11, 2019:

    Sorry to further nit-pick, but what are the advantages of making this a matrix job (duplicating the above and only changing this one)?

    If there are none, I'd rather combine them.


    practicalswift commented at 8:44 AM on March 12, 2019:

    The advantage is that we get testing under the sanitizers for both signed and unsigned char builds.

    Combining them would mean that we only get sanitizer testing for unsigned char builds.


    MarcoFalke commented at 4:59 PM on March 12, 2019:

    I guess the default is signed char, so developers should run those already.

    If you insist, I'd rather combine it with one of the jobs further up (dash, or the bionic job right after the one using dash)

    We already use 12 jobs and the free travis capacity is 5 jobs in parallel, so if we bloat the matrix further, travis might slow down some devs that run it on their own repo.


    practicalswift commented at 12:41 PM on March 13, 2019:

    Fixed!

  20. practicalswift force-pushed on Mar 13, 2019
  21. practicalswift renamed this:
    tests: Add a Travis ASan/LSan/UBSan job testing in a unsigned char environment (-funsigned-char)
    tests: Switch one of the Travis jobs to an unsigned char environment (-funsigned-char)
    on Mar 13, 2019
  22. practicalswift commented at 8:37 AM on March 14, 2019: contributor

    @MarcoFalke Updated version looks good?

  23. practicalswift force-pushed on Mar 14, 2019
  24. MarcoFalke commented at 9:00 PM on March 14, 2019: member

    ACK

  25. practicalswift commented at 3:17 PM on April 1, 2019: contributor

    @laanwj Would you mind reviewing?

    I think we should be ready for merge :-)

  26. in .travis.yml:104 in 11ffa2e231 outdated
     101 |          HOST=x86_64-unknown-linux-gnu
     102 |          PACKAGES="python3-zmq qtbase5-dev qttools5-dev-tools protobuf-compiler libdbus-1-dev libharfbuzz-dev libprotobuf-dev"
     103 |          DEP_OPTS="NO_QT=1 NO_UPNP=1 DEBUG=1 ALLOW_HOST_PACKAGES=1"
     104 |          GOAL="install"
     105 | -        BITCOIN_CONFIG="--enable-zmq --with-gui=qt5 --enable-glibc-back-compat --enable-reduce-exports --enable-debug CXXFLAGS=\"-g0 -O2\""
     106 | +        BITCOIN_CONFIG="--enable-zmq --with-gui=qt5 --enable-glibc-back-compat --enable-reduce-exports --enable-debug CXXFLAGS=\"-g0 -O2 -funsigned-char\""
    


    luke-jr commented at 5:59 AM on April 17, 2019:

    Don't we need -funsigned-char in CFLAGS as well, for our C code?

    And what about dependencies? Are there situations where the mismatch may result in ABI breakage?


    practicalswift commented at 7:46 AM on April 17, 2019:

    Good point! Added CFLAGS as well.

    I'm not sure what to do about dependencies TBH: I guess this change is as far as we can get in terms of low-cost unsigned char testing in Travis. Let me know if you think I'm mistaken and there is some way to do this in a smarter way :-)

  27. practicalswift force-pushed on Apr 17, 2019
  28. tests: Switch one of the Travis jobs to an unsigned char environment (-funsigned-char) 0c78e49be3
  29. practicalswift force-pushed on Jun 26, 2019
  30. practicalswift commented at 2:04 PM on June 28, 2019: contributor

    @MarcoFalke @laanwj Is this one ready for merge? Having one -funsigned-char testing job would make me slightly less worried about mistakes due to incorrect signedness assumptions for char :-)

    Let me know if any further changes are needed!

  31. practicalswift commented at 10:08 AM on July 30, 2019: contributor

    @MarcoFalke @laanwj Friendly ping: is this PR still of interest? Let me know if it should closed :-)

  32. MarcoFalke commented at 11:56 AM on July 30, 2019: member

    Concept ACK, but I am lacking the background to properly review

  33. laanwj commented at 12:59 PM on July 30, 2019: member

    ACK 0c78e49be3a258695b7f363f2d5b1cfdb93f9522

  34. laanwj merged this on Jul 30, 2019
  35. laanwj closed this on Jul 30, 2019

  36. laanwj referenced this in commit 74f1a27f2f on Jul 30, 2019
  37. sidhujag referenced this in commit aee14eaf22 on Jul 30, 2019
  38. MarcoFalke referenced this in commit 7027c67cac on Jul 2, 2020
  39. practicalswift deleted the branch on Apr 10, 2021
  40. PastaPastaPasta referenced this in commit 89b248a190 on Sep 11, 2021
  41. PastaPastaPasta referenced this in commit 39ad464fd9 on Sep 11, 2021
  42. PastaPastaPasta referenced this in commit ac7f4cb16e on Sep 11, 2021
  43. PastaPastaPasta referenced this in commit 74c4da8a5e on Sep 11, 2021
  44. PastaPastaPasta referenced this in commit f3d40e05e4 on Sep 12, 2021
  45. PastaPastaPasta referenced this in commit e50cd8c727 on Sep 12, 2021
  46. PastaPastaPasta referenced this in commit bd0e9c90e2 on Sep 12, 2021
  47. vijaydasmp referenced this in commit dfa262c93f on Oct 4, 2021
  48. DrahtBot locked this on Aug 18, 2022

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-13 18:15 UTC

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