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

"struct hack" for bigarrays clashes with clang array bounds checks #5516

Closed
vicuna opened this issue Feb 25, 2012 · 11 comments

Comments

Projects
None yet
1 participant
@vicuna
Copy link

commented Feb 25, 2012

Original bug ID: 5516
Reporter: @mmottl
Status: closed (set by @xavierleroy on 2015-12-11T18:08:10Z)
Resolution: fixed
Priority: normal
Severity: minor
Platform: Apple Mac
OS: Mac OS X
OS Version: 10.7.3
Version: 3.12.1
Fixed in version: 3.13.0+dev
Category: otherlibs
Related to: #5761
Monitored by: mww

Bug description

The most recent release of clang (part of Xcode) for Mac OS X Lion reportedly fails to compile C-bindings accessing bigarrays due to the "struct hack" used to declare the "dim" field in the caml_ba_array declaration. This hack can trigger bogus array bounds errors in user code.

Would it be possible to make this declaration more general? It seems that using "CAML_BA_MAX_NUM_DIMS" instead of "1" as declared size (+ accordingly adapting caml_ba_alloc) would work. Or using the C-preprocessor to generate different code if C99 mode is detected as described here (would also require adapting caml_ba_alloc):

http://bugs.call-cc.org/attachment/ticket/778/0001-Use-flexible-array-member-in-C99-mode-silences-clang.patch

@vicuna

This comment has been minimized.

Copy link
Author

commented Feb 27, 2012

Comment author: @xavierleroy

Tentative fix in SVN trunk, revision 12188. Use a "flexible" array type if C99 or GCC. Tested using GCC under Linux; let us know if it doesn't fix the issue with Clang.

@vicuna

This comment has been minimized.

Copy link
Author

commented Feb 27, 2012

Comment author: mww

Does not seem to work: OCaml itself builds and installs fine;
bin-prot compiles but it's test fails:

I: Running test 'test_runner'
I: Changing directory to 'lib_test'
I: Running command '/opt/local/var/macports/...../ocaml-bin-prot/work/bin-prot-2.0.3/_build/lib_test/test_runner'
..F...............FFFFFFF..........F.....W: Test 'test_runner' fails: Command '/opt/local/var/macports/...../ocaml-bin-prot/work/bin-prot-2.0.3/_build/lib_test/test_runner' terminated with error code 255
I: Changing directory to '/opt/local/var/macports/...../ocaml-bin-prot/work/bin-prot-2.0.3'
I: Running test 'mac_test'
I: Changing directory to 'lib_test'
I: Running command '/opt/local/var/macports/...../ocaml-bin-prot/work/bin-prot-2.0.3/_build/lib_test/mac_test'
msgs: 100000 msg length: 461
write time: 0.198s write rate: 503912.91 msgs/s write throughput: 221.54 MB/s
read time: 0.276s read rate: 362862.97 msgs/s read throughput: 159.53 MB/s
I: Changing directory to '/opt/local/var/macports/..../ocaml-bin-prot/work/bin-prot-2.0.3'
E: Tests had a 50.00% failure rate
make: *** [all] Error 1

This is OS X 10.7.3/x86_64/clang-llvm 3.0/ocaml 3.12.1 with the bigarray.h patch

@vicuna

This comment has been minimized.

Copy link
Author

commented Feb 28, 2012

Comment author: @mmottl

@mww

please use the latest version of bin-prot (2.0.7), which can be downloaded from here: http://forge.ocamlcore.org/projects/bin-prot

The latest version should fix some portability problems, which are the likely reason for the observed errors.

The next release of bin-prot will be part of the Jane Street Core library at bitbucket, which should be finalized any day now. Until then, I'm afraid, there will still be some version confusion due to multiple download sites.

@vicuna

This comment has been minimized.

Copy link
Author

commented Feb 28, 2012

Comment author: mww

bin-prot 2.0.7 also has a test failure rate of 50%; the crash-reporter gives me something like this:

Exception Type: EXC_BAD_ACCESS (SIGBUS)
Exception Codes: KERN_PROTECTION_FAILURE at 0x000000010055cff0

