Fix warning on 4.10

[aantron]

See https://travis-ci.org/ocsigen/lwt/jobs/654080430#L430.

Made slightly difficult by that Bytes_val was added in 4.06.

[rwmjones]

For reasons I'm not very clear on this is actually an error (not a warning) for the Fedora build. It may be that the Fedora build system is adding some hardening flags which causes this.

[rwmjones]

I suggested changing String_val to Bytes_val, but as you point out that macro was only added in 4.06. It could be easier to simply cast the String_val to (char *).

[aantron]

I just added the cast in a branch, will merge after CI. By the way, the original patch filename weakly implied that this was caused by Lwt 5.1.2. In fact, it has been in Lwt for a very long time. 5.1.2 is just the first release after OCaml 4.10 was released. It is OCaml 4.10 that caused this.

[aantron]

I'll retag the 5.1.2 release, since it's stalled in opam's CI anyway.

[hannesm]

the issue with a cast from const char * to char * is that this is undefined behaviour in C AFAICT -- an alternative if some #ifdef dance and use Bytes_val for < 4.06, String_val for >= 4.06.

[rwmjones]

I guess in the future if the OCaml compiler decided to allocate strings in the text segment, this could actually fail. I don't think it does at the moment.

[aantron]

Do you have a reference for why and what exactly is UB? Is the actual cast operation UB, or is the argument about the effect of treating the underlying data "unsafely?"

Also note that, at the moment,

#define String_val(x) ((const char *) Bp_val(x))
#define Bp_val(v) ((char *) (v))

Is there a C implementation on which (char*)(const char*)(char*)(v) isn't the same as just (char*)v? I'll try the #ifdef version anyway, but I am pretty skeptical about the cast being invalid.

[aantron]

@hannesm See commit attached above.

[hannesm]

I don't have an authoritative reference, sorry. But https://en.wikipedia.org/wiki/Const_%28computer_programming%29#Loopholes_to_const-correctness says "Writing into a const variable this way may work as intended, but it causes undefined behavior and seriously contradicts const-correctness".

Thanks.

[aantron]

This part of the Wikipedia article needs some editing attention :p

What ultimately convinced me is @rwmjones' last comment about the representation of strings potentially changing. Even if their storage doesn't move to a read-only section like .text or .rodata, the definition of String_val might change in the future, and diverge from Bytes_val, even despite the fact that we have Bytes.unsafe_of_string and Bytes.unsafe_to_string. So, it's better to just use the macros.