Page MenuHomePhabricator

[libc] add option to use SCUDO as the allocator
ClosedPublic

Authored by michaelrj on Wed, Jul 21, 3:22 PM.

Details

Summary

This patch adds LLVM_LIBC_INCLUDE_SCUDO as a flag. When enabled it
should link in the standalone version of SCUDO as the allocator for LLVM
libc.

Diff Detail

Event Timeline

michaelrj created this revision.Wed, Jul 21, 3:22 PM
michaelrj requested review of this revision.Wed, Jul 21, 3:22 PM

As a follow up to this patch, we should add some kind of integration test to make sure that what is getting packaged is actually usable and works as expected.

libc/cmake/modules/LLVMLibCLibraryRules.cmake
82

Before we extract the objects from the dep, we should verify that the dep is an object library. Else, report a failure. One way could be to use the TYPE property to decide if the dep is an OBJECT_LIBRARY: https://cmake.org/cmake/help/v3.13/prop_tgt/TYPE.html

Most of this is already done in the code above from lines 65 to 78. So, we might just be able make use of the DEPENDS option with small adjustments.

michaelrj updated this revision to Diff 360640.Wed, Jul 21, 4:35 PM
michaelrj marked an inline comment as done.

added a comment explaining EXT_DEPS.

libc/cmake/modules/LLVMLibCLibraryRules.cmake
82

We can't check the properties of the dep since it may not be defined yet. The libc cmake scripts are evaluated before the compiler-rt scripts. This means we can't check the type of anything in EXT_DEPS, which means that combining EXT_DEPS and DEPENDS wouldn't work.

cheng.w added inline comments.
libc/CMakeLists.txt
74

What about set this option ON by default?

sivachandra added a comment.EditedWed, Jul 21, 9:25 PM

@cryptoad - This adds a build level dependency across projects. Do you see any problems which this arrangement? We will also start testing this arrangement via integration tests on the LLVM libc buildbot workers.

libc/CMakeLists.txt
74

Not sure if ON or OFF is the correct default value while LLVM libc is not complete. When it is complete enough, I think default ON makes more sense. That said, if you have a reason to make it ON do share here.

@cryptoad - This adds a build level dependency across projects. Do you see any problems which this arrangement? We will also start testing this arrangement via integration tests on the LLVM libc buildbot workers.

No problem with this, thank you!

michaelrj added inline comments.Thu, Jul 22, 10:27 AM
libc/CMakeLists.txt
74

I like OFF as a default value, so that if someone is trying to build LLVM libc for the first time they don't get unexpected errors due to not including compiler-rt.

cheng.w added inline comments.Thu, Jul 22, 6:17 PM
libc/CMakeLists.txt
74

Agree. I have no other reasons.

sivachandra accepted this revision.Fri, Jul 23, 10:05 AM
This revision is now accepted and ready to land.Fri, Jul 23, 10:05 AM
michaelrj marked 3 inline comments as done.Fri, Jul 23, 10:25 AM
This revision was automatically updated to reflect the committed changes.