Issue982

Title Add build configuration for debugging LP builds
Priority bug Status resolved
Superseder Nosy List florian, jendrik, malte, thomas
Assigned To florian Keywords
Optional summary

Created on 2020-10-01.12:13:56 by florian, last changed by malte.

Messages
msg10020 (view) Author: malte Date: 2021-02-10.12:02:33
For visitors to this page from the future, for example if you have been directed here by the build error message that you get when using GLIBCXX_DEBUG together with an LP build: it should be theoretically possible to have both an LP build and GLIBCXX_DEBUG. If you are interested in this, or even interested in working on this, let us know.

I think there are two ways of making GLIBCXX_DEBUG compatible with LP solvers:

1. Compile OSI with GLIBCXX_DEBUG.
2. Get rid of the OSI dependency.

Neither is trivial: for the first, we would need to use two different versions of OSI depending on the build configuration, which sounds like it would require a bit of CMake-fu. The second requires building our own adapter layer for the LP solvers we want to support. This is a bit of coding work, but it's something we've been interested in independently of this issue. There may be an open issue for this, but I couldn't find one immediately.

The above assumes that OSI is the only library we use that is affected by GLIBCXX_DEBUG, but this would need to be checked. As far as I understand, GLIBCXX_DEBUG needs to be set consistently between parts of the code that exchange STL containers with each other (because the flag changes their memory layout etc.).

I would be surprised if CPLEX used STL containers in its interface because this is difficult to do when you only ship a binary and need to compatible with a wide variety of C++ setups. So libraries like CPLEX often have a C-style interface rather than a C++-style interface, or at least don't use STL containers in their interface functions and classes, and then these issues should not apply. But that's just a guess.

For Soplex, I'd be less surprised if it used a C++-style interface that requires a compatible STL implementation on both sides. But I also wouldn't be surprised if it doesn't. Much number-crunching is done using languages other than C++, and a C interface is much easier to integrate with other programming languages than a C++ interface.
msg10019 (view) Author: florian Date: 2021-02-10.11:47:21
After handling the comments, we decided to merge. Thanks for the review.
msg10017 (view) Author: malte Date: 2021-02-09.19:22:37
I left some review comments on github.
msg10015 (view) Author: florian Date: 2021-02-09.18:22:00
We discussed this and decided that we wanted the "normal" debug build to work with and without an LP solver, so it should not include the flag. We kept the debugnolp build for building with the flag but renamed it to glibcxx_debug to be more explicit about what it is meant for. It uses the flag and has to switch off the LP solver to avoid errors. We previously used the {release,debug}nolp builds on our automated tests but decided that it is sufficient if they only test the build with an LP solver.

The pull request is updated now.
msg10013 (view) Author: malte Date: 2021-02-09.12:52:26
Thanks, Florian and Thomas! You weren't there yesterday, but when we selected the story we said that the first thing we should do is discuss which of several possible solutions we actually want. (That's why the task board only has the task "Decide which option to implement.")

Can we have this discussion? I would like us to take a little step back for this, but I think the implementation work you already did will be useful either way.
msg10012 (view) Author: florian Date: 2021-02-09.12:47:50
Thomas and I prepared a pull request for this:
https://github.com/aibasel/downward/pull/25

It adds a CMake option to enable/disable the flag which defaults to false but is enabled in the build configuration for our debug build. We also added a new build configuration for compiling the planner with an LP solver in debug mode. Using the option in a way that would cause an error stops the build with an error message.

In particular, the behavior of the builds is as follows:

all release builds:
  Release builds are compiled without GLIBCXX_DEBUG as before but custom builds
  can enable the flag now.

debug:
  This build uses the flag and exits with an error message if an LP solver is used.

debugnolp:
  As before this compiles in debug mode without an LP solver even if one is installed.
  The flag is enabled in this build.

New build configuration "debuglp":
  This configuration disables the flag and thus can be used with an LP solver.

All builds (in particular custom builds):
  If the flag is used and an LP would be used, the build exits with an error.


Error Message:
Your build includes an LP solver and builds with the flag GLIBCXX_DEBUG. This makes the planner binary incompatible with the OSI library and leads to subtle errors. Set one of the options USE_LP or USE_GLIBCXX_DEBUG to false. You can also use the build configuration 'debuglp' for building with LP solvers but without GLIBCXX_DEBUG or the configuration 'debugnolp' for building with GLIBCXX_DEBUG but without LP solvers.
msg9729 (view) Author: malte Date: 2020-10-01.12:18:45
LP debug builds being broken is a serious issue that warrants removing _GLIBCXX_DEBUG. I'm of course happy with more advanced solutions that enable _GLIBCXX_DEBUG when it's safe.
msg9727 (view) Author: florian Date: 2020-10-01.12:13:56
issue837 introduced the flag _GLIBCXX_DEBUG which makes debug builds incompatible with libraries that are compiled without the flag. This means that currently, we have no way to run a debug build with an LP configuration.

We discussed a build configuration that disables _GLIBCXX_DEBUG. In issue837 we also discussed disabling the LP components in the (normal) debug build. Maybe we should combine the two suggestions and change the normal debug mode, so it enables _GLIBCXX_DEBUG if and only if no LP component is enabled? An alternative would be to have two debug builds: one with _GLIBCXX_DEBUG that forces all LP components to be disabled, and one without _GLIBCXX_DEBUG. What do you prefer?

(I'm classifying this as a bug because of the undefined behavior that can currently happen in debug LP builds but feel free to demote it if you disagree.)
History
Date User Action Args
2021-02-10 12:02:33maltesetmessages: + msg10020
2021-02-10 11:47:21floriansetstatus: reviewing -> resolved
assignedto: florian
messages: + msg10019
2021-02-09 19:22:37maltesetmessages: + msg10017
2021-02-09 18:22:00floriansetmessages: + msg10015
2021-02-09 12:52:27maltesetmessages: + msg10013
2021-02-09 12:47:50floriansetstatus: chatting -> reviewing
messages: + msg10012
2021-02-08 13:31:18thomassetnosy: + thomas
2020-10-01 12:18:45maltesetmessages: + msg9729
2020-10-01 12:13:56floriancreate