Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unix.LargeFile.ftruncate fd 2^32 truncates to 0 on armv7 #8841

Closed
rwmjones opened this issue Jul 29, 2019 · 10 comments
Closed

Unix.LargeFile.ftruncate fd 2^32 truncates to 0 on armv7 #8841

rwmjones opened this issue Jul 29, 2019 · 10 comments

Comments

@rwmjones
Copy link
Contributor

@rwmjones rwmjones commented Jul 29, 2019

The following test program should obviously truncate fd 1 (stdout) to 2^32:

let size = Int64.shift_left 1L 32 in
Unix.LargeFile.ftruncate Unix.stdout size

For example:

$ touch /tmp/file
$ strace ./test > /tmp/file
...
ftruncate(1, 4294967296)                = 0

However on armv7 (only) it actually truncates the file to 0 bytes:

ftruncate64(1, 0)                       = 0

Testing with OCaml 4.08.0 and armv7. It seems as if this is a regression since 4.07. We are tracking this bug downstream in https://bugzilla.redhat.com/show_bug.cgi?id=1733743 .

@rwmjones
Copy link
Contributor Author

@rwmjones rwmjones commented Jul 29, 2019

I compiled the test program on armv7 and disassembled it. The disassembly is rather large so I've linked it here: http://oirase.annexia.org/ocaml-8841.txt

@nojb
Copy link
Contributor

@nojb nojb commented Jul 29, 2019

Can you provide the assembly generated by the OCaml compiler, by doing

ocamlopt -c -S test.ml

?

Also, do you know if size = 0 before the call to ftruncate ?

@nojb
Copy link
Contributor

@nojb nojb commented Jul 29, 2019

(The output of ocamlopt -c -dcmm test.ml may also be useful.)

@rwmjones
Copy link
Contributor Author

@rwmjones rwmjones commented Jul 29, 2019

It's quite frustrating to get these results because I don't have access to the armv7 hardware myself. I have to submit everything as a job to the Fedora build system! Here's the -S output:

http://oirase.annexia.org/ocaml-8841-2.txt

In answer to the question about size before calling ftruncate, I added a printf statement and the right value (4294967296) is printed before calling ftruncate.

@stedolan
Copy link
Contributor

@stedolan stedolan commented Jul 29, 2019

From the linked disassembly:

00062328 <unix_ftruncate_64>:
   62328:       e92d4070        push    {r4, r5, r6, lr}
   6232c:       e1a04000        mov     r4, r0
   62330:       e5915004        ldr     r5, [r1, #4]
   62334:       eb0020f7        bl      6a718 <caml_enter_blocking_section>
   62338:       e1a02005        mov     r2, r5
   6233c:       e1a000c4        asr     r0, r4, #1
   62340:       e1a03fc5        asr     r3, r5, #31
   62344:       ebff63c1        bl      3b250 <ftruncate64@plt>

The relevant C code (with some macros expanded) is:

file_offset ofs = ((file_offset) Int64_val(len));
caml_enter_blocking_section();
result = ftruncate(Int_val(fd), ofs);

The assembly code is doing a 32-bit read from len and then sign-extending to 64-bits before calling ftruncate64. This looks like OCaml's file_offset type is being incorrectly defined as a 32-bit type on a 32-bit platform with large file support.

The file_offset type is defined in io.h as:

#if defined(_WIN32)
typedef __int64 file_offset;
#elif defined(HAS_OFF_T)
#include <sys/types.h>
typedef off_t file_offset;
#else
typedef long file_offset;
#endif

What should happen here is that configure should (a) ensure that whatever macros necessary to make off_t be 64 bits long are defined (_LARGEFILE_SOURCE or _FILE_OFFSET_BITS, maybe? This has always confused me), and (b) ensure the macro HAS_OFF_T is defined. It seems like at least one of these is failing to happen.

On my machine (amd64), HAS_OFF_T does not seem to be defined, so I suspect it's a bug in configure. (It doesn't break anything for me, because the fallback long type happens to be the right size here).

To help debugging, could you please run ./configure on a fresh checkout of the OCaml source tree, and post the three generated files below?

  • Makefile.config
  • runtime/caml/m.h
  • runtime/caml/s.h

@rwmjones
Copy link
Contributor Author

@rwmjones rwmjones commented Jul 29, 2019

I don't have access to the armv7 hardware so obtaining this is difficult. However here is the ./configure output when OCaml was compiled for armv7 on the builders: https://kojipkgs.fedoraproject.org//packages/ocaml/4.08.0/2.fc31/data/logs/armv7hl/build.log

@rwmjones
Copy link
Contributor Author

@rwmjones rwmjones commented Jul 29, 2019

Of course another question is why OCaml doesn't use off_t rather than homebrewing the same thing. We are defining -D_FILE_OFFSET_BITS=64 so off_t should be 64 bits.

@rwmjones
Copy link
Contributor Author

@rwmjones rwmjones commented Jul 29, 2019

@rwmjones
Copy link
Contributor Author

@rwmjones rwmjones commented Jul 29, 2019

@stedolan
Copy link
Contributor

@stedolan stedolan commented Jul 29, 2019

Thanks! This seems to be the bug: runtime/caml/s.h.in is missing a template for HAS_OFF_T, so the #ifdef HAS_OFF_T does not trigger.

I agree that we should just use off_t. The configure.ac even uses AC_TYPE_OFF_T, defining it to something sensible if the system headers don't, so there should be no problem using it unconditionally.

stedolan added a commit to stedolan/ocaml that referenced this issue Jul 29, 2019
stedolan added a commit to stedolan/ocaml that referenced this issue Jul 29, 2019
stedolan added a commit to stedolan/ocaml that referenced this issue Jul 29, 2019
stedolan added a commit to stedolan/ocaml that referenced this issue Jul 31, 2019
xavierleroy added a commit that referenced this issue Jul 31, 2019
xavierleroy added a commit that referenced this issue Jul 31, 2019
lpw25 added a commit to janestreet/ocaml that referenced this issue Aug 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants