search

janestreet / bin_prot

Binary protocol generator
MIT License
73 stars 21 forks source link

Compilation on Linux ARMv7L (32 bit) #4

Closed smondet closed 10 years ago

smondet commented 11 years ago

I used to be able to compile most of the Core suite on my Chromebook.

Now bin_prot fails with:

+ ocamlfind ocamlc -c lib/blit_stubs.c
lib/blit_stubs.c: In function ‘bin_prot_blit_float_array_buf_stub’:
lib/blit_stubs.c:75:22: error: lvalue required as unary ‘&’ operand
   char *arr = (char*)&Double_field(v_arr, Long_val(v_src_pos));
                      ^
lib/blit_stubs.c: In function ‘bin_prot_blit_buf_float_array_stub’:
lib/blit_stubs.c:85:22: error: lvalue required as unary ‘&’ operand
   char *arr = (char*)&Double_field(v_arr, Long_val(v_dst_pos));
                      ^
Command exited with code 2.

Some more info about the system:

 $ uname -a
Linux localhost 3.4.0 #1 SMP Tue Oct 8 23:22:11 PDT 2013 armv7l GNU/Linux
 $ cat /etc/debian_version 
jessie/sid
# Sys.word_size;;
- : int = 32
avsm commented 10 years ago

Confirmed this regression on ocaml/opam-repository#1346

# + ocamlfind ocamlc -c lib/blit_stubs.c
# lib/blit_stubs.c: In function 'bin_prot_blit_float_array_buf_stub':
# lib/blit_stubs.c:75:22: error: lvalue required as unary '&' operand
# lib/blit_stubs.c: In function 'bin_prot_blit_buf_float_array_stub':
# lib/blit_stubs.c:85:22: error: lvalue required as unary '&' operand
# Command exited with code 2.
$ uname -a
Linux quark 3.2.7 #1 PREEMPT Tue Feb 21 02:29:31 MST 2012 armv5tel GNU/Linux
ghost commented 10 years ago

Double_field(_, _) is supposed to be a lvalue, so I'm not sure how to fix this...

ghost commented 10 years ago

Can you try to replace (char*)&Double_field(v_arr, Long_val(v_dst_pos)) by (char*)v_arr + Long_val(v_dst_pos) * sizeof(double) and see if it works? I don't have an ARM machine at hand so I can't test this.

smondet commented 10 years ago

It compiles and the tests seem OK:

 $ make test
./setup-dev.exe -build 
W: Field XOCamlbuildLibraries is set but matching plugin OCamlbuild is not enabled.
W: Field XOCamlbuildLibraries is set but matching plugin OCamlbuild is not enabled.
W: No replace section found in file Makefile
W: Cannot find source file matching module 'bin_prot' in library bin_prot
Finished, 0 targets (0 cached) in 00:00:00.
Finished, 75 targets (75 cached) in 00:00:00.
./setup-dev.exe -test 
W: Field XOCamlbuildLibraries is set but matching plugin OCamlbuild is not enabled.
W: Field XOCamlbuildLibraries is set but matching plugin OCamlbuild is not enabled.
W: No replace section found in file Makefile
...................................
Ran: 35 tests in: 0.07 seconds.
OK

msgs: 100000  msg length: 461
write time: 4.239s  write rate:  23591.19 msgs/s  write throughput: 10.37 MB/s
 read time: 5.075s   read rate:  19705.90 msgs/s   read throughput: 8.66 MB/s

With 

 $ git diff
