cloudius-systems / osv

OSv, a new operating system for the cloud.
osv.io
Other
4.12k stars 605 forks source link

Failing build on Fedora 39 #1301

Closed nyh closed 8 months ago

nyh commented 10 months ago

When trying to build OSv on Fedora 39 (which has Gcc 13.2.1, boost 1.81, glibc 2.38), OSv fails to build.

There appears to be just one compilation error, although who knows what we'll discover after fixing it. The problem I see is that two source files - core/mempool.cc and core/callstack.cc end up using /usr/include/boost/describe/members.hpp and failing on errors like

/usr/include/boost/describe/members.hpp:78:42: error: expected unqualified-id be
fore ‘=’ token
   78 |         static constexpr unsigned hidden = name_is_hidden<D, L>::value? 
mod_hidden: 0;
      |                                          ^

The problem is that in include/glibc-compat/features.h we do

#define hidden __attribute__((__visibility__("hidden")))

amd this macro prevents boost from using the name "hidden" for a variable like it wants to!

@wkozaczuk explained why this "hidden" macro exists in Musl in commit af2d371a61f6ab1eb5a066a0c3e93230faf6611c, but we now need to find a way to avoid it being included in the specific source files which use this Boost feature :-(

Maybe this dangerous macro shouldn't be in a header file at all, and rather be set with "-D" just when building files in the musl directory, and not in core/? Or maybe it is a mistake for the two specific files mempool.cc and callstack.cc to include include/glibc-compat/features.h and we need to find a way to avoid this inclusion?

nyh commented 10 months ago

The macro "hidden" is unfortunately assumed by dozens of Musl-based files, some of which we modifed and some we didn't, so instead of renaming it in dozens of places (we have the name OSV_HIDDEN already!), I did the following very short and simple patch, which in the two places where Boost is bothered by the "hidden" macro, we #undef it:

diff --git a/core/mempool.cc b/core/mempool.cc
index df30c28e..52173873 100644
--- a/core/mempool.cc
+++ b/core/mempool.cc
@@ -31,11 +31,15 @@
 #include <osv/shrinker.h>
 #include <osv/defer.hh>
 #include <osv/dbg-alloc.hh>
+#include <osv/migration-lock.hh>
+#include <osv/export.h>
+
+// recent Boost gets confused by the "hidden" macro we add in some Musl
+// header files, so need to undefine it
+#undef hidden
 #include <boost/dynamic_bitset.hpp>
 #include <boost/lockfree/stack.hpp>
 #include <boost/lockfree/policies.hpp>
-#include <osv/migration-lock.hh>
-#include <osv/export.h>

 TRACEPOINT(trace_memory_malloc, "buf=%p, len=%d, align=%d", void *, size_t,
            size_t);
diff --git a/include/osv/callstack.hh b/include/osv/callstack.hh
index 0543f970..8128fc8c 100644
--- a/include/osv/callstack.hh
+++ b/include/osv/callstack.hh
@@ -8,13 +8,16 @@
 #ifndef CALLSTACK_HH_
 #define CALLSTACK_HH_

-#include <boost/intrusive/unordered_set.hpp>
 #include <osv/trace.hh>
 #include <osv/percpu.hh>
 #include <memory>
 #include <atomic>
 #include <stdlib.h>
 #include <set>
+// recent Boost gets confused by the "hidden" macro we add in some Musl
+// header files, so need to undefine it
+#undef hidden
+#include <boost/intrusive/unordered_set.hpp>

 // An object that instruments tracepoints to collect backtraces.
 //

@wkozaczuk what do you think? You've been involved recently (relatively...) in the update of Musl, does this look like a reasonable workaround to you, or should we start modifying our Musl-based code to get rid of that "hidden" macro?

wkozaczuk commented 10 months ago

@nyh I think this is a very reasonable solution. Most source files (except for musl/* and most of libc/*) are simply compiled with -fvisibility=hidden if symbols hiding is enabled. Therefore the hidden macro does not play any role in files under core/ and can be undef-ed like you propose. I also agree that in the long run, it would be nice to not include include/glibc-compat/features.h by non-libc/non-musl files or move the hidden macro to some better place where it would not affect files likemempool.cc.