Page MenuHomePhabricator

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

Authored by mgorny on Jul 26 2017, 9:28 AM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

mgorny created this revision.Jul 26 2017, 9:28 AM
whitequark added inline comments.Jul 26 2017, 9:34 AM
cmake/modules/AddOCaml.cmake
86 ↗(On Diff #108303)

What is the relationship between CMAKE_C_FLAGS* and LLVM_DEFINITIONS?

mgorny added inline comments.Jul 26 2017, 9:46 AM
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.

This revision is now accepted and ready to land.Jul 27 2017, 12:34 PM
This revision was automatically updated to reflect the committed changes.
mgorny reopened this revision.Jul 27 2017, 9:30 PM

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

This revision is now accepted and ready to land.Jul 27 2017, 9:30 PM
mgorny planned changes to this revision.Jul 27 2017, 9:30 PM
mgorny updated this revision to Diff 108630.Jul 28 2017, 5:19 AM
mgorny retitled this revision from [OCaml] Respect CMAKE_C_FLAGS for OCaml C files to [OCaml] Respect CMAKE_C_COMPILER & CMAKE_C_FLAGS for OCaml C files.
mgorny edited the summary of this revision. (Show Details)
mgorny added a reviewer: jpdeplaix.

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.

This revision is now accepted and ready to land.Jul 28 2017, 5:19 AM
mgorny requested review of this revision.Jul 28 2017, 5:19 AM
mgorny edited edge metadata.
jpdeplaix accepted this revision.Jul 28 2017, 7:46 AM

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}")
This revision is now accepted and ready to land.Jul 28 2017, 7:46 AM
whitequark requested changes to this revision.Jul 28 2017, 2:15 PM
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.

This revision now requires changes to proceed.Jul 28 2017, 2:15 PM

@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 ;-).

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.

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.

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

mgorny updated this revision to Diff 108778.Jul 29 2017, 12:23 AM
mgorny edited edge metadata.
mgorny retitled this revision from [OCaml] Respect CMAKE_C_COMPILER & CMAKE_C_FLAGS for OCaml C files to [OCaml] Pass -D/-UNDEBUG through to ocamlc.
mgorny edited the summary of this revision. (Show Details)

Updated to only play with NDEBUG definitions.

This revision is now accepted and ready to land.Jul 29 2017, 12:24 AM
This revision was automatically updated to reflect the committed changes.

Is it possible to backport this to the 5.0 branch ?

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