ocaml: Fix compile with ocaml 4.06, use unsafe strings

RFC PATCH In review John Thomson 5 participants 11 messages Jan 26, 2018 – Jan 30, 2018

lore

Share & tools

lore

John Thomson

author

2018-01-26 08:53 UTC #0 lore raw

RFC PATCH 1/1 ocaml: Fix compile with ocaml 4.06, use unsafe strings

Use autoconf to test if ocaml version >= 4.02
If so, use -unsafe-string for ocamlopt and ocamlc

Bytes & safe-string were introduced in ocaml 4.02 (2015-07-27)

With ocaml 4.06, -safe-string is now default
This separates the types bytes (mutable string) and string

ocaml 4.06 throws errors in compiling:

/build/xen/src/xen-4.10.0/tools/ocaml/libs/xc
File "xenctrl.ml", line 295, characters 44-45:
Error: This expression has type bytes but an expression was expected of type string File "xenctrl.ml", line 296, characters 25-26:
Error: This expression has type bytes but an expression was expected of type string

/build/xen/src/xen-4.10.0/tools/ocaml/libs/xb
File "xb.ml", line 88, characters 46-47:
Error: This expression has type bytes but an expression was expected of type string

With the safe-string changes, Unix.write and Unix.read use bytes, not string val write : file_descr -> bytes -> int -> int -> int

File "xb.ml", line 152, characters 19-22:
Error: This expression has type string but an expression was expected of type bytes

String.blit
val blit : string -> int -> bytes -> int -> int -> unit

There may be other locations

Signed-off-by John Thomson <[email protected]>

---
 config/Tools.mk.in      | 1 +
 tools/configure.ac      | 7 +++++++
 tools/ocaml/common.make | 6 ++++--
 3 files changed, 12 insertions(+), 2 deletions(-)

config/Tools.mk.in +1 -0

index 0f79f4e0c2..d4f5bb5d38 100644
@@ -50,6 +50,7 @@ GIT_HTTP            := @githttp@
 # Optional components
 XENSTAT_XENTOP      := @monitors@
 OCAML_TOOLS         := @ocamltools@
+OCAML_UNSAFE_STRING := @ocaml_unsafe_string@
 FLASK_POLICY        := @xsmpolicy@
 CONFIG_OVMF         := @ovmf@
 CONFIG_ROMBIOS      := @rombios@

tools/configure.ac +7 -0

index d1a3a78d87..30964e4a41 100644
@@ -299,6 +299,13 @@ AS_IF([test "x$ocamltools" = "xy"], [
                 AC_MSG_ERROR([Your version of OCaml: $OCAMLVERSION is not supported])])
             ocamltools="n"
         ])
+        AX_COMPARE_VERSION([$OCAMLVERSION], [ge], [4.02.0], [
+            ocaml_unsafe_string="-unsafe-string"
+            AC_SUBST(ocaml_unsafe_string)
+            AC_MSG_WARN([Your version of OCaml: $OCAMLVERSION requires compiling xen libraries with -unsafe-string])
+        ], [
+            ocaml_unsafe_string=""
+        ])
     ])
 ])

tools/ocaml/common.make +4 -2

index d5478f626f..d7fb80e728 100644
@@ -11,9 +11,11 @@ OCAMLFIND ?= ocamlfind
 
 CFLAGS += -fPIC -Werror -I$(shell ocamlc -where)
 
+OCAML_UNSAFE_STRING ?=
+
 OCAMLOPTFLAG_G := $(shell $(OCAMLOPT) -h 2>&1 | sed -n 's/^  *\(-g\) .*/\1/p')
-OCAMLOPTFLAGS = $(OCAMLOPTFLAG_G) -ccopt "$(LDFLAGS)" -dtypes $(OCAMLINCLUDE) -cc $(CC) -w F -warn-error F
-OCAMLCFLAGS += -g $(OCAMLINCLUDE) -w F -warn-error F
+OCAMLOPTFLAGS = $(OCAMLOPTFLAG_G) -ccopt "$(LDFLAGS)" -dtypes $(OCAMLINCLUDE) -cc $(CC) -w F -warn-error F $(OCAML_UNSAFE_STRING)
+OCAMLCFLAGS += -g $(OCAMLINCLUDE) -w F -warn-error F $(OCAML_UNSAFE_STRING)
 
 VERSION := 4.1

