[OCaml] Pass -D/-UNDEBUG through to ocamlc

[mgorny]

Summary

Detect [/-][DU]NDEBUG in CMAKE_C_FLAGS* and pass them through to ocamlc.
This is necessary because their value might affect visibility of dump
functions in LLVM and ocamlc uses its own compiler and flags by default.

[whitequark]

cmake/modules/AddOCaml.cmake

86 ↗

(On Diff #108303)

What is the relationship between CMAKE_C_FLAGS* and LLVM_DEFINITIONS?

[mgorny]

cmake/modules/AddOCaml.cmake

86 ↗

(On Diff #108303)

I haven't investigated it closely. But I have added a message() here and I didn't see any duplicates between the two, if that's what you're asking about.

[whitequark]

LGTM

[mgorny]

This broke some buildbots due to C compiler mismatch between OCaml and LLVM. Will have to think it over.

[mgorny]

v2: sets the compiler and with the flags so that we won't try to pass incorrect flags to another compiler. Since C compilers are reasonably compatible with each other, it shouldn't be a problem if OCaml was built with another.

[jpdeplaix]

Seems fair.

The -DNDEBUG related problem is a bit annoying when using the OCAML_LLVM_OUT_OF_TREE flag, but maybe we should forbid to have this flag and to be in Debug mode like this ?

diff --git a/cmake/modules/AddOCaml.cmake b/cmake/modules/AddOCaml.cmake
index 1b805c0710a..31aca43bc2f 100644
--- a/cmake/modules/AddOCaml.cmake
+++ b/cmake/modules/AddOCaml.cmake
@@ -174,6 +174,8 @@ function(add_ocaml_library name)
     foreach( llvm_lib ${llvm_libs} )
       add_dependencies("ocaml_${name}" "${llvm_lib}")
     endforeach()
+  elseif( uppercase_CMAKE_BUILD_TYPE STREQUAL "DEBUG" )
+    message(FATAL_ERROR "LLVM_OCAML_OUT_OF_TREE cannot be enabled while on Debug mode. Use -DCMAKE_BUILD_TYPE=Release instead.")
   endif()
 
   add_dependencies("ocaml_all" "ocaml_${name}")

[whitequark]

message(FATAL_ERROR "LLVM_OCAML_OUT_OF_TREE cannot be enabled while on Debug mode. Use -DCMAKE_BUILD_TYPE=Release instead.")

Why not just ignore CMAKE_C_COMPILER &c when building out of tree? What's the problem here exactly? This seems like an arbitrary restriction.

[mgorny]

@jpdeplaix, well, I don't know how NDEBUG affects you but in Gentoo it's more complex. We wipe out the CMake defaults that play with NDEBUG and set it manually so that we don't need to depend on CMAKE_BUILD_TYPE to enable assertions&co.

@whitequark, please review the diff, not comments that are not on the diff ;-).

[mgorny]

In D35898#824549, @whitequark wrote:

message(FATAL_ERROR "LLVM_OCAML_OUT_OF_TREE cannot be enabled while on Debug mode. Use -DCMAKE_BUILD_TYPE=Release instead.")

Why not just ignore CMAKE_C_COMPILER &c when building out of tree? What's the problem here exactly? This seems like an arbitrary restriction.

The problem is that I can't pass -DNDEBUG to the build because it does not respect CFLAGS. If I make it respect CFLAGS, the problem becomes that OCaml defaults to using a random compiler that's been hardcoded at build time and that doesn't necessarily work correctly (or even exist). It caused buildbots to fail because the compiler used for OCaml did not support flags needed for LLVM. On my system, it would cause random build failures because I always put full compiler version in CC/CXX, so everytime I upgrade compiler, packages that hardcode CC instead of respecting what I pass to it fail horribly.

So it's either respecting CMAKE_* and building OCaml just like the rest of LLVM, or adding extra variables so that I can enforce sane compiler+flags for the build. I don't think there's really a point in adding extra variables to make OCaml special unless reusing what's in there for LLVM already causes trouble.

[whitequark]

The problem is that I can't pass -DNDEBUG to the build because it does not respect CFLAGS.

This can be easily solved by only looking for -DNDEBUG in CMAKE_C_FLAGS* and then passing that specific flag in -ccopt. This works with every imaginable compiler, including MSVC.

If I make it respect CFLAGS, the problem becomes that OCaml defaults to using a random compiler that's been hardcoded at build time and that doesn't necessarily work correctly (or even exist).

If the compiler hardcoded into ocamlc does not exist your OCaml installation is simply broken. You won't be able to compile *any* native code via the OCaml toolchain, whether that's OCaml or C stubs. I don't think the LLVM build system should work around broken OCaml installations; to handle this case, a check during configure time that ocamlc can indeed compile C files is a good idea.

[mgorny]

All right, I'll update it to tackle [-/][DU]NDEBUG and move on.

[mgorny]

Updated to only play with NDEBUG definitions.

[whitequark]

LGTM

[jpdeplaix]

Is it possible to backport this to the 5.0 branch ?

[jpdeplaix]

Can you also backport this commit ? https://github.com/llvm-mirror/llvm/commit/876aa0d1c66ddcb1952d5153b4cb8486d5ac0ecc

[mgorny]

Yes, I've planned on doing that. https://bugs.llvm.org/show_bug.cgi?id=33987 now open for that purpose.

llvm/trunk/cmake/modules/AddOCaml.cmake

if (system_lib MATCHES "^-")
       list(APPEND ocaml_flags "${system_lib}" )
     else()
       # Otherwise assume it's a library name we need to link with.
       list(APPEND ocaml_flags "-l${system_lib}" )
     endif()
   endforeach()
 
   string(REPLACE ";" " " ARG_CFLAGS "${ARG_CFLAGS}")
-  set(c_flags "${ARG_CFLAGS} ${LLVM_DEFINITIONS}")
+  set(c_flags "${CMAKE_C_FLAGS} ${CMAKE_C_FLAGS_${uppercase_CMAKE_BUILD_TYPE}} ${ARG_CFLAGS} ${LLVM_DEFINITIONS}")
   foreach( include_dir ${LLVM_INCLUDE_DIR} ${LLVM_MAIN_INCLUDE_DIR} )
     set(c_flags "${c_flags} -I${include_dir}")
   endforeach()
 
   foreach( ocaml_file ${ARG_OCAML} )
     list(APPEND sources "${ocaml_file}.mli" "${ocaml_file}.ml")
 
     list(APPEND ocaml_inputs "${bin}/${ocaml_file}.mli" "${bin}/${ocaml_file}.ml")