Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enforce levelization in libxrpl with CMake #5111

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions Builds/levelization/results/loops.txt
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,6 @@ Loop: test.jtx test.toplevel
Loop: test.jtx test.unit_test
test.unit_test == test.jtx

Loop: xrpl.basics xrpl.json
xrpl.json ~= xrpl.basics

Loop: xrpld.app xrpld.core
xrpld.app > xrpld.core

Expand Down
2 changes: 1 addition & 1 deletion Builds/levelization/results/ordering.txt
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
libxrpl.basics > xrpl.basics
libxrpl.basics > xrpl.protocol
libxrpl.crypto > xrpl.basics
libxrpl.json > xrpl.basics
libxrpl.json > xrpl.json
Expand Down Expand Up @@ -130,6 +129,7 @@ test.shamap > xrpl.protocol
test.toplevel > test.csf
test.toplevel > xrpl.json
test.unit_test > xrpl.basics
xrpl.json > xrpl.basics
xrpl.protocol > xrpl.basics
xrpl.protocol > xrpl.json
xrpl.resource > xrpl.basics
Expand Down
63 changes: 63 additions & 0 deletions cmake/RippledCore.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,57 @@ target_link_libraries(xrpl.libpb
gRPC::grpc++
)

# TODO: Clean up the number of library targets later.
add_library(xrpl.imports.main INTERFACE)
target_link_libraries(xrpl.imports.main INTERFACE
LibArchive::LibArchive
OpenSSL::Crypto
Ripple::boost
Ripple::opts
Ripple::syslibs
absl::random_random
date::date
ed25519::ed25519
secp256k1::secp256k1
xxHash::xxhash
)

include(add_module)
include(target_link_modules)

# Level 01
add_module(xrpl beast)
target_link_libraries(xrpl.libxrpl.beast PUBLIC
xrpl.imports.main
xrpl.libpb
)

# Level 02
add_module(xrpl basics)
target_link_libraries(xrpl.libxrpl.basics PUBLIC xrpl.libxrpl.beast)

# Level 03
add_module(xrpl json)
target_link_libraries(xrpl.libxrpl.json PUBLIC xrpl.libxrpl.basics)

add_module(xrpl crypto)
target_link_libraries(xrpl.libxrpl.crypto PUBLIC xrpl.libxrpl.basics)

# Level 04
add_module(xrpl protocol)
target_link_libraries(xrpl.libxrpl.protocol PUBLIC
xrpl.libxrpl.crypto
xrpl.libxrpl.json
)

# Level 05
add_module(xrpl resource)
target_link_libraries(xrpl.libxrpl.resource PUBLIC xrpl.libxrpl.protocol)

add_module(xrpl server)
target_link_libraries(xrpl.libxrpl.server PUBLIC xrpl.libxrpl.protocol)


add_library(xrpl.libxrpl)
set_target_properties(xrpl.libxrpl PROPERTIES OUTPUT_NAME xrpl)
if(unity)
Expand All @@ -60,7 +111,19 @@ file(GLOB_RECURSE sources CONFIGURE_DEPENDS
)
target_sources(xrpl.libxrpl PRIVATE ${sources})

target_link_modules(xrpl PUBLIC
basics
beast
crypto
json
protocol
resource
server
)

