Page MenuHomePhabricator

[CMAKE] Improve the FindJemalloc module
ClosedPublic

Authored by Fabien on May 13 2020, 07:37.

Details

Summary

Prefer the PIC version when available, and pull the dependencies as
needed.

Test Plan
cmake -GNinja .. -DUSE_JEMALLOC_EXPERIMENTAL=ON

On Debian, check in the cmake output that libdl is required.

ninja all check

Require D6038:

cmake -GNinja .. -DCMAKE_TOOLCHAIN_FILE=../cmake/platforms/Linux64.cmake -DUSE_JEMALLOC_EXPERIMENTAL=ON

Check in the cmake output that libdl is NOT required.

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Fabien created this revision.May 13 2020, 07:37
Herald added a reviewer: Restricted Project. · View Herald TranscriptMay 13 2020, 07:37
Fabien requested review of this revision.May 13 2020, 07:37
deadalnix added inline comments.
cmake/modules/FindJemalloc.cmake
49 ↗(On Diff #19978)

Is there a way to detect if dl is needed? Depending on jemalloc's build options it may or may not be a dependency.

Fabien planned changes to this revision.May 13 2020, 14:18
Fabien edited the test plan for this revision. (Show Details)May 13 2020, 14:21
Fabien updated this revision to Diff 19997.May 13 2020, 14:22

Try to build without libdl and link it only if it fails.

deadalnix requested changes to this revision.May 13 2020, 14:31
deadalnix added inline comments.
cmake/modules/FindJemalloc.cmake
56 ↗(On Diff #19997)

Why not provide a source file directly rather than go with this strange dance?

61 ↗(On Diff #19997)

You probably want to check for a dl related error, because if not, then you'll add dl as a dependency numerous times and then fail to build at the end.

79 ↗(On Diff #19997)

You want to check if libdl is missing. Stating it this way and changing the code accordingly will make everything both clearer and more robust.

This revision now requires changes to proceed.May 13 2020, 14:31
Fabien updated this revision to Diff 20000.May 13 2020, 15:17

Use a source file rather than generating it.
Check the output to make sure the failure is related to libdl missing.

deadalnix accepted this revision.May 13 2020, 23:08

There are a few problems with this. Because they are either trivials, or on the other may not have a solution, I'm accepting it this.

cmake/modules/FindJemalloc.cmake
67 ↗(On Diff #20000)

You might want to use something a bit more specific as regex.

71 ↗(On Diff #20000)

Presumably we verified this is missing, so filtering based on windows or not does not seems necessary.

75 ↗(On Diff #20000)

First, this is a confusing error message. You need to point out that jemalloc was found, but that we couldn't build a simple test proram with it. Solution are: verify jemalloc is installed properly or disable it.

In the case dl was missing, but there is also another problem, then this message won't show up and failure will happen at link time later. I guess this is okay.

This revision is now accepted and ready to land.May 13 2020, 23:08
Fabien added inline comments.May 13 2020, 23:28
cmake/modules/FindJemalloc.cmake
67 ↗(On Diff #20000)

This is difficult due to the translation of the message according to user's locale. Only the symbol name will remain identical.

71 ↗(On Diff #20000)

I kept it only as belt and suspenders but not a strong case, it can be removed.

Fabien updated this revision to Diff 20033.May 13 2020, 23:30

Address comments

This revision was automatically updated to reflect the committed changes.