Link against the Boehm-Demers-Weiser Garbage Collector using FetchContent. #3930

fruffy · 2023-03-19T23:40:37Z

To have more control over linking and setup of the Boehm-Demers-Weiser garbage collector P4C uses we add it as a FetchContent dependency. For compatibility users can still choose to link with an external dependency. Several other changes were necessary to make this work:

We build and link the static libraries of the garbage collection library. This way we do not run into missing library problems. To be able to do so we had to disable some definitions in gc_cpp.cpp.
We enable threads and fork support by default to ensure parallelism support.
We do not link bdwgc's include directories globally. Instead we use the Abseil approach and mandate that every target links to library that pulls in the BDWGC headers.
Clean up the top-level CMakelists a little and move all the module includes to the top.

vlstill · 2023-03-20T08:09:06Z

Hi @fruffy! What is the motivation to turn libGC into a submodule? Do we need something from a specific new version? Even if so, is submodule really a good solution? Unless we want to track upstream closely I think we should stick either to using installed libgc, or if we can't we can use cmake's fetch content. A submodule adds extra overhead for checkout (and both submodule and fetch content add build overhead, but I guess that is less of a concern).

There are concerns that this would impact downstream compilers which now use a fixed version of libGC in their build/release environments (especially if p4c was to track upstream libCG closely, as opposed to tracking releases).

fruffy · 2023-03-20T12:37:37Z

Hi @fruffy! What is the motivation to turn libGC into a submodule? Do we need something from a specific new version? Even if so, is submodule really a good solution? Unless we want to track upstream closely I think we should stick either to using installed libgc, or if we can't we can use cmake's fetch content. A submodule adds extra overhead for checkout (and both submodule and fetch content add build overhead, but I guess that is less of a concern).

There are concerns that this would impact downstream compilers which now use a fixed version of libGC in their build/release environments (especially if p4c was to track upstream libCG closely, as opposed to tracking releases).

libGC interacts badly with the current version of the Z3 solver (for example, #3927). A more recent version of Z3 (4.12.0) does not work at all. A newer version of the garbage collector and the appropiate configuration appears to fix these problems. The configuration is not possible with the system-installed libgc.

The new version also makes the garbage collection more stable. For example, the crashes when running gdb disappears and Too many heap sections: Increase MAXHINCR or MAX_HEAP_SECTS becomes less frequent. Increased performance is another nice side-effect. I have to verify this but I see a 30% improvement in long-running test-case generation time for P4Testgen.

It seems prudent to include something as essential as the garbage collection directly as a submodule. It allows us to more easily configure the collector and fix bugs. Thankfully, there are not many source files and it compiles very quickly.

There is still a lot more testing to do, of course. I am not planning to merge this any time soon, but it is good to have this PR as scratch pad to weed out issues with P4Testgen and Z3 etc.

vlstill · 2023-03-20T14:11:00Z

I see. I've noticed the Z3 problem with libGC but did not investigate it further. It is good that it can be fixed on libGC side.

The improvements in stability & speed would be very nice :-).

It seems prudent to include something as essential as the garbage collection directly as a submodule. It allows us to more easily configure the collector and fix bugs. Thankfully, there are not many source files and it compiles very quickly.

If we are dependent on a particular (minimal) version that is not widely available and on custom configuration than I agree this would be simpler or even required.

I still wonder if we can:

use a release tag/branch for our submodule
use fetch-content instead to submodule to avoid having the sources checked out in the repo (but still have a fixed version from git, just using cmake instead of submodules).

fruffy · 2023-03-21T01:48:39Z

I see. I've noticed the Z3 problem with libGC but did not investigate it further. It is good that it can be fixed on libGC side.

The improvements in stability & speed would be very nice :-).

It seems prudent to include something as essential as the garbage collection directly as a submodule. It allows us to more easily configure the collector and fix bugs. Thankfully, there are not many source files and it compiles very quickly.

If we are dependent on a particular (minimal) version that is not widely available and on custom configuration than I agree this would be simpler or even required.

I still wonder if we can:
* use a release tag/branch for our submodule