target_include_directories(xrpl.libxrpl
PRIVATE
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/src>
Comment on lines +125 to +126
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is probably not needed ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, upon some testing, this whole target_include_directories seem to be not needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not needed now that all of the subdirectories in libxrpl are modules. It was necessary as I migrated them one-by-one, and it could be necessary in the future if someone adds a non-module subdirectory. Could be nice if it "just works" without tampering with the CMake. What do you think?

PUBLIC
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>
$<INSTALL_INTERFACE:include>)
Expand Down
8 changes: 8 additions & 0 deletions cmake/RippledInstall.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,15 @@ install (
opts
ripple_syslibs
ripple_boost
xrpl.imports.main
xrpl.libpb
xrpl.libxrpl.basics
xrpl.libxrpl.beast
xrpl.libxrpl.crypto
xrpl.libxrpl.json
xrpl.libxrpl.protocol
xrpl.libxrpl.resource
xrpl.libxrpl.server
xrpl.libxrpl
EXPORT RippleExports
LIBRARY DESTINATION lib
Expand Down
37 changes: 37 additions & 0 deletions cmake/add_module.cmake
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
include(isolate_headers)

# Create an OBJECT library target named
#
# ${PROJECT_NAME}.lib${parent}.${name}
#
# with sources in src/lib${parent}/${name}
# and headers in include/${parent}/${name}
# that cannot include headers from other directories in include/
# unless they come through linked libraries.
#
# add_module(parent a)
# add_module(parent b)
# target_link_libraries(project.libparent.b PUBLIC project.libparent.a)
function(add_module parent name)
set(target ${PROJECT_NAME}.lib${parent}.${name})
add_library(${target} OBJECT)
file(GLOB_RECURSE sources CONFIGURE_DEPENDS
"${CMAKE_CURRENT_SOURCE_DIR}/src/lib${parent}/${name}/*.cpp"
)
target_sources(${target} PRIVATE ${sources})
target_include_directories(${target} PUBLIC
"$<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>"
)
isolate_headers(
${target}
"${CMAKE_CURRENT_SOURCE_DIR}/include"
"${CMAKE_CURRENT_SOURCE_DIR}/include/${parent}/${name}"
PUBLIC
)
isolate_headers(
${target}
"${CMAKE_CURRENT_SOURCE_DIR}/src"
"${CMAKE_CURRENT_SOURCE_DIR}/src/lib${parent}/${name}"
PRIVATE
)
Comment on lines +31 to +36
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is not needed ? Or my misunderstanding of how CMake works just showed :-D

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's in case a module has private headers, not installed, but included by other translation units. I don't know if any of our modules have these (yet), but I'm copying this implementation from my more generalized project.

endfunction()
46 changes: 46 additions & 0 deletions cmake/isolate_headers.cmake
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
# Consider include directory B nested under prefix A:
#
# /path/to/A/then/to/B/...
#
# Call C the relative path from A to B.
# C is what we want to write in `#include` directives:
#
# #include <then/to/B/...>
#
# Examples, all from the `jobqueue` module:
#
# - Library public headers:
# B = /include/xrpl/jobqueue
# A = /include/
# C = xrpl/jobqueue
#
# - Library private headers:
# B = /src/libxrpl/jobqueue
# A = /src/
# C = libxrpl/jobqueue
#
# - Test private headers:
# B = /tests/jobqueue
# A = /
# C = tests/jobqueue
#
# To isolate headers from each other,
# we want to create a symlink Y that points to B,
# within a subdirectory X of the `CMAKE_BINARY_DIR`,
# that has the same relative path C between X and Y,
# and then add X as an include directory of the target,
# sometimes `PUBLIC` and sometimes `PRIVATE`.
# The Cs are all guaranteed to be unique.
# We can guarantee a unique X per target by using
# `${CMAKE_CURRENT_BINARY_DIR}/include/${target}`.
#
# isolate_headers(target A B scope)
function(isolate_headers target A B scope)
file(RELATIVE_PATH C "${A}" "${B}")
set(X "${CMAKE_CURRENT_BINARY_DIR}/include/${target}")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think /include/ is misleading here. It's source file isolation feature, not includes.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest

-  set(X "${CMAKE_CURRENT_BINARY_DIR}/include/${target}")
+  set(X "${CMAKE_CURRENT_BINARY_DIR}/modules/${target}")

set(Y "${X}/${C}")
cmake_path(GET Y PARENT_PATH parent)
file(MAKE_DIRECTORY "${parent}")
file(CREATE_LINK "${B}" "${Y}" SYMBOLIC)
target_include_directories(${target} ${scope} "$<BUILD_INTERFACE:${X}>")
endfunction()
24 changes: 24 additions & 0 deletions cmake/target_link_modules.cmake
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
# Link a library to its modules (see: `add_module`)
# and remove the module sources from the library's sources.
#
# add_module(parent a)
# add_module(parent b)
# target_link_libraries(project.libparent.b PUBLIC project.libparent.a)
# add_library(project.libparent)
# target_link_modules(parent PUBLIC a b)
function(target_link_modules parent scope)
set(library ${PROJECT_NAME}.lib${parent})
foreach(name ${ARGN})
set(module ${library}.${name})
get_target_property(sources ${library} SOURCES)
list(LENGTH sources before)
get_target_property(dupes ${module} SOURCES)
list(LENGTH dupes expected)
list(REMOVE_ITEM sources ${dupes})
list(LENGTH sources after)
math(EXPR actual "${before} - ${after}")
message(STATUS "${module} with ${expected} sources took ${actual} sources from ${library}")
set_target_properties(${library} PROPERTIES SOURCES "${sources}")
target_link_libraries(${library} ${scope} ${module})
endforeach()
endfunction()
10 changes: 1 addition & 9 deletions include/xrpl/basics/Number.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
#ifndef RIPPLE_BASICS_NUMBER_H_INCLUDED
#define RIPPLE_BASICS_NUMBER_H_INCLUDED

#include <xrpl/basics/XRPAmount.h>
#include <cstdint>
#include <limits>
#include <ostream>
Expand Down Expand Up @@ -51,8 +50,6 @@ class Number
explicit Number(rep mantissa, int exponent);
explicit constexpr Number(rep mantissa, int exponent, unchecked) noexcept;

Number(XRPAmount const& x);

constexpr rep
mantissa() const noexcept;
constexpr int
Expand Down Expand Up @@ -88,8 +85,7 @@ class Number
static constexpr Number
lowest() noexcept;

explicit operator XRPAmount() const; // round to nearest, even on tie
explicit operator rep() const; // round to nearest, even on tie
explicit operator rep() const; // round to nearest, even on tie

friend constexpr bool
operator==(Number const& x, Number const& y) noexcept
Expand Down Expand Up @@ -206,10 +202,6 @@ inline Number::Number(rep mantissa) : Number{mantissa, 0}
{
}

inline Number::Number(XRPAmount const& x) : Number{x.drops()}
{
}

inline constexpr Number::rep
Number::mantissa() const noexcept
{
Expand Down
8 changes: 8 additions & 0 deletions include/xrpl/basics/SHAMapHash.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#define RIPPLE_BASICS_SHAMAP_HASH_H_INCLUDED

#include <xrpl/basics/base_uint.h>
#include <xrpl/basics/partitioned_unordered_map.h>

#include <ostream>

Expand Down Expand Up @@ -108,6 +109,13 @@
return !(x == y);
}

template <>
inline std::size_t
extract(SHAMapHash const& key)

Check warning on line 114 in include/xrpl/basics/SHAMapHash.h

View check run for this annotation

Codecov / codecov/patch

include/xrpl/basics/SHAMapHash.h#L114

Added line #L114 was not covered by tests
{
return *reinterpret_cast<std::size_t const*>(key.as_uint256().data());

Check warning on line 116 in include/xrpl/basics/SHAMapHash.h

View check run for this annotation

Codecov / codecov/patch

include/xrpl/basics/SHAMapHash.h#L116

Added line #L116 was not covered by tests
}

} // namespace ripple

#endif // RIPPLE_BASICS_SHAMAP_HASH_H_INCLUDED
12 changes: 12 additions & 0 deletions include/xrpl/basics/base_uint.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
#include <xrpl/basics/Slice.h>
#include <xrpl/basics/contract.h>
#include <xrpl/basics/hardened_hash.h>
#include <xrpl/basics/partitioned_unordered_map.h>
#include <xrpl/basics/strHex.h>
#include <xrpl/beast/utility/Zero.h>
#include <boost/endian/conversion.hpp>
Expand Down Expand Up @@ -630,6 +631,17 @@ operator<<(std::ostream& out, base_uint<Bits, Tag> const& u)
return out << to_string(u);
}

template <>
inline std::size_t
extract(uint256 const& key)
{
std::size_t result;
// Use memcpy to avoid unaligned UB
// (will optimize to equivalent code)
std::memcpy(&result, key.data(), sizeof(std::size_t));
return result;
}

#ifndef __INTELLISENSE__
static_assert(sizeof(uint128) == 128 / 8, "There should be no padding bytes");
static_assert(sizeof(uint160) == 160 / 8, "There should be no padding bytes");
Expand Down
19 changes: 16 additions & 3 deletions include/xrpl/basics/partitioned_unordered_map.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,12 @@
#ifndef RIPPLE_BASICS_PARTITIONED_UNORDERED_MAP_H
#define RIPPLE_BASICS_PARTITIONED_UNORDERED_MAP_H

#include <xrpl/beast/hash/uhash.h>

#include <cassert>
#include <functional>
#include <optional>
#include <string>
#include <thread>
#include <unordered_map>
#include <utility>
Expand All @@ -31,8 +34,18 @@
namespace ripple {

template <typename Key>
std::size_t
partitioner(Key const& key, std::size_t const numPartitions);
static std::size_t
extract(Key const& key)
{
return key;
}

template <>
inline std::size_t
extract(std::string const& key)
{
return ::beast::uhash<>{}(key);
}

template <
typename Key,
Expand Down Expand Up @@ -211,7 +224,7 @@ class partitioned_unordered_map
std::size_t
partitioner(Key const& key) const
{
return ripple::partitioner(key, partitions_);
return extract(key) % partitions_;
}

template <class T>
Expand Down
2 changes: 1 addition & 1 deletion include/xrpl/protocol/AmountConversions.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@
#define RIPPLE_PROTOCOL_AMOUNTCONVERSION_H_INCLUDED

#include <xrpl/basics/IOUAmount.h>
#include <xrpl/basics/XRPAmount.h>
#include <xrpl/protocol/STAmount.h>
#include <xrpl/protocol/XRPAmount.h>

#include <type_traits>

Expand Down
Loading
Loading