Bug 1392147 - brltty FTBFS with OCaml 4.04.0
Summary: brltty FTBFS with OCaml 4.04.0
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: brltty
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Jaroslav Škarvada
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2016-11-05 09:44 UTC by Richard W.M. Jones
Modified: 2016-11-09 10:52 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
: 1393750 (view as bug list)
Environment:
Last Closed: 2016-11-08 12:58:39 UTC
Type: Bug
Embargoed:


Attachments (Terms of Use)
build.log (186.66 KB, text/plain)
2016-11-05 09:44 UTC, Richard W.M. Jones
no flags Details
Proposed fix (801 bytes, patch)
2016-11-08 10:43 UTC, Jaroslav Škarvada
no flags Details | Diff
Proposed fix (778 bytes, patch)
2016-11-08 11:05 UTC, Jaroslav Škarvada
no flags Details | Diff

Description Richard W.M. Jones 2016-11-05 09:44:49 UTC
Created attachment 1217541 [details]
build.log

Description of problem:

See the failed build here:
http://koji.fedoraproject.org/koji/taskinfo?taskID=16300469

The build log is also attached.

Version-Release number of selected component (if applicable):

brltty-5.4-3.fc26

How reproducible:

Once.

Steps to Reproduce:
1. Build brltty in f26-ocaml side tag.

Comment 1 Jaroslav Škarvada 2016-11-08 10:43:46 UTC
Created attachment 1218481 [details]
Proposed fix

The attached patch fixed the problem for me, but maybe it's not correct (I don't know much about OCAML).

From the OCAML documentation (e.g. [1]):
> 19.5.1  Simple interface
> ...
> Rule 1
> ...
> In particular, CAMLlocal and CAMLxparam can only be called after CAMLparam.

In e.g. raise_brlapi_error and raise_brlapi_exception the CAMLlocal1 and CAMLlocal2 were used without CAMLparam.

The proposed patch seems to at least fix the compilation problem.

[1] http://caml.inria.fr/pub/docs/manual-ocaml-4.04/intfc.html

Comment 2 Richard W.M. Jones 2016-11-08 11:02:06 UTC
The patch is nearly but not quite right.

You mustn't pass non-"value" type arguments to CAMLparamX.  If
you try that then the garbage collector may crash when it tries
to follow those non-value pointers and interpret them as OCaml
heap objects.  So the second instance must use CAMLparam0() as well.

Comment 3 Jaroslav Škarvada 2016-11-08 11:04:59 UTC
(In reply to Richard W.M. Jones from comment #2)
> The patch is nearly but not quite right.
> 
> You mustn't pass non-"value" type arguments to CAMLparamX.  If
> you try that then the garbage collector may crash when it tries
> to follow those non-value pointers and interpret them as OCaml
> heap objects.  So the second instance must use CAMLparam0() as well.

Thanks for info, I doubt the patch was correct :)

Comment 4 Jaroslav Škarvada 2016-11-08 11:05:30 UTC
Created attachment 1218489 [details]
Proposed fix

Comment 5 Jaroslav Škarvada 2016-11-08 11:06:47 UTC
Jon, do you want to handle this bugzilla? I can do it myself as co-maintainer.

Comment 6 Richard W.M. Jones 2016-11-08 11:11:44 UTC
Patch looks good to me.  I'll add it and do the build as I
am coordinating everything in the f26-ocaml side tag.

Comment 7 Richard W.M. Jones 2016-11-08 11:24:45 UTC
Fix included in:
http://koji.fedoraproject.org/koji/taskinfo?taskID=16351380
brltty-5.4-4.fc26

Please send the patch upstream.

Comment 8 Jaroslav Škarvada 2016-11-08 12:18:02 UTC
(In reply to Richard W.M. Jones from comment #7)
> Fix included in:
> http://koji.fedoraproject.org/koji/taskinfo?taskID=16351380
> brltty-5.4-4.fc26
> 
> Please send the patch upstream.

Reassigning to myself, I will manage it with upstream.

Comment 9 Jaroslav Škarvada 2016-11-08 12:58:39 UTC
Patch sent upstream.

Comment 10 Jaroslav Škarvada 2016-11-08 16:34:04 UTC
Richard, upstream encountered the following warnings with OCaml-4.00 and the patch applied:

> ./brlapi_stubs.c: In function ‘raise_brlapi_error’:
> ./brlapi_stubs.c:147:29: warning: unused variable ‘caml__frame’
> [-Wunused-variable]
> ./brlapi_stubs.c: In function ‘raise_brlapi_exception’:
> ./brlapi_stubs.c:161:29: warning: unused variable ‘caml__frame’
> [-Wunused-variable]

This doesn't occur with OCaml-4.04. Is it bug? Is there a way how to workaround it?

Comment 11 Richard W.M. Jones 2016-11-08 16:39:38 UTC
It does occur with OCaml 4.04.0, it is a bug, and it is fixed by your patch
which I have now included in the package.

Comment 12 Jaroslav Škarvada 2016-11-08 16:41:57 UTC
(In reply to Richard W.M. Jones from comment #11)
> It does occur with OCaml 4.04.0, it is a bug, and it is fixed by your patch
> which I have now included in the package.

Sure, I wonder whether there is a way how to workaround the warnings with older OCaml. Brltty upstream is curious. He needs to support multiple versions of OCaml in brltty.

Comment 13 Richard W.M. Jones 2016-11-08 17:04:51 UTC
Interesting ...  I believe this was NOT an error in older versions
of OCaml.  I'm not sure it's an error even in OCaml 4.04.

There has been a change to the CAMLxparam<N> macros in the header,
but at the moment I'm unclear why it causes this new warning.

Perhaps it's newer GCC being cleverer about finding unused variables.

Comment 14 Jaroslav Škarvada 2016-11-08 17:26:08 UTC
FYI upstream fix:
https://github.com/brltty/brltty/pull/81

It seems upstream is happy, so it's resolved for me. Thanks Richard.

Comment 15 Richard W.M. Jones 2016-11-08 18:06:43 UTC
Thanks for liaising with upstream on this issue.

Comment 16 Jaroslav Škarvada 2016-11-09 10:52:28 UTC
I used upstream patch in rawhide (just commited, not built) as according to upstream using CAMLparam0() without CAMLreturn0() can lead to runtime problems.


Note You need to log in before you can comment on or make changes to this bug.