The test suite for libosmocore fails on big-endian architectures
Hi, While building the package for Debian, apparently there is a problem related to big-endian architectures. Please see the logs here: https://buildd.debian.org/status/fetch.php?pkg=libosmocore&arch=powerpc&ver=... and https://buildd.debian.org/status/package.php?p=libosmocore It is related to the test case for smscb and the struct gsm341_ms_message: +++ /«PKGBUILDDIR»/tests/testsuite.dir/at-groups/7/stdout 2015-12-05 15:51:53.463182812 +0000 @@ -1,4 +1,4 @@ -(srl) GS: 1 MSG_CODE: 1 UPDATE: 0 +(srl) GS: 0 MSG_CODE: 256 UPDATE: 1 (msg) msg_id: 1293 -(dcs) group: 1 language: 0 +(dcs) group: 0 language: 1 (pge) page total: 1 current: 1 7. testsuite.at:45: 7. smscb (testsuite.at:45): FAILED (testsuite.at:48) Could you please suggest how we should fix this so that the package also can build for powerpc and other big-endian architectures? Thank you very much in advance. Ruben
Hi Ruben, On Sun, Dec 06, 2015 at 04:53:09PM +0100, Ruben Undheim wrote:
While building the package for Debian, apparently there is a problem related to big-endian architectures.
While I still own several PPC machines, I haven't booted any of them in years, and don't have a build setup ready. Please try the patch below and report back if it works. If yes, we can merge it.
From 51ae645e220556bbeabce3ac57304639328e2164 Mon Sep 17 00:00:00 2001 From: Harald Welte <laforge@gnumonks.org> Date: Sun, 6 Dec 2015 22:12:43 +0100 Subject: [PATCH] untested fix for gsm_03_41.h and big-endian machines
Our gsm_03_41 structs use bit-fields, but don't do the usual little/big-endian jumping. --- include/osmocom/gsm/protocol/gsm_03_41.h | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/include/osmocom/gsm/protocol/gsm_03_41.h b/include/osmocom/gsm/protocol/gsm_03_41.h index 0ece6cc..f007cc1 100644 --- a/include/osmocom/gsm/protocol/gsm_03_41.h +++ b/include/osmocom/gsm/protocol/gsm_03_41.h @@ -2,6 +2,7 @@ #include <stdint.h> +#include <osmocom/core/endian.h> #include <osmocom/gsm/protocol/gsm_04_12.h> /* GSM TS 03.41 definitions also TS 23.041*/ @@ -13,19 +14,36 @@ /* Chapter 9.3.2 */ struct gsm341_ms_message { struct { +#ifdef OSMO_IS_LITTLE_ENDIAN uint8_t code_hi:6; uint8_t gs:2; uint8_t update:4; uint8_t code_lo:4; +#else + uint8_t code_lo:4; + uint8_t update:4; + uint8_t gs:2; + uint8_t code_hi:6; +#endif } serial; uint16_t msg_id; struct { +#ifdef OSMO_IS_LITTLE_ENDIAN uint8_t language:4; uint8_t group:4; +#else + uint8_t group:4; + uint8_t language:4; +#endif } dcs; struct { +#ifdef OSMO_IS_LITTLE_ENDIAN uint8_t total:4; uint8_t current:4; +#else + uint8_t current:4; + uint8_t total:4; +#endif } page; uint8_t data[0]; } __attribute__((packed)); @@ -33,12 +51,21 @@ struct gsm341_ms_message { /* Chapter 9.4.1.3 */ struct gsm341_etws_message { struct { +#ifdef OSMO_IS_LITTLE_ENDIAN uint8_t code_hi:4; uint8_t popup:1; uint8_t alert:1; uint8_t gs:2; uint8_t update:4; uint8_t code_lo:4; +#else + uint8_t code_lo:4; + uint8_t update:4; + uint8_t gs:2; + uint8_t alert:1; + uint8_t popup:1; + uint8_t code_hi:4; +#endif } serial; uint16_t msg_id; uint16_t warning_type; -- 2.6.2 -- - Harald Welte <laforge@gnumonks.org> http://laforge.gnumonks.org/ ============================================================================ "Privacy in residential applications is a desirable marketing option." (ETSI EN 300 175-7 Ch. A6)
Hi, Thanks! Apparently it doesn't work correctly, but I'm now trying with: #if OSMO_IS_LITTLE_ENDIAN == 1 instead, and I have big hopes. (OSMO_IS_LITTLE_ENDIAN is always defined, 1 for low-endian archs and 0 for big-endian archs.) Cheers, Ruben 2015-12-06 22:15 GMT+01:00 Harald Welte <laforge@gnumonks.org>:
Hi Ruben,
On Sun, Dec 06, 2015 at 04:53:09PM +0100, Ruben Undheim wrote:
While building the package for Debian, apparently there is a problem related to big-endian architectures.
While I still own several PPC machines, I haven't booted any of them in years, and don't have a build setup ready. Please try the patch below and report back if it works. If yes, we can merge it.
From 51ae645e220556bbeabce3ac57304639328e2164 Mon Sep 17 00:00:00 2001 From: Harald Welte <laforge@gnumonks.org> Date: Sun, 6 Dec 2015 22:12:43 +0100 Subject: [PATCH] untested fix for gsm_03_41.h and big-endian machines
Our gsm_03_41 structs use bit-fields, but don't do the usual little/big-endian jumping. --- include/osmocom/gsm/protocol/gsm_03_41.h | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)
diff --git a/include/osmocom/gsm/protocol/gsm_03_41.h b/include/osmocom/gsm/protocol/gsm_03_41.h index 0ece6cc..f007cc1 100644 --- a/include/osmocom/gsm/protocol/gsm_03_41.h +++ b/include/osmocom/gsm/protocol/gsm_03_41.h @@ -2,6 +2,7 @@
#include <stdint.h>
+#include <osmocom/core/endian.h> #include <osmocom/gsm/protocol/gsm_04_12.h>
/* GSM TS 03.41 definitions also TS 23.041*/ @@ -13,19 +14,36 @@ /* Chapter 9.3.2 */ struct gsm341_ms_message { struct { +#ifdef OSMO_IS_LITTLE_ENDIAN uint8_t code_hi:6; uint8_t gs:2; uint8_t update:4; uint8_t code_lo:4; +#else + uint8_t code_lo:4; + uint8_t update:4; + uint8_t gs:2; + uint8_t code_hi:6; +#endif } serial; uint16_t msg_id; struct { +#ifdef OSMO_IS_LITTLE_ENDIAN uint8_t language:4; uint8_t group:4; +#else + uint8_t group:4; + uint8_t language:4; +#endif } dcs; struct { +#ifdef OSMO_IS_LITTLE_ENDIAN uint8_t total:4; uint8_t current:4; +#else + uint8_t current:4; + uint8_t total:4; +#endif } page; uint8_t data[0]; } __attribute__((packed)); @@ -33,12 +51,21 @@ struct gsm341_ms_message { /* Chapter 9.4.1.3 */ struct gsm341_etws_message { struct { +#ifdef OSMO_IS_LITTLE_ENDIAN uint8_t code_hi:4; uint8_t popup:1; uint8_t alert:1; uint8_t gs:2; uint8_t update:4; uint8_t code_lo:4; +#else + uint8_t code_lo:4; + uint8_t update:4; + uint8_t gs:2; + uint8_t alert:1; + uint8_t popup:1; + uint8_t code_hi:4; +#endif } serial; uint16_t msg_id; uint16_t warning_type; -- 2.6.2
-- - Harald Welte <laforge@gnumonks.org> http://laforge.gnumonks.org/ ============================================================================ "Privacy in residential applications is a desirable marketing option." (ETSI EN 300 175-7 Ch. A6)
Hi, thanks, I encountered the same in Fedora on ppc machines. Unfortunately, the patch even with '#if OSMO_IS_LITTLE_ENDIAN == 1' doesn't seem to resolve the problem for me, the test is still failing. I am going to check this deeper thanks & regards Jaroslav ----- Original Message -----
Hi,
Thanks!
Apparently it doesn't work correctly, but I'm now trying with: #if OSMO_IS_LITTLE_ENDIAN == 1
instead, and I have big hopes.
(OSMO_IS_LITTLE_ENDIAN is always defined, 1 for low-endian archs and 0 for big-endian archs.)
Cheers, Ruben
2015-12-06 22:15 GMT+01:00 Harald Welte <laforge@gnumonks.org>:
Hi Ruben,
On Sun, Dec 06, 2015 at 04:53:09PM +0100, Ruben Undheim wrote:
While building the package for Debian, apparently there is a problem related to big-endian architectures.
While I still own several PPC machines, I haven't booted any of them in years, and don't have a build setup ready. Please try the patch below and report back if it works. If yes, we can merge it.
From 51ae645e220556bbeabce3ac57304639328e2164 Mon Sep 17 00:00:00 2001 From: Harald Welte <laforge@gnumonks.org> Date: Sun, 6 Dec 2015 22:12:43 +0100 Subject: [PATCH] untested fix for gsm_03_41.h and big-endian machines
Our gsm_03_41 structs use bit-fields, but don't do the usual little/big-endian jumping. --- include/osmocom/gsm/protocol/gsm_03_41.h | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)
diff --git a/include/osmocom/gsm/protocol/gsm_03_41.h b/include/osmocom/gsm/protocol/gsm_03_41.h index 0ece6cc..f007cc1 100644 --- a/include/osmocom/gsm/protocol/gsm_03_41.h +++ b/include/osmocom/gsm/protocol/gsm_03_41.h @@ -2,6 +2,7 @@
#include <stdint.h>
+#include <osmocom/core/endian.h> #include <osmocom/gsm/protocol/gsm_04_12.h>
/* GSM TS 03.41 definitions also TS 23.041*/ @@ -13,19 +14,36 @@ /* Chapter 9.3.2 */ struct gsm341_ms_message { struct { +#ifdef OSMO_IS_LITTLE_ENDIAN uint8_t code_hi:6; uint8_t gs:2; uint8_t update:4; uint8_t code_lo:4; +#else + uint8_t code_lo:4; + uint8_t update:4; + uint8_t gs:2; + uint8_t code_hi:6; +#endif } serial; uint16_t msg_id; struct { +#ifdef OSMO_IS_LITTLE_ENDIAN uint8_t language:4; uint8_t group:4; +#else + uint8_t group:4; + uint8_t language:4; +#endif } dcs; struct { +#ifdef OSMO_IS_LITTLE_ENDIAN uint8_t total:4; uint8_t current:4; +#else + uint8_t current:4; + uint8_t total:4; +#endif } page; uint8_t data[0]; } __attribute__((packed)); @@ -33,12 +51,21 @@ struct gsm341_ms_message { /* Chapter 9.4.1.3 */ struct gsm341_etws_message { struct { +#ifdef OSMO_IS_LITTLE_ENDIAN uint8_t code_hi:4; uint8_t popup:1; uint8_t alert:1; uint8_t gs:2; uint8_t update:4; uint8_t code_lo:4; +#else + uint8_t code_lo:4; + uint8_t update:4; + uint8_t gs:2; + uint8_t alert:1; + uint8_t popup:1; + uint8_t code_hi:4; +#endif } serial; uint16_t msg_id; uint16_t warning_type; -- 2.6.2
-- - Harald Welte <laforge@gnumonks.org> http://laforge.gnumonks.org/ ============================================================================ "Privacy in residential applications is a desirable marketing option." (ETSI EN 300 175-7 Ch. A6)
On Wed, Dec 09, 2015 at 03:30:25AM -0500, Jaroslav Skarvada wrote:
Hi,
thanks, I encountered the same in Fedora on ppc machines. Unfortunately, the patch even with '#if OSMO_IS_LITTLE_ENDIAN == 1'
Has the status changed with the patch? According to https://buildd.debian.org/status/fetch.php?pkg=libosmocore&arch=powerpc&ver=... from https://buildd.debian.org/status/package.php?p=libosmocore the test suite has succeeded on powerpc. Does that mean the problem is resolved? (Previously, the test smscb failed, now it says "smscb ok") If not, I could try it, too; asking my fellow geeky neighbor for a BE machine... As a side note, even though the above "#if" should work, to me it looks unusual to explicitly write "== 1". It suffices to remove the "def" from "#ifdef": #if OSMO_IS_LITTLE_ENDIAN ... #endif which is skipped when OSMO_IS_LITTLE_ENDIAN == 0 and is run when OSMO_IS_LITTLE_ENDIAN != 0, more like your typical int-as-boolean logic, and would also work as expected if OSMO_IS_LITTLE_ENDIAN were 2 ;) ~Neels
----- Original Message -----
On Wed, Dec 09, 2015 at 03:30:25AM -0500, Jaroslav Skarvada wrote:
Hi,
thanks, I encountered the same in Fedora on ppc machines. Unfortunately, the patch even with '#if OSMO_IS_LITTLE_ENDIAN == 1'
Has the status changed with the patch? According to https://buildd.debian.org/status/fetch.php?pkg=libosmocore&arch=powerpc&ver=... from https://buildd.debian.org/status/package.php?p=libosmocore the test suite has succeeded on powerpc. Does that mean the problem is resolved? (Previously, the test smscb failed, now it says "smscb ok")
The output of the test changed, so it differs now only in one line on ppc64, but it still means failure: --- ./smscb_test.fail 2015-12-09 11:29:22.143808034 -0500 +++ ./smscb_test.ok 2015-12-08 11:10:37.480795985 -0500 @@ -1,4 +1,4 @@ -(srl) GS: 0 MSG_CODE: 260 UPDATE: 0 +(srl) GS: 1 MSG_CODE: 1 UPDATE: 0 (msg) msg_id: 1293 (dcs) group: 1 language: 0 (pge) page total: 1 current: 1 Patch used: http://fpaste.org/299159/49679080/ Fedora build system log: https://kojipkgs.fedoraproject.org//work/tasks/6119/12126119/build.log Fedora secondary arch bug: https://bugzilla.redhat.com/show_bug.cgi?id=1289940 thanks & regards Jaroslav
Two more failures seen on https://buildd.debian.org/status/package.php?p=libosmocore For FreeBSD, both i386 and amd64: macaddr.c: In function 'osmo_get_macaddr': macaddr.c:106:17: error: 'SIOCGIFHWADDR' undeclared (first use in this function) rc = ioctl(fd, SIOCGIFHWADDR, &ifr); ^ The code in question has an explicit FreeBSD alternative which seems to not have been entered as intended, libosmocore/src/macaddr.c line 50: " #if defined(__FreeBSD__) || defined(__APPLE__) [...] #else [...] <-- error is in this block #endif " The same #if appears in numerous places in the osmo code base, so it seems to be correct ... ? Or all of them are wrong?? Another error on sparc64: three tests fail with a "Bus error" message, which I don't understand: " ## ------------------------ ## ## Summary of the failures. ## ## ------------------------ ## Failed tests: libosmocore 0.9.0 test suite test groups: NUM: FILE-NAME:LINE TEST-GROUP-NAME KEYWORDS 3: testsuite.at:18 bits bits 19: testsuite.at:121 gprs-bssgp gprs-bssgp 20: testsuite.at:127 gprs-ns gprs-ns ## ---------------------- ## ## Detailed failed tests. ## ## ---------------------- ## # -*- compilation -*- 3. testsuite.at:18: testing bits ... ./testsuite.at:21: $abs_top_builddir/tests/bits/bitrev_test --- /dev/null 2011-03-04 11:53:52.000000000 +0000 +++ /«PKGBUILDDIR»/tests/testsuite.dir/at-groups/3/stderr 2015-12-09 08:07:05.318729809 +0000 @@ -0,0 +1 @@ +/«PKGBUILDDIR»/tests/testsuite.dir/at-groups/3/test-source: line 25: 14139 Bus error $abs_top_builddir/tests/bits/bitrev_test --- expout 2015-12-09 08:07:05.018702966 +0000 +++ /«PKGBUILDDIR»/tests/testsuite.dir/at-groups/3/stdout 2015-12-09 08:07:05.022703325 +0000 @@ -1,55 +0,0 @@ -INORDER: 01 02 04 08 10 20 40 80 -REVERSED: 80 40 20 10 08 04 02 01 - -INORDER: 02 04 08 10 20 40 80 -REVERSED: 40 20 10 08 04 02 01 [...] " (many expected output lines missing) Could it be a sporadic HW failure on the machine itself, or something? The "bitrev_test" doesn't really look like something that would fail because of a bus. ~Neels -- - Neels Hofmeyr <nhofmeyr@sysmocom.de> http://www.sysmocom.de/ ======================================================================= * sysmocom - systems for mobile communications GmbH * Alt-Moabit 93 * 10559 Berlin, Germany * Sitz / Registered office: Berlin, HRB 134158 B * Geschäftsführer / Managing Directors: Holger Freyther, Harald Welte
Hi, In the end, I ended up with the following patch that works: diff --git a/include/osmocom/gsm/protocol/gsm_03_41.h b/include/osmocom/gsm/protocol/gsm_03_41.h index 0ece6cc..40051cd 100644 --- a/include/osmocom/gsm/protocol/gsm_03_41.h +++ b/include/osmocom/gsm/protocol/gsm_03_41.h @@ -2,8 +2,13 @@ #include <stdint.h> +#include <osmocom/core/endian.h> #include <osmocom/gsm/protocol/gsm_04_12.h> +#ifndef OSMO_IS_LITTLE_ENDIAN + #define OSMO_IS_LITTLE_ENDIAN 0 +#endif + /* GSM TS 03.41 definitions also TS 23.041*/ #define GSM341_MAX_PAYLOAD (GSM412_MSG_LEN-sizeof(struct gsm341_ms_message)) @@ -13,19 +18,36 @@ /* Chapter 9.3.2 */ struct gsm341_ms_message { struct { +#if OSMO_IS_LITTLE_ENDIAN == 1 uint8_t code_hi:6; uint8_t gs:2; uint8_t update:4; uint8_t code_lo:4; +#else + uint8_t gs:2; + uint8_t code_hi:6; + uint8_t code_lo:4; + uint8_t update:4; +#endif } serial; uint16_t msg_id; struct { +#if OSMO_IS_LITTLE_ENDIAN == 1 uint8_t language:4; uint8_t group:4; +#else + uint8_t group:4; + uint8_t language:4; +#endif } dcs; struct { +#if OSMO_IS_LITTLE_ENDIAN == 1 uint8_t total:4; uint8_t current:4; +#else + uint8_t current:4; + uint8_t total:4; +#endif } page; uint8_t data[0]; } __attribute__((packed)); @@ -33,12 +55,21 @@ struct gsm341_ms_message { /* Chapter 9.4.1.3 */ struct gsm341_etws_message { struct { +#if OSMO_IS_LITTLE_ENDIAN == 1 uint8_t code_hi:4; uint8_t popup:1; uint8_t alert:1; uint8_t gs:2; uint8_t update:4; uint8_t code_lo:4; +#else + uint8_t gs:2; + uint8_t alert:1; + uint8_t popup:1; + uint8_t code_hi:4; + uint8_t code_lo:4; + uint8_t update:4; +#endif } serial; uint16_t msg_id; uint16_t warning_type; Note that the order of the fields is a bit different than your proposal. I also, interestingly enough, found this: http://lists.osmocom.org/pipermail/baseband-devel/2015-February/000021.html It's from Februrary, and already a suggestion for a patch for this problem. I don't think however that patch will work with other big-endian architectures. Cheers, Ruben 2015-12-07 22:16 GMT+01:00 Ruben Undheim <ruben.undheim@gmail.com>:
Hi,
Thanks!
Apparently it doesn't work correctly, but I'm now trying with: #if OSMO_IS_LITTLE_ENDIAN == 1
instead, and I have big hopes.
(OSMO_IS_LITTLE_ENDIAN is always defined, 1 for low-endian archs and 0 for big-endian archs.)
Cheers, Ruben
2015-12-06 22:15 GMT+01:00 Harald Welte <laforge@gnumonks.org>:
Hi Ruben,
On Sun, Dec 06, 2015 at 04:53:09PM +0100, Ruben Undheim wrote:
While building the package for Debian, apparently there is a problem related to big-endian architectures.
While I still own several PPC machines, I haven't booted any of them in years, and don't have a build setup ready. Please try the patch below and report back if it works. If yes, we can merge it.
From 51ae645e220556bbeabce3ac57304639328e2164 Mon Sep 17 00:00:00 2001 From: Harald Welte <laforge@gnumonks.org> Date: Sun, 6 Dec 2015 22:12:43 +0100 Subject: [PATCH] untested fix for gsm_03_41.h and big-endian machines
Our gsm_03_41 structs use bit-fields, but don't do the usual little/big-endian jumping. --- include/osmocom/gsm/protocol/gsm_03_41.h | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)
diff --git a/include/osmocom/gsm/protocol/gsm_03_41.h b/include/osmocom/gsm/protocol/gsm_03_41.h index 0ece6cc..f007cc1 100644 --- a/include/osmocom/gsm/protocol/gsm_03_41.h +++ b/include/osmocom/gsm/protocol/gsm_03_41.h @@ -2,6 +2,7 @@
#include <stdint.h>
+#include <osmocom/core/endian.h> #include <osmocom/gsm/protocol/gsm_04_12.h>
/* GSM TS 03.41 definitions also TS 23.041*/ @@ -13,19 +14,36 @@ /* Chapter 9.3.2 */ struct gsm341_ms_message { struct { +#ifdef OSMO_IS_LITTLE_ENDIAN uint8_t code_hi:6; uint8_t gs:2; uint8_t update:4; uint8_t code_lo:4; +#else + uint8_t code_lo:4; + uint8_t update:4; + uint8_t gs:2; + uint8_t code_hi:6; +#endif } serial; uint16_t msg_id; struct { +#ifdef OSMO_IS_LITTLE_ENDIAN uint8_t language:4; uint8_t group:4; +#else + uint8_t group:4; + uint8_t language:4; +#endif } dcs; struct { +#ifdef OSMO_IS_LITTLE_ENDIAN uint8_t total:4; uint8_t current:4; +#else + uint8_t current:4; + uint8_t total:4; +#endif } page; uint8_t data[0]; } __attribute__((packed)); @@ -33,12 +51,21 @@ struct gsm341_ms_message { /* Chapter 9.4.1.3 */ struct gsm341_etws_message { struct { +#ifdef OSMO_IS_LITTLE_ENDIAN uint8_t code_hi:4; uint8_t popup:1; uint8_t alert:1; uint8_t gs:2; uint8_t update:4; uint8_t code_lo:4; +#else + uint8_t code_lo:4; + uint8_t update:4; + uint8_t gs:2; + uint8_t alert:1; + uint8_t popup:1; + uint8_t code_hi:4; +#endif } serial; uint16_t msg_id; uint16_t warning_type; -- 2.6.2
-- - Harald Welte <laforge@gnumonks.org> http://laforge.gnumonks.org/ ============================================================================ "Privacy in residential applications is a desirable marketing option." (ETSI EN 300 175-7 Ch. A6)
Thanks, this works for me, but I still don't understand why, the ordering of bitfields looks quite strange to me :) Jaroslav ----- Original Message -----
Hi,
In the end, I ended up with the following patch that works:
diff --git a/include/osmocom/gsm/protocol/gsm_03_41.h b/include/osmocom/gsm/protocol/gsm_03_41.h index 0ece6cc..40051cd 100644 --- a/include/osmocom/gsm/protocol/gsm_03_41.h +++ b/include/osmocom/gsm/protocol/gsm_03_41.h @@ -2,8 +2,13 @@
#include <stdint.h>
+#include <osmocom/core/endian.h> #include <osmocom/gsm/protocol/gsm_04_12.h>
+#ifndef OSMO_IS_LITTLE_ENDIAN + #define OSMO_IS_LITTLE_ENDIAN 0 +#endif + /* GSM TS 03.41 definitions also TS 23.041*/
#define GSM341_MAX_PAYLOAD (GSM412_MSG_LEN-sizeof(struct gsm341_ms_message)) @@ -13,19 +18,36 @@ /* Chapter 9.3.2 */ struct gsm341_ms_message { struct { +#if OSMO_IS_LITTLE_ENDIAN == 1 uint8_t code_hi:6; uint8_t gs:2; uint8_t update:4; uint8_t code_lo:4; +#else + uint8_t gs:2; + uint8_t code_hi:6; + uint8_t code_lo:4; + uint8_t update:4; +#endif } serial; uint16_t msg_id; struct { +#if OSMO_IS_LITTLE_ENDIAN == 1 uint8_t language:4; uint8_t group:4; +#else + uint8_t group:4; + uint8_t language:4; +#endif } dcs; struct { +#if OSMO_IS_LITTLE_ENDIAN == 1 uint8_t total:4; uint8_t current:4; +#else + uint8_t current:4; + uint8_t total:4; +#endif } page; uint8_t data[0]; } __attribute__((packed)); @@ -33,12 +55,21 @@ struct gsm341_ms_message { /* Chapter 9.4.1.3 */ struct gsm341_etws_message { struct { +#if OSMO_IS_LITTLE_ENDIAN == 1 uint8_t code_hi:4; uint8_t popup:1; uint8_t alert:1; uint8_t gs:2; uint8_t update:4; uint8_t code_lo:4; +#else + uint8_t gs:2; + uint8_t alert:1; + uint8_t popup:1; + uint8_t code_hi:4; + uint8_t code_lo:4; + uint8_t update:4; +#endif } serial; uint16_t msg_id; uint16_t warning_type;
Note that the order of the fields is a bit different than your proposal.
I also, interestingly enough, found this: http://lists.osmocom.org/pipermail/baseband-devel/2015-February/000021.html
It's from Februrary, and already a suggestion for a patch for this problem. I don't think however that patch will work with other big-endian architectures.
Cheers, Ruben
2015-12-07 22:16 GMT+01:00 Ruben Undheim <ruben.undheim@gmail.com>:
Hi,
Thanks!
Apparently it doesn't work correctly, but I'm now trying with: #if OSMO_IS_LITTLE_ENDIAN == 1
instead, and I have big hopes.
(OSMO_IS_LITTLE_ENDIAN is always defined, 1 for low-endian archs and 0 for big-endian archs.)
Cheers, Ruben
2015-12-06 22:15 GMT+01:00 Harald Welte <laforge@gnumonks.org>:
Hi Ruben,
On Sun, Dec 06, 2015 at 04:53:09PM +0100, Ruben Undheim wrote:
While building the package for Debian, apparently there is a problem related to big-endian architectures.
While I still own several PPC machines, I haven't booted any of them in years, and don't have a build setup ready. Please try the patch below and report back if it works. If yes, we can merge it.
From 51ae645e220556bbeabce3ac57304639328e2164 Mon Sep 17 00:00:00 2001 From: Harald Welte <laforge@gnumonks.org> Date: Sun, 6 Dec 2015 22:12:43 +0100 Subject: [PATCH] untested fix for gsm_03_41.h and big-endian machines
Our gsm_03_41 structs use bit-fields, but don't do the usual little/big-endian jumping. --- include/osmocom/gsm/protocol/gsm_03_41.h | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)
diff --git a/include/osmocom/gsm/protocol/gsm_03_41.h b/include/osmocom/gsm/protocol/gsm_03_41.h index 0ece6cc..f007cc1 100644 --- a/include/osmocom/gsm/protocol/gsm_03_41.h +++ b/include/osmocom/gsm/protocol/gsm_03_41.h @@ -2,6 +2,7 @@
#include <stdint.h>
+#include <osmocom/core/endian.h> #include <osmocom/gsm/protocol/gsm_04_12.h>
/* GSM TS 03.41 definitions also TS 23.041*/ @@ -13,19 +14,36 @@ /* Chapter 9.3.2 */ struct gsm341_ms_message { struct { +#ifdef OSMO_IS_LITTLE_ENDIAN uint8_t code_hi:6; uint8_t gs:2; uint8_t update:4; uint8_t code_lo:4; +#else + uint8_t code_lo:4; + uint8_t update:4; + uint8_t gs:2; + uint8_t code_hi:6; +#endif } serial; uint16_t msg_id; struct { +#ifdef OSMO_IS_LITTLE_ENDIAN uint8_t language:4; uint8_t group:4; +#else + uint8_t group:4; + uint8_t language:4; +#endif } dcs; struct { +#ifdef OSMO_IS_LITTLE_ENDIAN uint8_t total:4; uint8_t current:4; +#else + uint8_t current:4; + uint8_t total:4; +#endif } page; uint8_t data[0]; } __attribute__((packed)); @@ -33,12 +51,21 @@ struct gsm341_ms_message { /* Chapter 9.4.1.3 */ struct gsm341_etws_message { struct { +#ifdef OSMO_IS_LITTLE_ENDIAN uint8_t code_hi:4; uint8_t popup:1; uint8_t alert:1; uint8_t gs:2; uint8_t update:4; uint8_t code_lo:4; +#else + uint8_t code_lo:4; + uint8_t update:4; + uint8_t gs:2; + uint8_t alert:1; + uint8_t popup:1; + uint8_t code_hi:4; +#endif } serial; uint16_t msg_id; uint16_t warning_type; -- 2.6.2
-- - Harald Welte <laforge@gnumonks.org> http://laforge.gnumonks.org/ ============================================================================ "Privacy in residential applications is a desirable marketing option." (ETSI EN 300 175-7 Ch. A6)
I am afraid using bitfields is not portable and bits ordering may differ between architectures and compilers (it's not only about endianess, IMHO it's not defined by the standard). I think correct portable implementation should use macros and bitmasks, maybe something to consider for future updates thanks & regards Jaroslav ----- Original Message -----
Thanks,
this works for me, but I still don't understand why, the ordering of bitfields looks quite strange to me :)
Jaroslav
----- Original Message -----
Hi,
In the end, I ended up with the following patch that works:
diff --git a/include/osmocom/gsm/protocol/gsm_03_41.h b/include/osmocom/gsm/protocol/gsm_03_41.h index 0ece6cc..40051cd 100644 --- a/include/osmocom/gsm/protocol/gsm_03_41.h +++ b/include/osmocom/gsm/protocol/gsm_03_41.h @@ -2,8 +2,13 @@
#include <stdint.h>
+#include <osmocom/core/endian.h> #include <osmocom/gsm/protocol/gsm_04_12.h>
+#ifndef OSMO_IS_LITTLE_ENDIAN + #define OSMO_IS_LITTLE_ENDIAN 0 +#endif + /* GSM TS 03.41 definitions also TS 23.041*/
#define GSM341_MAX_PAYLOAD (GSM412_MSG_LEN-sizeof(struct gsm341_ms_message)) @@ -13,19 +18,36 @@ /* Chapter 9.3.2 */ struct gsm341_ms_message { struct { +#if OSMO_IS_LITTLE_ENDIAN == 1 uint8_t code_hi:6; uint8_t gs:2; uint8_t update:4; uint8_t code_lo:4; +#else + uint8_t gs:2; + uint8_t code_hi:6; + uint8_t code_lo:4; + uint8_t update:4; +#endif } serial; uint16_t msg_id; struct { +#if OSMO_IS_LITTLE_ENDIAN == 1 uint8_t language:4; uint8_t group:4; +#else + uint8_t group:4; + uint8_t language:4; +#endif } dcs; struct { +#if OSMO_IS_LITTLE_ENDIAN == 1 uint8_t total:4; uint8_t current:4; +#else + uint8_t current:4; + uint8_t total:4; +#endif } page; uint8_t data[0]; } __attribute__((packed)); @@ -33,12 +55,21 @@ struct gsm341_ms_message { /* Chapter 9.4.1.3 */ struct gsm341_etws_message { struct { +#if OSMO_IS_LITTLE_ENDIAN == 1 uint8_t code_hi:4; uint8_t popup:1; uint8_t alert:1; uint8_t gs:2; uint8_t update:4; uint8_t code_lo:4; +#else + uint8_t gs:2; + uint8_t alert:1; + uint8_t popup:1; + uint8_t code_hi:4; + uint8_t code_lo:4; + uint8_t update:4; +#endif } serial; uint16_t msg_id; uint16_t warning_type;
Note that the order of the fields is a bit different than your proposal.
I also, interestingly enough, found this: http://lists.osmocom.org/pipermail/baseband-devel/2015-February/000021.html
It's from Februrary, and already a suggestion for a patch for this problem. I don't think however that patch will work with other big-endian architectures.
Cheers, Ruben
2015-12-07 22:16 GMT+01:00 Ruben Undheim <ruben.undheim@gmail.com>:
Hi,
Thanks!
Apparently it doesn't work correctly, but I'm now trying with: #if OSMO_IS_LITTLE_ENDIAN == 1
instead, and I have big hopes.
(OSMO_IS_LITTLE_ENDIAN is always defined, 1 for low-endian archs and 0 for big-endian archs.)
Cheers, Ruben
2015-12-06 22:15 GMT+01:00 Harald Welte <laforge@gnumonks.org>:
Hi Ruben,
On Sun, Dec 06, 2015 at 04:53:09PM +0100, Ruben Undheim wrote:
While building the package for Debian, apparently there is a problem related to big-endian architectures.
While I still own several PPC machines, I haven't booted any of them in years, and don't have a build setup ready. Please try the patch below and report back if it works. If yes, we can merge it.
From 51ae645e220556bbeabce3ac57304639328e2164 Mon Sep 17 00:00:00 2001 From: Harald Welte <laforge@gnumonks.org> Date: Sun, 6 Dec 2015 22:12:43 +0100 Subject: [PATCH] untested fix for gsm_03_41.h and big-endian machines
Our gsm_03_41 structs use bit-fields, but don't do the usual little/big-endian jumping. --- include/osmocom/gsm/protocol/gsm_03_41.h | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)
diff --git a/include/osmocom/gsm/protocol/gsm_03_41.h b/include/osmocom/gsm/protocol/gsm_03_41.h index 0ece6cc..f007cc1 100644 --- a/include/osmocom/gsm/protocol/gsm_03_41.h +++ b/include/osmocom/gsm/protocol/gsm_03_41.h @@ -2,6 +2,7 @@
#include <stdint.h>
+#include <osmocom/core/endian.h> #include <osmocom/gsm/protocol/gsm_04_12.h>
/* GSM TS 03.41 definitions also TS 23.041*/ @@ -13,19 +14,36 @@ /* Chapter 9.3.2 */ struct gsm341_ms_message { struct { +#ifdef OSMO_IS_LITTLE_ENDIAN uint8_t code_hi:6; uint8_t gs:2; uint8_t update:4; uint8_t code_lo:4; +#else + uint8_t code_lo:4; + uint8_t update:4; + uint8_t gs:2; + uint8_t code_hi:6; +#endif } serial; uint16_t msg_id; struct { +#ifdef OSMO_IS_LITTLE_ENDIAN uint8_t language:4; uint8_t group:4; +#else + uint8_t group:4; + uint8_t language:4; +#endif } dcs; struct { +#ifdef OSMO_IS_LITTLE_ENDIAN uint8_t total:4; uint8_t current:4; +#else + uint8_t current:4; + uint8_t total:4; +#endif } page; uint8_t data[0]; } __attribute__((packed)); @@ -33,12 +51,21 @@ struct gsm341_ms_message { /* Chapter 9.4.1.3 */ struct gsm341_etws_message { struct { +#ifdef OSMO_IS_LITTLE_ENDIAN uint8_t code_hi:4; uint8_t popup:1; uint8_t alert:1; uint8_t gs:2; uint8_t update:4; uint8_t code_lo:4; +#else + uint8_t code_lo:4; + uint8_t update:4; + uint8_t gs:2; + uint8_t alert:1; + uint8_t popup:1; + uint8_t code_hi:4; +#endif } serial; uint16_t msg_id; uint16_t warning_type; -- 2.6.2
-- - Harald Welte <laforge@gnumonks.org> http://laforge.gnumonks.org/ ============================================================================ "Privacy in residential applications is a desirable marketing option." (ETSI EN 300 175-7 Ch. A6)
On Wed, Dec 09, 2015 at 01:26:41PM -0500, Jaroslav Skarvada wrote:
I think correct portable implementation should use macros and bitmasks, maybe something to consider for future updates
IP headers and TCP headers are always defined as bit filds, look at the netinet/ip.h and netinet/tcp.h. We're in the best of traditions here ;) -- - Harald Welte <laforge@gnumonks.org> http://laforge.gnumonks.org/ ============================================================================ "Privacy in residential applications is a desirable marketing option." (ETSI EN 300 175-7 Ch. A6)
On Thu, Dec 10, 2015 at 09:51:14AM +0100, Harald Welte wrote:
On Wed, Dec 09, 2015 at 01:26:41PM -0500, Jaroslav Skarvada wrote:
I think correct portable implementation should use macros and bitmasks, maybe something to consider for future updates
IP headers and TCP headers are always defined as bit filds, look at the netinet/ip.h and netinet/tcp.h. We're in the best of traditions here ;)
...not that it would win a code beauty contest though :) Maybe a consolation prize for the most widely used redundancy... ~Neels
On Wed, Dec 09, 2015 at 12:41:33PM -0500, Jaroslav Skarvada wrote:
this works for me, but I still don't understand why, the ordering of bitfields looks quite strange to me :)
The reversal needs to be done byte-wise. So in this example:
+#if OSMO_IS_LITTLE_ENDIAN == 1 uint8_t code_hi:6; uint8_t gs:2; uint8_t update:4; uint8_t code_lo:4;
The first byte is code_hi and gs, reverse those:
+#else + uint8_t gs:2; + uint8_t code_hi:6;
Next byte is update and code_lo, reversed:
+ uint8_t code_lo:4; + uint8_t update:4; +#endif
I hope that helps :) ~Neels
+#if OSMO_IS_LITTLE_ENDIAN == 1 uint8_t code_hi:4; uint8_t popup:1; uint8_t alert:1; uint8_t gs:2; uint8_t update:4; uint8_t code_lo:4; +#else + uint8_t gs:2; + uint8_t alert:1; + uint8_t popup:1; + uint8_t code_hi:4; + uint8_t code_lo:4; + uint8_t update:4; +#endif
-- - Neels Hofmeyr <nhofmeyr@sysmocom.de> http://www.sysmocom.de/ ======================================================================= * sysmocom - systems for mobile communications GmbH * Alt-Moabit 93 * 10559 Berlin, Germany * Sitz / Registered office: Berlin, HRB 134158 B * Geschäftsführer / Managing Directors: Holger Freyther, Harald Welte
Hi Ruben, On Wed, Dec 09, 2015 at 06:20:58PM +0100, Ruben Undheim wrote:
In the end, I ended up with the following patch that works:
Thansk. Your e-mail client mangled the whitespaces/tabs, but I fixed it up manually. Now pushed to master. Regards, Harald -- - Harald Welte <laforge@gnumonks.org> http://laforge.gnumonks.org/ ============================================================================ "Privacy in residential applications is a desirable marketing option." (ETSI EN 300 175-7 Ch. A6)
On 09 Dec 2015, at 18:20, Ruben Undheim <ruben.undheim@gmail.com> wrote:
Hi,
+#ifndef OSMO_IS_LITTLE_ENDIAN + #define OSMO_IS_LITTLE_ENDIAN 0 +#endif
I am surprised by this hunk. Our endian.h should define both macros. I think I used the vim search to search for a typo in one of the branches (BSD vs. Linux, LE vs. BE) but it looks good. Do you remember why this was needed? holger
participants (5)
-
Harald Welte -
Holger Freyther -
Jaroslav Skarvada -
Neels Hofmeyr -
Ruben Undheim