CAML*alloc macros: remove unused variadic argument

[nojb]

There are some macros in misc.h (or rather, their callsites) which are not strictly valid according to C99:

In file included from ./link.c:42:
/home/malc/x/scm/git/llpp/build/lib/ocaml/caml/memory.h:89:32: error: ISO C99 requires at least one argument for the "..." in a variadic macro [-Werror]
   89 | CAMLmalloc(caml_stat_free, 1, 1) CAMLreturns_nonnull()

This warning does not trigger in the compiler build because not enough warnings are enabled. But since this header is liable to be included in user code which may be compiled with more strict warnings, I think it would be a good idea to fix this, as per this PR.

[nojb]

Assigning to @MisterDA as a C standards compliance expert. (Well, I can't seem to be able to actually assign the PR to him...)

[kit-ty-kate]

I believe this was part of #13777. There seems to also be more than just these macros (but maybe the branch is too old and that's not the case anymore)

[nojb]

I believe this was part of #13777. There seems to also be more than just these macros (but maybe the branch is too old and that's not the case anymore)

Indeed, thanks for the pointer. From a quick glance, I think the present PR is complete with regards to these macros, but I will take a closer look later.

[MisterDA]

Assigning to @MisterDA as a C standards compliance expert.

It's a work in progress ;-) I wasn't aware of the problem with variadic macros at the time.

(Well, I can't seem to be able to actually assign the PR to him...)

I'm not a core dev! (cf ⬆️ )

I believe this was part of #13777. There seems to also be more than just these macros (but maybe the branch is too old and that's not the case anymore)

Yep, that's 0b01951. No, I don't think that the branch is too old. The C11 atomic issue has been resolved, and the thread_local issue is being addressed.

[nojb]

I believe this was part of #13777. There seems to also be more than just these macros (but maybe the branch is too old and that's not the case anymore)

Yep, that's 0b01951.

Thanks for the pointer. I replaced my patch by a straight up cherry-pick of your commit and added a Changes entry (with you as author, and me as reviewer :))

[MisterDA]

My concern when I added the variadic macros was to account for future extensibility. Maybe we could add a note saying that the macros are reserved for call headers only and should not be used by 3rd-party code?

[dra27]

(Well, I can't seem to be able to actually assign the PR to him...)

You have - as you did - to trick him into commenting and then he can be assigned 😈

[dra27]

LGTM (adding to @nojb's implicit approval from reviewing)

[jmid]

This warning does not trigger in the compiler build because not enough warnings are enabled. But since this header is liable to be included in user code which may be compiled with more strict warnings, I think it would be a good idea to fix this, as per this PR.

Polite ping @Octachron:
FYI, this triggered in the 5.4.0~alpha1 CI tests of this opam-repo PR: ocaml/opam-repository#28619.
I'll let you decide if this is enough reason to cherry-pick to 5.4.0 and roll a 5.4.0~rc2.

[Octachron]

While this PR reduces the number of false positive warnings emitted by C compilers, my opinion is that the root mistake in the opam-repository is the use of -Wall -Wextra -Werror -pedantic in a release settings. Thus I tend to think that the bug fixes can wait for 5.5 .

[dra27]

Apologies, given that the original change was introduced in 5.4 (#13537), I should have cherry-picked this after merging.

I agree with @Octachron's assessment, though - however, after 5.4.0 is released, I think we should cherry-pick this to the 5.4 branch just so it can be picked up in a 5.4.1.

[jonahbeckford]

Context: This is fully embedded stub library code that was failing - the merger of a popular zip library and OCaml. The zip library uses the -Werror flag for release builds (https://github.com/kuba--/zip/blob/81314fff0a882b72a9ad321e7a3311660125b56e/CMakeLists.txt#L72), is fuzzed, participates in GitHub's Code Security program and generally may be considered overly security obsessed. In OCaml, we have two core devs saying not to use -Werror. So, I have to mention the context that -Werror did not come out of nowhere.

I'll dump a couple notes from the Open Source Security Foundation (https://best.openssf.org/Compiler-Hardening-Guides/Compiler-Options-Hardening-Guide-for-C-and-C++.html):

We recommend developers to additionally use a blanket -Werror to treat all warnings as errors during development. However, -Werror should not be used in this blanket form when distributing source code, as this use of -Werror creates a dependency on specific toolchain vendors and versions.

Test processes should use both instrumented test code and production code.

Going by that, the real root cause is that ocaml/ocaml test processes do not include "instrumented test code" where undefined behavior and other bugs are regularly caught by -Werror. This PR (#14238) seems to be evidence of that (please correct me if I'm wrong). And IMHO the root of the root cause is because ocamlc -config does not differentiate between development and production compiler flags.

I am quite uncomfortable ignoring real errors that are being uncovered by -Werror, especially since it seems only by accident someone caught it earlier. I do understand the fragility that places on ocaml/ocaml today. A better compromise that accounts for the fragility would be:

1. excluding 5.4.0 from my packages in the opam-repository, and
2. if -Werror does trigger a false positive in some future ocaml version, it is my responsibility to selectively edit the warning at that time, and
3. completely removing -Werror from default flags if ocamlc -config gains something like a ocamlc_cppflags_debug variable in the future

What do you think?

[MisterDA]

I authored a change in Dune that automatically adds -Wall or -W2 for cc and msvc compilation types, respectively. ocaml/dune#11535
Here's an exchange we had:

(me) Maybe we could go further and enable the -Werror Make all warnings into errors flag in debug mode (-WX for msvc).

(@nojb) I would be very careful about this change. We did that with OCaml warnings, and it is a recurring pain point for users. So I would leave this for a later PR at least.

The problem can also be fixed by patching end-users build systems, like Dune, rather than the compiler itself.

[jonahbeckford]

I authored a change in Dune that automatically adds -Wall or -W2 for cc and msvc compilation types, respectively. ocaml/dune#11535 Here's an exchange we had:

(me) Maybe we could go further and enable the -Werror Make all warnings into errors flag in debug mode (-WX for msvc).

(@nojb) I would be very careful about this change. We did that with OCaml warnings, and it is a recurring pain point for users. So I would leave this for a later PR at least.

Interesting. Although I'm unsure how comparable OCaml warnings are to C warnings. (related thought: It is rare to find C build tools that don't distinguish between debug and release flags. The major anomaly was autoconf/automake which never standardized a way to split them. Maybe this makes it hard for OCaml? If so, this isn't a small PR discussion.)

The problem can also be fixed by patching end-users build systems, like Dune, rather than the compiler itself.

How would that work in end-user build systems? To get debug support in MSVC+OCaml today, I have to patch the compiler (https://github.com/diskuv/dkml-compiler/blob/6032af38f19b2d15c45145bd0fef3a9265a2f901/src/p/ocaml-common-4_14-a05-msvccflags.patch) to remove the -O2. Why? Because optimization flags aren't additive, and MSVC rightly creates a compiler warning if -Od (etc.) gets added to -O2. On the flip side, I have had to make PRs to several projects who add -O3 in Dune flags ... which isn't an optimization level in MSVC ... ex. mirage/mirage-crypto@d086ab2.

If you are suggesting that Dune and other build systems parse the C flags provided by ocamlc -config (and the user?) to find which are debug and which are production flags ... yeah, that will work. Horribly hacky, but that will work. If the real unstated technical difficulty is OCaml's use of autoconf ... which isn't going to change anytime soon ... then the horribly hacky thing is the right thing to do.

[MisterDA]

If you are suggesting that Dune and other build systems parse the C flags provided by ocamlc -config (and the user?) to find which are debug and which are production flags ... yeah, that will work.

Am I correct in understanding the return of ocamlc -config as being the minimal set of flags to read the OCaml headers and link with the runtime?
Note that the compiler has internal flags that are used during its build, and also flags that are exposed by the -config option.

Why? Because optimization flags aren't additive, and MSVC rightly creates a compiler warning if -Od (etc.) gets added to -O2.

That could be considered a bug in ocaml -config, but I guess backwards compatibility could prevent us from removing the -O2. TBD.

How would that work in end-user build systems?

From Dune's Flags in Foreign Code

the base :standard set of flags for C will contain only ocamlc_cflags or both ocamlc_cflags and ocamlc_cppflags.

Using Dune's Ordered Set Language one can build the list of flags, in particular remove a flag from the :standard set with :standard \ -O2. I often use the following trick (courtesy of nojb) to select different flags based on the ccomp_type:

(rule
 (with-stdout-to c_flags.cc (echo "(-Wall)")))

(rule
 (with-stdout-to c_flags.msvc (echo "(/W2)")))

(library
 (name mylib)
 (foreign_stubs
  (language c)
  (flags :standard (:include c_flags.%{ocaml-config:ccomp_type}))
  (names mystubs)))

maybe it can be extended to exclude flags.

[dra27]

Just in case the way at @Octachron and I were expressing things makes it look we're passing the buck completely, I definitely agree that testing on ocaml/ocaml not catching this issue earlier was bad, and anything which can improve this is a good idea (I'm not sure if our sanitizers checks were running this or not, or were catching and were being ignored, cf. https://ci.inria.fr/ocaml/job/sanitizers/). Our own CI - and any working on a dev version of OCaml - is always done with -Werror

[hannesm]

I have not managed to figure out what configure options the main CI (on GitHub workers) is using:

• I'd propose to use -Wall -Wextra -Wpedantic -Werror in the CI -- the C compiler is there to help (and if this fails, you can always exclude some warnings): given the zoo of OS & distributions the actions are executed, there will be a whole family of different C compilers inspecting the code base (which is good), much more than a single developer usually uses on their machine;
• I also suggest to not release software with -Werror, given the C compilers are getting new releases, and if you want your software to last, you can never know which new warnings a C compiler will have. With projects such as solo5 and mirage-crypto we used to release with -Werror, and are glad we backed out (new C compilers may generate warnings, but won't refuse to build the software) - leading to less pain (errors) for the opam-repo-ci.

Just my 2 cents

[MisterDA]

The compiler itself cannot (currently) be built in pedantic mode as we are using extensions such as casting function pointers to object pointers, passing non void * pointers to the %p format specifier, or GCC's computed gotos, (also this would require setting all feature test macros and getting our POSIX support right), but we can definitely add a test that checks that both a C and a C++ file that includes all caml/ headers compile correctly with the -Wpedantic flag.

[Octachron]

@jonahbeckford

Please do use -Werror in your tests and during development ! As stated in your citation, this is good practice. However, as also stated in this citation, it is generally better to disable -Werror when distributing release archive.

This could be achieved in MlFront by adding a -profile %{profile}argument to the action that generate zipfile_discover.sexp in order to not add -Werror in release mode. Do you wish me to send a PR?

@dra27 , Nevertheless taking in account that the variadiac argument was added to those macros in 5.4, and there are removed in trunk, I think that there is a good argument to cherry-pick this change in the 5.4 release candidate. From my point of view, this would simplify the maintenance in the long term to avoiding a transient complexity for just one released version.

Similarly, I am not sure if it is a good idea to add the well-named -pedantic flag everywhere, but maybe enabling it for the public header in runtime/caml would be a good step forward?

[MisterDA]

we can definitely add a test that checks that a C and a C++ file that includes all caml/ headers compiles correctly with the -Wpedantic flag.

maybe enabling it for the public header in runtime/caml would be a good step forward?

A possible starting point, from a while ago: dd1197b ;)

The compiler itself cannot (currently) be built in pedantic mode as we are using extensions such as casting function pointers to object pointers, passing non void * pointers to the %p format specifier, or GCC's computed gotos

I was slightly wrong here. To get access to these extensions we'd need to specify the GNU C dialect with -std=gnuc17, see the docs of -Wpedantic. Perhaps it could indeed be done in the CI. This might require setting feature test macros, and getting our POSIX support right (#10505).

[jonahbeckford]

Thanks @Octachron but you don't need to send a PR since that change is trivial. More importantly, I'll eventually add a development CI job for ocaml/ocaml+patches in my own test cluster, and can restrict the range of available OCaml versions until my CI job is ready. I will also look at the references that @MisterDA posted at some point.

[Octachron]

Cherry-picked to the 5.4 branch as 59ca319 to avoid ephemeral complexity. It will be part of the final release of 5.4 .