4 ms·

Funnily enough just spent a day tracking down a weird problem in an embedded system - some event timestamps were getting corrupted in a weird way. The pattern w

by zh3 4y ago

Funnily enough just spent a day tracking down a weird problem in an embedded system - some event timestamps were getting corrupted in a weird way. The pattern wasn't obvious until in desperation I dumped them in hex and found the second MSB was toggling between 0 and 1 (whereas it should have been been part of a count sequence). That told me exactly where to look - where the count was re-assembled from four bytes and I found this upstream (paraphrased):- uint8_t tx_data[64]; .... tx_data[43] = utime>>24; tx_data[44] = utime>16; tx_data[45] = utime>>8; tx_data[46] = utime;

zoomablemind 4y ago
tx_data[44] = utime>16; Good catch! I wonder if this would have been flagged with -Wall?
- edflsafoiewq 4y ago
  Nope. There's no warning for bool->int conversion: https://stackoverflow.com/questions/28716391/gcc-forbid-implicit-bool-int-conversion https://stackoverflow.com/questions/28716391/gcc-forbid-impl...
  - HarHarVeryFunny 4y ago
    Somewhat oddly even in C++, where bool is a distinct type, g++ doesn't have any warning for implicit bool to int conversion, although clang's clang-tidy "linter" tool does.
- zh3 4y ago
  Nope, code was clean with -Wall on arm-9 compiler (i.e. gcc). Interesting thought now you say that (no warning) - only found it because of seeing the pattern (apropos the article) and from that having a good idea what was causing it (knowing the int was assembled a byte at a time). If I had a criticism of modern compilers, it's the blizzard of uninteresting warnings ("strncmp takes const char star, did you really mean to pass it unsigned char star") that make people want to not use -Wall.
  - JohnFen 4y ago
    All warnings are uninteresting until they're not. > ("strncmp takes const char star, did you really mean to pass it unsigned char star") This warning (with a different function) actually saved my bacon once, pointing me to a very obscure bug in the code. My practice is to use -Wall and make sure that the code compiles without any warnings at all. Then I don't have a deluge of warnings to wade through.
    - markrages 4y ago
      Remember -Wall isn't all. -Wall -Wextra is the new -Wall. I recommend asan too, where possible.
      - JohnFen 4y ago
        Oh, you're right. I need to update my build system.
      - PokestarFan 4y ago
        Why do compilers do stupid things like this? Seriously don't get why -Wall isn't all warnings.
- kps 4y ago
  clang-tidy has an open issue to catch this. https://github.com/llvm/llvm-project/issues/56009 https://github.com/llvm/llvm-project/issues/56009
layer8 4y ago
This is one case where lining up values (and surrounding operators with spaces) can be good practice: tx_data[43] = utime >> 24; tx_data[44] = utime >> 16; tx_data[45] = utime >> 8; tx_data[46] = utime >> 0; Or even: tx_data[43] = utime >> (3 * 8); tx_data[44] = utime >> (2 * 8); tx_data[45] = utime >> (1 * 8); tx_data[46] = utime >> (0 * 8);
- mtlmtlmtlmtl 4y ago
  Yep. Bugs like this just pop out at you this way. Having spent a lot of time digging into C code and doing microcontroller programming professionally, I have learned a certain disdain for the practice of using minimal whitespace in expressions, and the related practice of minimising LOC using C's very dense expressions syntax(unary dec/inc operators, the fact that assignment is itself an expression, etc). The dense expressions syntax is pretty much a holdover from the time C was invented by Dennis Ritchie for the purpose of porting Unix; when it had to be typed on a painfully slow electromechanical teletype. This is also why most Unix commands are 2-4 characters long. But it serves very little meaningful purpose today. It's important to remember that you might be able to turn 3 lines of code into 1, but it'll still be the same amount of machine code. And it will probably be harder to read, harder to change, and harder to reason about. And it can be a lot easier to miss some edgecase or even a typo like in this case. I have seen such a silly amount of off-by-one errors in C code due to some subtle misuse of unary increment/decrement that I stopped using those operators altogether. They don't improve your software in any meaningful way.