VM Regions Near 0x10055cff0:
MALLOC guard page 000000010055b000-000000010055c000 [ 4K] ---/rwx SM=NUL
--> MALLOC metadata 000000010055c000-000000010055d000 [ 4K] r--/rwx SM=PRV
MALLOC_LARGE 000000010055d000-0000000100600000 [ 652K] rw-/rwx SM=PRV

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0 libsystem_c.dylib 0x00007fff9214fe7f memmove$VARIANT$sse42 + 450
1 test_runner 0x000000010043446e caml_blit_string + 30
2 test_runner 0x0000000100400b6a .L193 + 37

@vicuna

This comment has been minimized.

Copy link
Author

commented Feb 28, 2012

Comment author: @xavierleroy

After rereading the change, I still can't see anything wrong. It doesn't mean nothing is wrong, just that I need help finding what's wrong :-)

I downloaded bin-prot 2.0.7 and tried to test it on my Linux x86-64 box with the development version of OCaml. I gave up because I'm too lazy to set up ocamlfind and the dependencies of bin-prot with the devel version.

To mww: does bin-prot 2.0.7 work on your machine with a stock OCaml 3.12.1 ?

@vicuna

This comment has been minimized.

Copy link
Author

commented Feb 28, 2012

Comment author: @mmottl

I have tried to reproduce the problem on my Mac using clang for compilation (using the -cc flag with the OCaml compiler), but have not managed to trigger any problems. My configuration seems to be the same as yours. The resulting code seems to be running somewhat faster than with gcc, though this library is highly sensitive to code placement, etc.

@mww: please send me an email detailing how exactly you are using clang with bin-prot on your Mac.

@xLeroy: it's usually easiest to install Godi first (takes a few human minutes only) and just select the bin-prot package. This should build and install everything you need for testing. Then you can toy with the library source as usual.

@vicuna

This comment has been minimized.

Copy link
Author

commented Feb 28, 2012

Comment author: mww

bin-prot works w/o the bigarray patch (though I had to patch bin-prot to silence the array-out-of-bound warnings and to remove all "inline" directives);
if I only patch bigarray.h after compiling ocaml, everything works fine;

@vicuna

This comment has been minimized.

Copy link
Author

commented Mar 1, 2012

Comment author: @damiendoligez

Another data point: bin-prot 2.0.3 works with the current trunk (revision 12189) on Mac OS 10.6.

(modulo a trivial change to bin-prot to silence C compiler warnings).

@vicuna

This comment has been minimized.

Copy link
Author

commented Mar 1, 2012

Comment author: @mmottl

I'm pretty certain the observed problem was due to an incorrect patch file applied to the MacPorts OCaml package. It did not contain changes to bigarray_stubs.c, which would make allocated arrays too short with clang.

@mww, if updating the patch accordingly solves your problem, please also let us know here so that this issue can be closed. Thanks!

@vicuna

This comment has been minimized.

Copy link
Author

commented Mar 6, 2012

Comment author: mww

ok, I've added also the corresponding changes from bigarray_stubs.c (complete patch is here [1]) and bin-prot seems to compile and work flawless.

Regarding the changes: Don't they break for gcc? At least for Apple's gcc 4.2 on 10.6 this seems to break -- the preprocessor macro that checks for clang/llvm also says gcc is always fine:

#if (STDC_VERSION >= 199901L) || defined(GNUC)

This does not seem to work for Apple's XCode on 10.6: gcc 4.2; I changed the patch so the check only reads:

#if (STDC_VERSION >= 199901L)

which seems to work for XCode/gcc 4.2 on 10.6 and also for XCode/clang/llvm 4.3 on 10.7.

[1] https://trac.macports.org/browser/trunk/dports/lang/ocaml/files/patch-otherlibs-bigarray.diff

@vicuna

This comment has been minimized.

Copy link
Author

commented Apr 3, 2012

Comment author: @xavierleroy

Thanks for the testing. Normally, a compiler that defines GNUC should implement all GNU extensions to the C language, which include flexible arrays at end of structs, predating C99. But, to be on the safe side, I removed the "|| defined(GNUC)". (Commits 12311 and 12312.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.