Fix bit reversion function
diff mbox

Message ID 1453393016-20734-1-git-send-email-suraev@alumni.ntnu.no
State Accepted
Headers show

Commit Message

Suraev Jan. 21, 2016, 4:16 p.m. UTC
From: Max <msuraev@sysmocom.de>

Mark unsigned value as such.
Fix unaligned access error revealed by asan on 32 bit builds.

Sponsored-by: On-Waves ehf
---
 include/osmocom/core/bits.h | 2 +-
 src/bits.c                  | 8 +++-----
 2 files changed, 4 insertions(+), 6 deletions(-)

Comments

Holger Freyther Jan. 21, 2016, 4:21 p.m. UTC | #1
> On 21 Jan 2016, at 17:16, suraev@alumni.ntnu.no wrote:
> 
> From: Max <msuraev@sysmocom.de>
> 
> Mark unsigned value as such.
> Fix unaligned access error revealed by asan on 32 bit builds.

Linux vagrant-ubuntu-wily-64 4.2.0-23-generic #28-Ubuntu SMP Sun Dec 27 17:47:31 UTC 2015 x86_64 x86_64 x86_64 GNU/Linux
Holger Freyther Jan. 21, 2016, 8:50 p.m. UTC | #2
> On 21 Jan 2016, at 17:16, suraev@alumni.ntnu.no wrote:
> 
> 
> -void osmo_revbytebits_buf(uint8_t *buf, int len);
> +void osmo_revbytebits_buf(uint8_t *buf, unsigned int len);

yes that makes sense but please do not mix bugfix and API
change in one (unless the API change is the bugfix). E.g.
2/3 of the change is "noise"


> @@ -221,8 +220,7 @@ void osmo_revbytebits_buf(uint8_t *buf, int len)
> 	}
> 
> 	for (i = unaligned_cnt; i + 3 < len; i += 4) {
> -		uint32_t *cur = (uint32_t *) (buf + i);
> -		*cur = osmo_revbytebits_32(*cur);
> +		osmo_store32be(osmo_revbytebits_32(osmo_load32be(buf + i)), buf + i);

		 uint32_t cur;
		 memcpy(&cur, buf + 1, sizeof(cur));
		 cur = osmo_revbytebits_32(cur);
		 memcpy(buf + 1, &cur, sizeof(cur));


would be my approach. So let's compare it. Your code without the loop
is here https://goo.gl/vD3kqQ and the memcpy variant is https://goo.gl/5kzTmx
now if there would be a cloud microbenchmark we could resolve that once and
for all.

Patch
diff mbox

diff --git a/include/osmocom/core/bits.h b/include/osmocom/core/bits.h
index 1587b05..d559185 100644
--- a/include/osmocom/core/bits.h
+++ b/include/osmocom/core/bits.h
@@ -75,7 +75,7 @@  uint32_t osmo_revbytebits_32(uint32_t x);
 uint32_t osmo_revbytebits_8(uint8_t x);
 
 /* \brief reverse the bits of each byte in a given buffer */
-void osmo_revbytebits_buf(uint8_t *buf, int len);
+void osmo_revbytebits_buf(uint8_t *buf, unsigned int len);
 
 /*! \brief left circular shift
  *  \param[in] in The 16 bit unsigned integer to be rotated
diff --git a/src/bits.c b/src/bits.c
index 01d7e73..e90bb71 100644
--- a/src/bits.c
+++ b/src/bits.c
@@ -206,10 +206,9 @@  uint32_t osmo_revbytebits_8(uint8_t x)
  *
  *  This function reverses the bits in each byte of the buffer
  */
-void osmo_revbytebits_buf(uint8_t *buf, int len)
+void osmo_revbytebits_buf(uint8_t *buf, unsigned int len)
 {
-	unsigned int i;
-	unsigned int unaligned_cnt;
+	unsigned int i, unaligned_cnt;
 	int len_remain = len;
 
 	unaligned_cnt = ((unsigned long)buf & 3);
@@ -221,8 +220,7 @@  void osmo_revbytebits_buf(uint8_t *buf, int len)
 	}
 
 	for (i = unaligned_cnt; i + 3 < len; i += 4) {
-		uint32_t *cur = (uint32_t *) (buf + i);
-		*cur = osmo_revbytebits_32(*cur);
+		osmo_store32be(osmo_revbytebits_32(osmo_load32be(buf + i)), buf + i);
 		len_remain -= 4;
 	}