- ziml77 4y ago
  I've done the >> 0 thing to make it very clear what's going on, but I hadn't considered the * 8 construction. That's actually easier to comprehend at a glance because the numbers become less "magic". (8 and 16 are obvious to me, but 24 always has me second-guessing myself somewhere in the back of my mind)
  - lolcatuser 4y ago
    I use octal for this. x << (3 * 8); vs. x << 030;
Gibbon1 4y ago
This is a good example Why C's implicit conversions are trash. Why big endian is trash. Why you should do a reverse copy instead of stuff like this.

by zh3 4y ago

zoomablemind 4y ago

tx_data[44] = utime>16; Good catch! I wonder if this would have been flagged with -Wall?

edflsafoiewq 4y ago
Nope. There's no warning for bool->int conversion: https://stackoverflow.com/questions/28716391/gcc-forbid-implicit-bool-int-conversion https://stackoverflow.com/questions/28716391/gcc-forbid-impl...
- HarHarVeryFunny 4y ago
  Somewhat oddly even in C++, where bool is a distinct type, g++ doesn't have any warning for implicit bool to int conversion, although clang's clang-tidy "linter" tool does.
zh3 4y ago
Nope, code was clean with -Wall on arm-9 compiler (i.e. gcc). Interesting thought now you say that (no warning) - only found it because of seeing the pattern (apropos the article) and from that having a good idea what was causing it (knowing the int was assembled a byte at a time). If I had a criticism of modern compilers, it's the blizzard of uninteresting warnings ("strncmp takes const char star, did you really mean to pass it unsigned char star") that make people want to not use -Wall.
- JohnFen 4y ago
  All warnings are uninteresting until they're not. > ("strncmp takes const char star, did you really mean to pass it unsigned char star") This warning (with a different function) actually saved my bacon once, pointing me to a very obscure bug in the code. My practice is to use -Wall and make sure that the code compiles without any warnings at all. Then I don't have a deluge of warnings to wade through.
  - markrages 4y ago
    Remember -Wall isn't all. -Wall -Wextra is the new -Wall. I recommend asan too, where possible.
    - JohnFen 4y ago
      Oh, you're right. I need to update my build system.
    - PokestarFan 4y ago
      Why do compilers do stupid things like this? Seriously don't get why -Wall isn't all warnings.
kps 4y ago
clang-tidy has an open issue to catch this. https://github.com/llvm/llvm-project/issues/56009 https://github.com/llvm/llvm-project/issues/56009

layer8 4y ago

This is one case where lining up values (and surrounding operators with spaces) can be good practice: tx_data[43] = utime >> 24; tx_data[44] = utime >> 16; tx_data[45] = utime >> 8; tx_data[46] = utime >> 0; Or even: tx_data[43] = utime >> (3 * 8); tx_data[44] = utime >> (2 * 8); tx_data[45] = utime >> (1 * 8); tx_data[46] = utime >> (0 * 8);

mtlmtlmtlmtl 4y ago
Yep. Bugs like this just pop out at you this way. Having spent a lot of time digging into C code and doing microcontroller programming professionally, I have learned a certain disdain for the practice of using minimal whitespace in expressions, and the related practice of minimising LOC using C's very dense expressions syntax(unary dec/inc operators, the fact that assignment is itself an expression, etc). The dense expressions syntax is pretty much a holdover from the time C was invented by Dennis Ritchie for the purpose of porting Unix; when it had to be typed on a painfully slow electromechanical teletype. This is also why most Unix commands are 2-4 characters long. But it serves very little meaningful purpose today. It's important to remember that you might be able to turn 3 lines of code into 1, but it'll still be the same amount of machine code. And it will probably be harder to read, harder to change, and harder to reason about. And it can be a lot easier to miss some edgecase or even a typo like in this case. I have seen such a silly amount of off-by-one errors in C code due to some subtle misuse of unary increment/decrement that I stopped using those operators altogether. They don't improve your software in any meaningful way.
ziml77 4y ago
I've done the >> 0 thing to make it very clear what's going on, but I hadn't considered the * 8 construction. That's actually easier to comprehend at a glance because the numbers become less "magic". (8 and 16 are obvious to me, but 24 always has me second-guessing myself somewhere in the back of my mind)
- lolcatuser 4y ago
  I use octal for this. x << (3 * 8); vs. x << 030;

Gibbon1 4y ago

This is a good example Why C's implicit conversions are trash. Why big endian is trash. Why you should do a reverse copy instead of stuff like this.