* use [fetch-content](https://cmake.org/cmake/help/latest/module/FetchContent.html) instead to submodule to avoid having the sources checked out in the repo (but still have a fixed version from git, just using cmake instead of submodules).

FetchContent works great, thanks for pointing me to this. It took a little tweaking but now I got it integrated with the CMake build. The one downside is that this will not work with bazel etc, but those build scripts do not use GC anyway...

The only thing left to fix now is the MacOS build.

davidbolvansky · 2023-03-21T15:45:32Z

CMakeLists.txt

+    # add_definitions("-DNO_MARKER_SPECIAL_SIGMASK")
+  endif()
+  if(BUILD_STATIC_RELEASE)
+    set(enable_threads OFF CACHE BOOL "Support threads.")


I have not been able to successfully compile it with the thread library.

Then we can expect performance regressions in static builds (ones we give to customers :/)

~~Not quite sure, if I remember correctly the Ubuntu packages do not have THREADS defined by default.~~

Nevermind. Looking at the configure file threads are enabled.

E.g., if you try to use GC_allow_register_threads it will complain.

https://stackoverflow.com/questions/35116327/when-g-static-link-pthread-cause-segmentation-fault-why

I am running into this problem. Somehow pthread is not linked correctly. I have not found an order to make this work.
This change is possibly related, which makes things quite confusing.
https://developers.redhat.com/articles/2021/12/17/why-glibc-234-removed-libpthread#the_developer_view
Dropping -static-libgcc -static-libstdc++ does not seem to help either.

What does this option mean? Is it enabling support for running multi-threaded programs with libGC, or is it enabling parallel runs of garbage collecting threads?

I believe the first one.
https://github.com/ivmai/bdwgc/blob/1c2f2c679927538fa4969f85c67fafc3b1dddb23/docs/scale.md#garbage-collector-scalability
The second one can be enable with PARALLEL_MARK.

davidbolvansky · 2023-03-21T15:50:18Z

libGC interacts badly with the current version of the Z3 solver (for example, #3927). A more recent version of Z3 (4.12.0) does not work at all.

Oh, that is a quite serious issue for the future. Is there a bug report for this problem on z3 issue tracker? Could we bisect problematic commits?

fruffy · 2023-03-21T16:16:20Z

libGC interacts badly with the current version of the Z3 solver (for example, #3927). A more recent version of Z3 (4.12.0) does not work at all.

Oh, that is a quite serious issue for the future. Is there a bug report for this problem on z3 issue tracker? Could we bisect problematic commits?

I have not done a deep dive why Z3 is causing these issues but it could be related to Z3Prover/z3#6572. glibc 2.34 removes pthread which could have some effect. This might get fixed.

fruffy · 2023-03-21T16:40:09Z

The MacOS build fails with a bizarre error. Quite difficult to debug.

Edit: It looks like the problem was that libbmv2 was not linked statically. I do not quite understand why that causes issues.

cmake/bdwgc.patch

cmake/BDWGC.cmake

ChrisDodd

Looks like this is still getting hung up on the fedora and old Ubuntu issues, but hopefully doesn't interoduce any more hard-to-debug dependencies.

fruffy · 2024-05-08T00:19:15Z

Looks like this is still getting hung up on the fedora and old Ubuntu issues, but hopefully doesn't interoduce any more hard-to-debug dependencies.

#4623 introduced the Ubuntu 18.04 failure. Not sure why... for some reason this version of boost pulls in hash_combine for the cpp_int.

The Fedora failure should be fixed in #4644.

vlstill · 2024-05-09T13:47:26Z

What is the reason to use submodule instead of FetchContent?

vlstill · 2024-05-09T13:54:04Z

What is the reason to use submodule instead of FetchContent?

I see that is just an out-of-date top of the PR, could you please update it?

vlstill

A few little things.

vlstill · 2024-05-09T13:48:37Z

CMakeLists.txt

+# More utilities.
+include (CheckIncludeFile)
+include (CheckIncludeFileCXX)
+include (CheckFunctionExists)
+include (FindPythonModule)
+INCLUDE(CPack)


there is not much consistency in the CMake files, but still:

Suggested change

# More utilities.

include (CheckIncludeFile)

include (CheckIncludeFileCXX)

include (CheckFunctionExists)

include (FindPythonModule)

INCLUDE(CPack)

# More utilities.

include(CheckIncludeFile)

include(CheckIncludeFileCXX)

include(CheckFunctionExists)

include(FindPythonModule)

include(CPack)

vlstill · 2024-05-09T13:56:00Z

cmake/BDWGC.cmake

+    set(FETCHCONTENT_QUIET_PREV ${FETCHCONTENT_QUIET})
+    set(BUILD_SHARED_LIBS_PREV ${BUILD_SHARED_LIBS})
+    set(FETCHCONTENT_QUIET OFF)
+    set(BUILD_SHARED_LIBS OFF CACHE BOOL "")


nitpick: This probably belongs more to the GC options below then to the fetchcontent options in this block.

vlstill · 2024-05-09T13:58:17Z

cmake/BDWGC.cmake

+
+    # Add some extra compile definitions which allow us to use our customizations.
+    target_compile_definitions(gc PUBLIC
+      HANDLE_FORK ENABLE_DISCLAIM GC_USE_DLOPEN_WRAP SKIP_CPP_DEFINITIONS


What does GC_USE_DLOPEN_WRAP do?
And what is SKIP_CPP_DEFINTIONS?

It would be great if you added a comment why each of these options is needed.

Will do.
For context on DLOPEN_WRAP https://github.com/ivmai/bdwgc/blob/master/docs/README.macros#L416 and ivmai/bdwgc#561 (comment)
It seems to help in multi-threaded environments.

For an explanation of SKIP_CPP_DEFINTIONS see #3930 (comment)

GC_USE_DLOPEN_WRAP Causes the collector to redefine malloc and
intercepted pthread routines with their real names, and causes it to use
dlopen and dlsym to refer to the original versions. This makes it possible
to build an LD_PRELOADable malloc replacement library.

Doesn't this interfere with our own re-declaration of malloc? Or maybe the BDWGC one is not linked because there is already malloc defined by our binary...

GC_USE_DLOPEN_WRAP Causes the collector to redefine malloc and
intercepted pthread routines with their real names, and causes it to use
dlopen and dlsym to refer to the original versions. This makes it possible
to build an LD_PRELOADable malloc replacement library.

Doesn't this interfere with our own re-declaration of malloc? Or maybe the BDWGC one is not linked because there is already malloc defined by our binary...

I think it would be ok, as malloc ends up redirected to the gc regardless, but the main reason we have redeclarations of malloc is that the default libgc builds on Ubuntu and others DON'T use GC_USE_DLOPEN_WRAP. With it, we should be able to remove our malloc replacements.

yes, I was wondering if we could actually get rid of our gc.cpp (we would have to be willing to drop support for distribution version of libgc in most distros).

@vlstill I'm not sure we can drop on Darwin. Though, it's worth investigating.

fruffy · 2024-05-09T15:28:29Z

@asl Planning to merge this soon. Lmk if you have any concerns here.

fruffy changed the title ~~Make the Boehm GC a Git submodule. Simplify gc.cpp~~ Make the Boehm-Demers-Weiser Garbage Collector a Git submodule. Simplify gc.cpp Mar 19, 2023

fruffy force-pushed the fruffy/refactor_gc branch 3 times, most recently from 400d314 to 577b32d Compare March 20, 2023 02:59

fruffy force-pushed the fruffy/refactor_gc branch 5 times, most recently from cbf1073 to 3242665 Compare March 20, 2023 23:43

fruffy force-pushed the fruffy/refactor_gc branch 6 times, most recently from 88dfc05 to 8feb636 Compare March 21, 2023 15:19

vlstill mentioned this pull request Mar 21, 2023

p4tool: Fix build with Z3 installed in non-standard path #3932

Merged

davidbolvansky reviewed Mar 21, 2023

View reviewed changes

fruffy force-pushed the fruffy/refactor_gc branch from 8feb636 to e6590f5 Compare March 21, 2023 16:12

fruffy force-pushed the fruffy/refactor_gc branch from e6590f5 to 43e39c7 Compare March 21, 2023 16:18

fruffy force-pushed the fruffy/refactor_gc branch 4 times, most recently from 7ba4306 to 44fe150 Compare March 21, 2023 17:52

fruffy requested review from vlstill, asl and ChrisDodd April 5, 2024 19:29

fruffy commented Apr 5, 2024

View reviewed changes

cmake/bdwgc.patch Show resolved Hide resolved

fruffy requested review from asl, vlstill and ChrisDodd and removed request for asl, vlstill and ChrisDodd April 29, 2024 21:48

ChrisDodd reviewed Apr 30, 2024

View reviewed changes

cmake/BDWGC.cmake Show resolved Hide resolved

cmake/BDWGC.cmake Show resolved Hide resolved

fruffy mentioned this pull request May 7, 2024

Consider a centralized repository for dependency management? #4654

Open

fruffy force-pushed the fruffy/refactor_gc branch from 99fcd0f to b3cc43c Compare May 7, 2024 12:25

fruffy requested a review from ChrisDodd May 7, 2024 23:20

ChrisDodd approved these changes May 8, 2024

View reviewed changes

vlstill reviewed May 9, 2024

View reviewed changes

Make the Boehm GC a Git submodule.

693bc24

fruffy force-pushed the fruffy/refactor_gc branch 2 times, most recently from 32e042a to 9a983af Compare May 9, 2024 15:21

Review comments.

b65cf35

fruffy force-pushed the fruffy/refactor_gc branch from 9a983af to b65cf35 Compare May 9, 2024 21:25

vlstill approved these changes May 10, 2024

View reviewed changes

fruffy added this pull request to the merge queue May 13, 2024

Merged via the queue into main with commit b4d8e06 May 13, 2024
16 of 17 checks passed

fruffy deleted the fruffy/refactor_gc branch May 13, 2024 21:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Link against the Boehm-Demers-Weiser Garbage Collector using FetchContent. #3930

Link against the Boehm-Demers-Weiser Garbage Collector using FetchContent. #3930

fruffy commented Mar 19, 2023 •

edited

Loading

vlstill commented Mar 20, 2023

fruffy commented Mar 20, 2023 •

edited

Loading

vlstill commented Mar 20, 2023

fruffy commented Mar 21, 2023

davidbolvansky Mar 21, 2023

fruffy Mar 21, 2023

davidbolvansky Mar 21, 2023

fruffy Mar 23, 2023 •

edited

Loading

fruffy Mar 23, 2023

fruffy Mar 23, 2023

vlstill Mar 23, 2023

fruffy Mar 23, 2023 •

edited

Loading

davidbolvansky commented Mar 21, 2023 •

edited

Loading

fruffy commented Mar 21, 2023

fruffy commented Mar 21, 2023 •

edited

Loading

ChrisDodd left a comment

fruffy commented May 8, 2024

vlstill commented May 9, 2024

vlstill commented May 9, 2024

vlstill left a comment

vlstill May 9, 2024

vlstill May 9, 2024

vlstill May 9, 2024

fruffy May 9, 2024

vlstill May 9, 2024

ChrisDodd May 10, 2024

vlstill May 10, 2024

asl May 10, 2024

fruffy commented May 9, 2024

Link against the Boehm-Demers-Weiser Garbage Collector using FetchContent. #3930

Link against the Boehm-Demers-Weiser Garbage Collector using FetchContent. #3930

Conversation

fruffy commented Mar 19, 2023 • edited Loading

vlstill commented Mar 20, 2023

fruffy commented Mar 20, 2023 • edited Loading

vlstill commented Mar 20, 2023

fruffy commented Mar 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fruffy Mar 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fruffy Mar 23, 2023 • edited Loading

Choose a reason for hiding this comment

davidbolvansky commented Mar 21, 2023 • edited Loading

fruffy commented Mar 21, 2023

fruffy commented Mar 21, 2023 • edited Loading

ChrisDodd left a comment

Choose a reason for hiding this comment

fruffy commented May 8, 2024

vlstill commented May 9, 2024

vlstill commented May 9, 2024

vlstill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fruffy commented May 9, 2024

fruffy commented Mar 19, 2023 •

edited

Loading

fruffy commented Mar 20, 2023 •

edited

Loading

fruffy Mar 23, 2023 •

edited

Loading

fruffy Mar 23, 2023 •

edited

Loading

davidbolvansky commented Mar 21, 2023 •

edited

Loading

fruffy commented Mar 21, 2023 •

edited

Loading