1

I have Little Endian WordHH,WordHL,WordLH,WordLL values and they represent with a,b,c,d

I have this number as a decimal 1234456789 it equals as a hex 0x075BCD15 so I call the function like this :

BitShifting(0xCD15,0x075D,0x0000,0x000000)

But return value equals = 123587861 Why this happening where is my mistake ?

uint64_t BitShifting(uint16_t a,uint16_t b, uint16_t c, uint16_t d)

{
    uint64_t val;                                                                                                    
    val = ((a & 0x000000000000FFFF) | ((b << 16) & 0x00000000FFFF0000) | ((c << 32) & 0x0000FFFF00000000) | ((d << 48) & 0xFFFF000000000000));
    return val;
    
}
hobik
  • 439
  • 4
  • 15
  • 2
    You have a typo: `0x075D` should be `0x075B`. – Nate Eldredge Jun 29 '21 at 06:30
  • 2
    You will find more bugs if you try to use nonzero values for `c` and `d`. The expression `c << 32` is evaluated as `int` arithmetic and will overflow when `c != 0`, causing undefined behavior. – Nate Eldredge Jun 29 '21 at 06:31
  • 1
    @Nate Eldredge , The fix is `((uint64_t)c) << 32`, right? – ikegami Jun 29 '21 at 06:32
  • Oh, my god tahnk you. :) – hobik Jun 29 '21 at 06:32
  • Yes, that would do it, and likewise for `b,c,d`. (`b << 16` may also overflow `int` if the high bit of `b` is set.) – Nate Eldredge Jun 29 '21 at 06:33
  • Do you think a better code could be written? – hobik Jun 29 '21 at 06:35
  • @Tom Karzes Re "*makes a lot more sense to mask first, then shift.*", Except there's no reason to mask at all. – ikegami Jun 29 '21 at 06:47
  • @ikegami Yes, unless OP expects to pass values that don't fit into 16 bits, and wants them masked. But I see the args are declared as `uint16_t`, so yeah, masking is unnecessary. – Tom Karzes Jun 29 '21 at 06:51
  • 1
    Does this answer your question? [why can't you shift a uint16\_t](https://stackoverflow.com/questions/55066827/why-cant-you-shift-a-uint16-t) – phuclv Jun 29 '21 at 06:59
  • 1
    duplicates: [Why must I cast a `uint8_t` to `uint64_t` *before* left-shifting it?](https://stackoverflow.com/q/32228933/995714), [Problem of converting byte order for unsigned 64-bit number in C](https://stackoverflow.com/q/60457248/995714), [Merging uint8 array into uint64 error](https://stackoverflow.com/q/25669317/995714) – phuclv Jun 29 '21 at 07:04
  • Not duplicate, I want to answer my question I found another method for use. But I didnt answer why ? – hobik Jun 29 '21 at 08:13

1 Answers1

2

1,234,456,78910 = 4994,50D516
123,587,86110 = 75D,CD1516
So the correct value is being returned.

Which is not to say there are no problem.

On a machine with a 32-bit int type, the values will be promoted to int values for the shifts, but shifting by 32 and 64 is undefined behaviour and can't produce the correct result. Also, shifting by 16 is undefined behaviour on such a machine if the most significant bit of the input is set.

This means for common architectures, your code doesn't handle c != 0 and d != 0 at all, and b > 0x7FFF might not work reliably. To fix this, upcast the inputs to the shift to their final type.

uint64_t BitShifting(uint16_t a, uint16_t b, uint16_t c, uint16_t d) {
   return
      (uint64_t)a       | 
      (uint64_t)b << 16 | 
      (uint64_t)c << 32 |
      (uint64_t)d << 48;
}

(The first cast is not actually needed.)

The masks were pointless, so I removed them.


Demonstatration of the above working:

#include <inttypes.h>
#include <stdint.h>
#include <stdio.h>

uint64_t BitShifting(uint16_t a, uint16_t b, uint16_t c, uint16_t d) {
   return
      (uint64_t)a       | 
      (uint64_t)b << 16 | 
      (uint64_t)c << 32 |
      (uint64_t)d << 48;
}

int main() {
   printf("%016"PRIX64"\n", BitShifting(0xCD15, 0x075B, 0x0000, 0x0000));
   printf("%016"PRIX64"\n", BitShifting(0x0123, 0x4567, 0x89AB, 0xCDEF));
}
$ gcc -Wall -Wextra -pedantic a.c -o a && ./a
00000000075BCD15
CDEF89AB45670123
ikegami
  • 367,544
  • 15
  • 269
  • 518
  • Thank you but when I try with this I get "0" value: SerialPrint("%d",BitShifting(0xCD15,0x075B,0x0000 ,0x0000)); – hobik Jun 29 '21 at 07:19
  • *shrug* Can't comment on `SerialPrint` since I've never seen it. But passing a `uint64_t` to `%d` seems very fishy. With the printf family of functions, `%d` expects an `int`. – ikegami Jun 29 '21 at 07:41
  • 1
    @hobik This answer address the posted problem. If there are additional problems in code that was not posted, then that's a separate issue. In that case, I would accept this answer, then create a new post that includes the problem code. – Tom Karzes Jun 29 '21 at 07:46
  • I want to answer my question, I found another method. How can I share my answer – hobik Jun 29 '21 at 08:15
  • Why did you close my question, I dont understand – hobik Jun 29 '21 at 08:16
  • People voted to close for two different reasons: 1) The code you provided resulted the correct result. You merely thought it returned the wrong result. 2) The actual problem with the code was answered elsewhere. (See the links in the comments on the question). I got my answer in before the answer was closed. – ikegami Jun 29 '21 at 08:24