diff --git a/lib/blit_stubs.c b/lib/blit_stubs.c
index e67d9bc..59a6e7d 100644
--- a/lib/blit_stubs.c
+++ b/lib/blit_stubs.c
@@ -72,7 +72,8 @@ CAMLprim value bin_prot_blit_buf_stub(
 CAMLprim value bin_prot_blit_float_array_buf_stub(
   value v_src_pos, value v_arr, value v_dst_pos, value v_buf, value v_len)
 {
-  char *arr = (char*)&Double_field(v_arr, Long_val(v_src_pos));
+  /* char *arr = (char*)&Double_field(v_arr, Long_val(v_src_pos)); */
+  char *arr = (char*)v_arr + Long_val(v_src_pos) * sizeof(double);
   char *buf = get_buf(v_buf, v_dst_pos);
   memcpy(buf, arr, (size_t) (Long_val(v_len) * sizeof(double)));
   return Val_unit;
@@ -82,7 +83,8 @@ CAMLprim value bin_prot_blit_buf_float_array_stub(
   value v_src_pos, value v_buf, value v_dst_pos, value v_arr, value v_len)
 {
   char *buf = get_buf(v_buf, v_src_pos);
-  char *arr = (char*)&Double_field(v_arr, Long_val(v_dst_pos));
+  /* char *arr = (char*)&Double_field(v_arr, Long_val(v_dst_pos)); */
+  char *arr = (char*)v_arr + Long_val(v_dst_pos) * sizeof(double);
   memcpy(arr, buf, (size_t) (Long_val(v_len) * sizeof(double)));
   return Val_unit;
 }
ghost commented 10 years ago

Ok, I'll make the change then.

ghost commented 10 years ago

BTW, there might be similar issues in core_kernel, core or other libraries.

avsm commented 10 years ago

I don't entirely understand the fix -- is Double_val not an lvalue on ARM for some reason?

Jeremie: we could apply the patch conditionally on ARM in the OPAM repo on the existing 109.60 release, and I can do a bulk build on ARM with it applied. This would also unblock Irminsule building on ARM.

On 24 Jan 2014, at 09:35, Jérémie Dimino notifications@github.com wrote:

BTW, there might be similar issues in core_kernel, core or other libraries.

— Reply to this email directly or view it on GitHub.

ghost commented 10 years ago

I don't entirely understand the fix -- is Double_val not an lvalue on ARM for some reason?

It seems so, but I'm not sure why. it might be that one of the cast in Double_field causes a conversion...

Jeremie: we could apply the patch conditionally on ARM in the OPAM repo on the existing 109.60 release, and I can do a bulk build on ARM with it applied. This would also unblock Irminsule building on ARM.

Sure. Can you do it? I'm not sure what the test for arch=ARM is.

mshinwell commented 10 years ago

Double_val (and hence Double_field) are not always lvalues; it depends on the target's alignment constraints.

Anil, do we have a built compiler tree for this architecture to hand? I'd like to see the output of the configure script.

avsm commented 10 years ago

I'll do this when I get to the office shortly...

On 24 Jan 2014, at 09:58, mshinwell notifications@github.com wrote:

Double_val (and hence Double_field) are not always lvalues; it depends on the target's alignment constraints.

Anil, do we have a built compiler tree for this architecture to hand? I'd like to see the output of the configure script.

— Reply to this email directly or view it on GitHub.

avsm commented 10 years ago 
Linux quark 3.2.7 #1 PREEMPT Tue Feb 21 02:29:31 MST 2012 armv5tel GNU/Linux

m.h:

#undef ARCH_SIXTYFOUR
#define SIZEOF_INT 4
#define SIZEOF_LONG 4
#define SIZEOF_PTR 4
#define SIZEOF_SHORT 2
#define ARCH_INT64_TYPE long long
#define ARCH_UINT64_TYPE unsigned long long
#define ARCH_INT64_PRINTF_FORMAT "ll"
#undef ARCH_BIG_ENDIAN
#define ARCH_ALIGN_DOUBLE
#define ARCH_ALIGN_INT64
#undef NONSTANDARD_DIV_MOD
#define ASM_CFI_SUPPORTED

s.h:

#define OCAML_OS_TYPE "Unix"
#define OCAML_STDLIB_DIR "/home/avsm/.opam/4.01.0/lib/ocaml"
#define POSIX_SIGNALS
#define HAS_C99_FLOAT_OPS
#define HAS_GETRUSAGE
#define HAS_TIMES
#define HAS_TERMCAP
#define HAS_SOCKETS
#define HAS_SOCKLEN_T
#define HAS_INET_ATON
#define HAS_IPV6
#define HAS_UNISTD
#define HAS_OFF_T
#define HAS_DIRENT
#define HAS_REWINDDIR
#define HAS_LOCKF
#define HAS_MKFIFO
#define HAS_GETCWD
#define HAS_GETWD
#define HAS_GETPRIORITY
#define HAS_UTIME
#define HAS_UTIMES
#define HAS_DUP2
#define HAS_FCHMOD
#define HAS_TRUNCATE
#define HAS_SYS_SELECT_H
#define HAS_SELECT
#define HAS_SYMLINK
#define HAS_WAITPID
#define HAS_WAIT4
#define HAS_GETGROUPS
#define HAS_SETGROUPS
#define HAS_INITGROUPS
#define HAS_TERMIOS
#define HAS_ASYNC_IO
#define HAS_SETITIMER
#define HAS_GETHOSTNAME
#define HAS_UNAME
#define HAS_GETTIMEOFDAY
#define HAS_MKTIME
#define HAS_SETSID
#define HAS_PUTENV
#define HAS_LOCALE
#define SUPPORT_DYNAMIC_LINKING
#define HAS_MMAP
#define HAS_PWRITE
#define HAS_GETHOSTBYNAME_R 6
#define HAS_GETHOSTBYADDR_R 8
#define HAS_SIGWAIT
#define HAS_LIBBFD
mshinwell commented 10 years ago

Yeah, I think it's decided that doubles can only be read/written from 64-bit aligned addresses, so it goes via a temporary (see byterun/floats.c:caml_Double_val / caml_Store_double_val) when accessing fields of values.

I think we should just fix our code not to assume that Double_val is an lvalue.

avsm commented 10 years ago

Candidate fix in ocaml/opam-repository#1615 ; testing on the ARM now.

avsm commented 10 years ago

Merged into OPAM trunk