-3

I've been recording some numbers, but they've been verified to be wrong. However they don't look random, What exactly was I doing to the old numbers?.

uint8_t * m_rawValue

// The way I recently learned was wrong
return (uint16_t)*m_rawValue;
// The correct way
uint16_t* a = (uint16_t*)m_rawValue;
return a[0];
Adam Smith
  • 1
  • 2
  • 2
    You should only need `return *m_rawValue;`. The return statement will handle converting `*m_rawValue` into the return type. – NathanOliver Nov 27 '18 at 21:18
  • Um..... Neither is really correct, unless you actually know what you're doing. It's also going to create endian-dependent code. Casting a single element `uint8_t*` to `uint16_t*` is going to be undefined behavior, since it's going to use the higher 8 bits from the `uint8_t`, and the lower 8 bits will be random memory. – Alex Huszagh Nov 27 '18 at 21:18
  • 4
    Somehow, although short, this question still manages to be confusing. Who verified them to be wrong? And why should they appear random? –  Nov 27 '18 at 21:19
  • 1
    @hacksalot, agree -- specially with use of `m_rawValue` in one line and `m->m_rawValue` in another. – R Sahu Nov 27 '18 at 21:20
  • 1
    @hacksalot I was able to recreate the test as well. I'm getting bits with `modbus tcp` and using a simulator that lets me input values into registers I found that I was wrong. I would expect that if I was de-referencing incorrectly I would just be grabbing random memory locations. – Adam Smith Nov 27 '18 at 21:26
  • Thanks @Adam, makes sense. I probably need a vacation. –  Nov 27 '18 at 21:27
  • You probably have an endian issue. What happens if you convert the endianness? https://stackoverflow.com/questions/105252/how-do-i-convert-between-big-endian-and-little-endian-values-in-c – NathanOliver Nov 27 '18 at 21:30
  • 1
    Please describe the task that this code is supposed to be performing. Are you trying to extract a 16 bit value from a buffer of 8 bit values, or are you trying to extract an 8 bit value from a buffer of 8 bit values through an interface that requires you to return a 16 bit value? – Tim Randall Nov 27 '18 at 21:37
  • There is not enough information here. If you want to widen the values of your `uint8_t` then your first solution is correct. If you want to read every two `uint8_t` elements as a single `uint16_t` then neither is correct (although the second one may well work on some implementations). – Galik Nov 27 '18 at 22:06

1 Answers1

-2

What you were doing before:

  • Indirecting your uint8_t* array - this will result in a temporary uint8_t value, representing the first byte of your array
  • Casting that result to a uint16_t, and returning it.
  • In the end, you were returning the value of the first byte pointed to by m_rawValue.

What you were doing after:

  • Casting your uint8_t* pointer to a uint16_t* pointer - this tells the compiler to treat the value of a as a pointer to a bunch of 2-byte integers (uint16_t*) instead of a pointer to a bunch of 1-byte integers (uint8_t*)
  • Returning the first two bytes pointed to by m_rawValue as a uint16_t

Assumptions made:

  • That m_rawValue actually points to a buffer of valid uint16_t values. Since you say that you get the right value after your changes, this seems like a safe assumptions.

Edited to make clear assumptions I made.

ArtHare
  • 1,798
  • 20
  • 22
  • 3
    On what system does casting a `uint8_t` to a `uint16_t` produce a value greater than 255? – Tim Randall Nov 27 '18 at 21:27
  • 3
    Note that `(uint16_t*)m_rawValue` is not good code. Strict aliasing allows any object to be viewed as bytes, but the other way around, bytes viewed as anything else, is not guaranteed. The typical guaranteed solution is to use `memcpy`. – user4581301 Nov 27 '18 at 21:27
  • Where did you get all the details from? How do you know `m_rawValue` is an array? – SergeyA Nov 27 '18 at 21:31
  • 2
    There is no `uint16_t` array to return a value from here. All you have is a pointer to `uint16_t` that points to something idfferent. You have strict alias violation and the code is undefined behavior. Any answer that implies that it can predict what the code labeled "correct way" (it's not correct) does is incorrect. That is unless you can know that `m_rawValue`'s value is originally a pointer to a `uint16_t` that was cast to `uint8_t*`. – François Andrieux Nov 27 '18 at 21:59
  • As a note to @user4581301, I just want to add, that the use of memmove is preferable to memcpy since the aliasing issues. – Ilian Zapryanov Nov 27 '18 at 22:38
  • @IlianZapryanov PLease expand on that. I don't see benefit to `memmove` unless the source pointer and the destination somehow overlap. – user4581301 Nov 27 '18 at 22:58
  • `Tim Randall` - My bad, I was thinking of a totally different type of cast mishap I encounter too often when necessity requires the mixing of LE and BE values. user458... - Totally agree. `SergeyA` and `Francois` - Based on that the OP says their code gets their desired values after their changes and the way they were treating the value, I made the assumption that m_rawValue was a pointer to a `uint16_t` array. I can predict the "after" code does the right thing because the OP said it did. – ArtHare Nov 28 '18 at 01:25
  • @ArtHare That line of reasoning is categorically wrong in C++. Observing the expected behavior does not indicate an absence of undefined behavior. The existance of UB in C++ means you can't assume correctness based on the observed or reported behavior. At the very least you would need to share the assumptions you had to make to declare the expected behavior of your code. Edit : I now see you did that in a subsequent edit. Edit 2 : Nevermind it's still incorrect. I'll post again when I get to a computer. This has nothing to do with the values of the pointed bytes. – François Andrieux Nov 29 '18 at 12:35
  • @ArtHare Strict aliasing is a rule in c++ that two pointers of unrelated types cannot point to the same object. If `foo` and `bar` are unrelated types, a `foo*` that points to the address of an instance of `bar` cannot be dereferenced. The same is true of `uint8_t` and `uint16_t`. A pointer to `uint16_t` that points to a `uint8_t` cannot be dereferenced, even if the pointer points to an address of valid memory that would hold a valid memory representation of `uint16_t`. It's undefined behavior to do it. It might work for a while, but it could break at any time for any reason. – François Andrieux Nov 29 '18 at 14:45
  • In practice, when the compiler sees a `uint8_t*` and `uint16_t*` is it allowed to assume changes to the pointed `uint8_t` cannot possibly impact the pointed `uint16_t`. As such, it may optimize the code and rearrange operations in a way that you may not expect and would break the logic of your program but that is non the less conformant. See [type aliasing](https://en.cppreference.com/w/cpp/language/reinterpret_cast#Type_aliasing). – François Andrieux Nov 29 '18 at 14:49
  • That's interesting. It seems that any compiler that did that style of UB would break basically every C++ program and most of the APIs I've ever seen that manipulates buffers or reports data in the form `unsigned char*`. If the `unsigned char*` buffers must be translated to non-`uint8_t` forms in order to be useful, how does one do that safely? How do you implement `void thisRawBufferActuallyIncludesFloats(uint8_t* pb)`? (asking honestly) – ArtHare Nov 29 '18 at 17:11