Signature · 7 lines

2.16.0


_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

M A Young

2018-01-26 09:09 UTC #1 lore raw

Quoted 7 lines · John Thomson

Use autoconf to test if ocaml version >= 4.02
If so, use -unsafe-string for ocamlopt and ocamlc
Bytes & safe-string were introduced in ocaml 4.02 (2015-07-27)
With ocaml 4.06, -safe-string is now default
This separates the types bytes (mutable string) and string

For Fedora Rawhide, which has ocaml 4.06, we came up with a patch to make xen ocaml safe-strings compliant (which I haven't got around to submitting to this list yet).

The patch is at
https://src.fedoraproject.org/rpms/xen/blob/master/f/xen.ocaml.safe-strings.patch
with the related Fedora bug at
https://bugzilla.redhat.com/show_bug.cgi?id=1526703

Michael Young

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

Christian Lindig

2018-01-26 09:29 UTC #2 lore raw

Quoted 18 lines · M A Young

Quoted 7 lines · John Thomson
Use autoconf to test if ocaml version >= 4.02
If so, use -unsafe-string for ocamlopt and ocamlc
Bytes & safe-string were introduced in ocaml 4.02 (2015-07-27)
With ocaml 4.06, -safe-string is now default
This separates the types bytes (mutable string) and string
For Fedora Rawhide, which has ocaml 4.06, we came up with a patch to make xen ocaml safe-strings compliant (which I haven't got around to submitting to this list yet).

Quoted 6 lines

The patch is at
https://src.fedoraproject.org/rpms/xen/blob/master/f/xen.ocaml.safe-strings.patch
with the related Fedora bug at
https://bugzilla.redhat.com/show_bug.cgi?id=1526703
Michael Young

The patch looks good and I would prefer to take it than to use -unsafe-string which isn’t a solution in the long run.

— Christian
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

Wei Liu

2018-01-26 11:43 UTC #3 lore raw

Quoted 18 lines · Christian Lindig

Quoted 18 lines · M A Young
Quoted 7 lines · John Thomson
Use autoconf to test if ocaml version >= 4.02
If so, use -unsafe-string for ocamlopt and ocamlc
Bytes & safe-string were introduced in ocaml 4.02 (2015-07-27)
With ocaml 4.06, -safe-string is now default
This separates the types bytes (mutable string) and string
For Fedora Rawhide, which has ocaml 4.06, we came up with a patch to make xen ocaml safe-strings compliant (which I haven't got around to submitting to this list yet).

Quoted 9 lines

Quoted 6 lines
The patch is at
https://src.fedoraproject.org/rpms/xen/blob/master/f/xen.ocaml.safe-strings.patch
with the related Fedora bug at
https://bugzilla.redhat.com/show_bug.cgi?id=1526703
Michael Young
The patch looks good and I would prefer to take it than to use -unsafe-string which isn’t a solution in the long run.

Hi Christian

Is that an ack?

Could you please in future explicitly give an Acked-by or Reviewed-by tag so that committers know they can apply the proposed patch directly?

Wei.

Quoted 5 lines

— Christian
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

Andrew Cooper

2018-01-26 11:47 UTC #4 lore raw

Quoted 23 lines · Wei Liu

Quoted 22 lines · Christian Lindig
Quoted 22 lines · M A Young
Quoted 7 lines · John Thomson
Use autoconf to test if ocaml version >= 4.02
If so, use -unsafe-string for ocamlopt and ocamlc
Bytes & safe-string were introduced in ocaml 4.02 (2015-07-27)
With ocaml 4.06, -safe-string is now default
This separates the types bytes (mutable string) and string
For Fedora Rawhide, which has ocaml 4.06, we came up with a patch to make xen ocaml safe-strings compliant (which I haven't got around to submitting to this list yet).
The patch is at
https://src.fedoraproject.org/rpms/xen/blob/master/f/xen.ocaml.safe-strings.patch
with the related Fedora bug at
https://bugzilla.redhat.com/show_bug.cgi?id=1526703
Michael Young

Quoted 8 lines

Quoted 2 lines
The patch looks good and I would prefer to take it than to use -unsafe-string which isn’t a solution in the long run.
Hi Christian
Is that an ack?
Could you please in future explicitly give an Acked-by or Reviewed-by tag so that committers know they can apply the proposed patch directly?

Also, is this patch compatible with Ocaml 3.09.3? If not, either the fix or the minimum version of Ocaml needs suitably adjusting.

~Andrew

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

Christian Lindig

Reviewed-by

2018-01-26 11:47 UTC #5 lore raw

In reply to Wei Liu

Quoted 33 lines · Wei Liu

Quoted 18 lines · Christian Lindig
Quoted 18 lines · M A Young
Quoted 7 lines · John Thomson
Use autoconf to test if ocaml version >= 4.02
If so, use -unsafe-string for ocamlopt and ocamlc
Bytes & safe-string were introduced in ocaml 4.02 (2015-07-27)
With ocaml 4.06, -safe-string is now default
This separates the types bytes (mutable string) and string
For Fedora Rawhide, which has ocaml 4.06, we came up with a patch to make xen ocaml safe-strings compliant (which I haven't got around to submitting to this list yet).
Quoted 9 lines
Quoted 6 lines
The patch is at
https://src.fedoraproject.org/rpms/xen/blob/master/f/xen.ocaml.safe-strings.patch
with the related Fedora bug at
https://bugzilla.redhat.com/show_bug.cgi?id=1526703
Michael Young
The patch looks good and I would prefer to take it than to use -unsafe-string which isn’t a solution in the long run.

Quoted 6 lines

Hi Christian
Is that an ack?
Could you please in future explicitly give an Acked-by or Reviewed-by tag so that committers know they can apply the proposed patch directly?

Sorry, I was assuming because this is an RFC that we are in a general discussion. Also, the patch was not part of the message. I still reviewed it.

Reviewed-by Christian Lindig <[email protected]>

— Christian
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

Wei Liu

2018-01-26 12:01 UTC #6 lore raw

Quoted 35 lines · Christian Lindig

Quoted 35 lines · Wei Liu
Quoted 18 lines · Christian Lindig
Quoted 18 lines · M A Young
Quoted 7 lines · John Thomson
Use autoconf to test if ocaml version >= 4.02
If so, use -unsafe-string for ocamlopt and ocamlc
Bytes & safe-string were introduced in ocaml 4.02 (2015-07-27)
With ocaml 4.06, -safe-string is now default
This separates the types bytes (mutable string) and string
For Fedora Rawhide, which has ocaml 4.06, we came up with a patch to make xen ocaml safe-strings compliant (which I haven't got around to submitting to this list yet).
Quoted 9 lines
Quoted 6 lines
The patch is at
https://src.fedoraproject.org/rpms/xen/blob/master/f/xen.ocaml.safe-strings.patch
with the related Fedora bug at
https://bugzilla.redhat.com/show_bug.cgi?id=1526703
Michael Young
The patch looks good and I would prefer to take it than to use -unsafe-string which isn’t a solution in the long run.
Hi Christian

Quoted 7 lines

Quoted 4 lines
Is that an ack?
Could you please in future explicitly give an Acked-by or Reviewed-by tag so that committers know they can apply the proposed patch directly?
Sorry, I was assuming because this is an RFC that we are in a general discussion. Also, the patch was not part of the message. I still reviewed it.

Normally when committers see "patch looks good" they would ask the maintainer to clarify whether that should be treated as an ack or reviewed-by. Or if committers feel the intent is clearly enough, just turn that line into an ack or rb.

To avoid needlessly round trip maintainers would normally give explicitly ack or review, and perhaps avoid sending ambiguous message.

If this patch is still under discussion, that's fine. There is no need to give a tag in a hurry. I just want to make sure you know the norm of how your message is interpreted.

Quoted 3 lines

Reviewed-by Christian Lindig <[email protected]>

Taking the above into consideration, does this tag still stand?

Also I think Andrew's comment needs to be addressed.

Wei.

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

Christian Lindig

2018-01-26 13:17 UTC #7 lore raw

Quoted 9 lines · Wei Liu

Quoted 4 lines
Reviewed-by Christian Lindig <[email protected]>
Taking the above into consideration, does this tag still stand?

Thanks for the clarification. My tag still stands as the code in the patch is IMHO fine.

Regarding Andrew’s comment: the bytes type was introduced in OCaml 4.02 - this makes the patch not compatible with OCaml 3.09.3. I still think that using the -unsafe-string option (introduced in OCaml 4.02) to keep strings mutable to avoid code changes is not going to work in the long run. The current OCaml version is 4.06.

— Christian
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

John Thomson

author

2018-01-26 23:23 UTC #8 lore raw

Quoted 3 lines · Christian Lindig

using the -unsafe-string option (introduced in OCaml 4.02) to keep strings mutable to avoid code changes is not going to work in the long run.

Okay,
NAK my unsafe-string patch.

Quoted 4 lines · M A Young

The patch is at
https://src.fedoraproject.org/rpms/xen/blob/master/f/xen.ocaml.safe-strings.patch
with the related Fedora bug at
https://bugzilla.redhat.com/show_bug.cgi?id=1526703

The safe-string patch is the correct way to fix this problem.
Get that patch signed off, submitted and reviewed in another thread? with a bump to 4.02.0 in tools/configure.ac AX_COMPARE_VERSION([$OCAMLVERSION], [lt], [3.09.3], [
https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=tools/configure.ac;h=d1a3a78d87ce2f077e551c6ee039caad4ad2edc1;hb=refs/heads/staging#l297
The patch author agreed to have the patch upstreamed on the Fedora bug report. How should this then be signed off and attributed?

I do not know if caml-stubdom uses any of these files, but it seems to use ocaml-3.11.0?
https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=stubdom/configure;h=a7a0c0915440b7b351a7adf4f40c8f9ee8e739ef;hb=refs/heads/staging#l3534

Cheers,
--
John Thomson

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

Michael Young

2018-01-28 23:44 UTC #9 lore raw

Quoted 14 lines · John Thomson

Quoted 3 lines · Christian Lindig
using the -unsafe-string option (introduced in OCaml 4.02) to keep strings mutable to avoid code changes is not going to work in the long run.
Okay,
NAK my unsafe-string patch.
Quoted 4 lines · M A Young
The patch is at
https://src.fedoraproject.org/rpms/xen/blob/master/f/xen.ocaml.safe-strings.patch
with the related Fedora bug at
https://bugzilla.redhat.com/show_bug.cgi?id=1526703

Quoted 7 lines

The safe-string patch is the correct way to fix this problem.
Get that patch signed off, submitted and reviewed in another thread? with a bump to 4.02.0 in tools/configure.ac AX_COMPARE_VERSION([$OCAMLVERSION], [lt], [3.09.3], [
https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=tools/configure.ac;h=d1a3a78d87ce2f077e551c6ee039caad4ad2edc1;hb=refs/heads/staging#l297
The patch author agreed to have the patch upstreamed on the Fedora bug report. How should this then be signed off and attributed?

The patch attached the bug report isn't the complete patch, as it was done against a temporary build including a patch of where I had got to before getting stuck again.

So I patched
tools/ocaml/libs/xc/xenctrl.ml
based on https://bugzilla.redhat.com/show_bug.cgi?id=1526703#c3
and also
tools/ocaml/libs/xb/xb.ml tools/ocaml/xenstored/stdext.ml

The other two files
tools/ocaml/xenstored/logging.ml
tools/ocaml/xenstored/utils.ml
are from the bugzilla patch.

Quoted 2 lines

I do not know if caml-stubdom uses any of these files, but it seems to use ocaml-3.11.0?
https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=stubdom/configure;h=a7a0c0915440b7b351a7adf4f40c8f9ee8e739ef;hb=refs/heads/staging#l3534

If we change this should it be the earliest version the code will support (4.02) or the latest (4.06)?

Michael Young

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

Christian Lindig

2018-01-30 09:26 UTC #10 lore raw

Quoted 9 lines · Michael Young

Quoted 4 lines
I do not know if caml-stubdom uses any of these files, but it seems to use ocaml-3.11.0?
https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=stubdom/configure;h=a7a0c0915440b7b351a7adf4f40c8f9ee8e739ef;hb=refs/heads/staging#l3534
If we change this should it be the earliest version the code will support (4.02) or the latest (4.06)?

I think this should be 4.02 - especially because the OCaml compiler in versions 4.05 and 4.06 had some bugs that are now being ironed out.

— Christian

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel