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

Create a new ddcommon-ffi crate and factorise generic ffi code inside it #24

Merged
merged 20 commits into from
Jul 19, 2022
Merged
Show file tree
Hide file tree
Changes from all 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
17 changes: 17 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@ members = [
"ddprof-ffi",
"ddprof-profiles",
"ddcommon",
"ddcommon-ffi",
Comment on lines 10 to +11
Copy link
Member

Choose a reason for hiding this comment

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

I was talking a bit to Levi about how many packages we have yesterday and I got to wondering: why exactly do we need to have the ffi packages separated from the non-ffi ones?

I realize that if you're consuming these things from Rust, then you don't care about the ffi parts of the code. But if you're just using Rust, can't the compiler realize you're not using the ffi bits and just drop them from the final binary?

E.g. what would be the problem if we folded every -ffi package back into its "parent"?

(This may be an incredibly stupid question, apologies!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, not that bad of an idea. I don't think it would cause issues.

We could probably have an ffi module in ddcommon, ddprof, etc.. that exposes only what needs to be exposed.

"ddtelemetry",
"tools",
]
# https://doc.rust-lang.org/cargo/reference/resolver.html#feature-resolver-version-2
resolver = "2"
Expand Down
2 changes: 1 addition & 1 deletion LICENSE-3rdparty.yml
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
---
root_name: "ddprof, ddprof-exporter, ddcommon, ddprof-profiles, ddprof-ffi"
root_name: "ddprof, ddprof-exporter, ddcommon, ddprof-profiles, ddprof-ffi, ddcommon-ffi, ddtelemetry"
Copy link
Member

Choose a reason for hiding this comment

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

Should tools also go in here? (and if not, perhaps we could have a comment explaining why?)

third_party_libraries:
- package_name: aho-corasick
package_version: 0.7.18
Expand Down
2 changes: 1 addition & 1 deletion cmake/DDProfConfig.cmake.in
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ else ()
set(DDProf_ROOT "${CMAKE_CURRENT_SOURCE_DIR}/..")
endif ()

find_path(DDProf_INCLUDE_DIR ddprof/ffi.h
find_path(DDProf_INCLUDE_DIR datadog/profiling.h
Copy link
Member

Choose a reason for hiding this comment

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

Should we take the opportunity to rename this file LibdatadogConfig.cmake.in and possibly also adjust the stuff we call DDProf in this file?

(On the other hand I realize this PR already includes quite a lot of things, so I'm also OK with leaving this for later -- it's not like we'll forget about it, it's pretty obvious and easy to find)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Leaving it for latter

HINTS ${DDProf_ROOT}/include
)

Expand Down
14 changes: 14 additions & 0 deletions ddcommon-ffi/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
# Unless explicitly stated otherwise all files in this repository are licensed under the Apache License Version 2.0.
# This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2021-Present Datadog, Inc.

[package]
name = "ddcommon-ffi"
Comment on lines +4 to +5
Copy link
Member

Choose a reason for hiding this comment

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

⬆️ I think we're missing the usual licence two-liner here

version = "0.7.0-rc.1"
edition = "2021"
license = "Apache-2.0"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
ddcommon = { path = "../ddcommon", version = "0.7.0-rc.1" }
anyhow = "1.0"
51 changes: 51 additions & 0 deletions ddcommon-ffi/cbindgen.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
# Unless explicitly stated otherwise all files in this repository are licensed under the Apache License Version 2.0.
# This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2021-Present Datadog, Inc.

language = "C"
tab_width = 2
header = """// Unless explicitly stated otherwise all files in this repository are licensed under the Apache License Version 2.0.
// This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2021-Present Datadog, Inc.
"""
include_guard = "DDOG_COMMON_H"
Copy link
Member

Choose a reason for hiding this comment

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

Minor: Should this be DATADOG_COMMON_H to match the path of the file in the filesystem? (Same note applies to other files in the PR as well)

style = "both"

no_includes = true
sys_includes = ["stdbool.h", "stddef.h", "stdint.h"]

after_includes = """

#if defined(_MSC_VER)
#define DDOG_CHARSLICE_C(string) \\
/* NOTE: Compilation fails if you pass in a char* instead of a literal */ {.ptr = "" string, .len = sizeof(string) - 1}
#else
#define DDOG_CHARSLICE_C(string) \\
/* NOTE: Compilation fails if you pass in a char* instead of a literal */ ((ddog_CharSlice){ .ptr = "" string, .len = sizeof(string) - 1 })
Comment on lines +17 to +22
Copy link
Member

Choose a reason for hiding this comment

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

Interesting that we needed the top variant for Windows; I also run into issues with really old versions of GCC (https://github.com/DataDog/dd-trace-rb/blob/master/ext/ddtrace_profiling_native_extension/stack_recorder.h#L16) in some cases...

#endif

#if defined(__cplusplus) && (__cplusplus >= 201703L)
# define DDOG_CHECK_RETURN [[nodiscard]]
#elif defined(_Check_return_) /* SAL */
# define DDOG_CHECK_RETURN _Check_return_
#elif (defined(__has_attribute) && __has_attribute(warn_unused_result)) || \\
(defined(__GNUC__) && (__GNUC__ >= 4))
# define DDOG_CHECK_RETURN __attribute__((__warn_unused_result__))
#else
# define DDOG_CHECK_RETURN
#endif"""

[export]
prefix = "ddog_"

[export.mangle]
rename_types="SnakeCase"

[enum]
prefix_with_name = true
rename_variants = "ScreamingSnakeCase"

[fn]
must_use = "DDOG_CHECK_RETURN"

[parse]
parse_deps = true
include = ["ddcommon"]
9 changes: 9 additions & 0 deletions ddcommon-ffi/src/lib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// Unless explicitly stated otherwise all files in this repository are licensed under the Apache License Version 2.0.
// This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2021-Present Datadog, Inc.

pub mod slice;
pub mod tags;
pub mod vec;

pub use slice::Slice;
pub use vec::Vec;
2 changes: 1 addition & 1 deletion ddprof-ffi/src/slice.rs → ddcommon-ffi/src/slice.rs
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ impl<'a> From<&'a str> for Slice<'a, c_char> {
mod test {
use std::os::raw::c_char;

use crate::*;
use crate::slice::*;

#[test]
fn slice_from_into_slice() {
Expand Down
32 changes: 14 additions & 18 deletions ddprof-ffi/src/tags.rs → ddcommon-ffi/src/tags.rs
Original file line number Diff line number Diff line change
@@ -1,17 +1,17 @@
// Unless explicitly stated otherwise all files in this repository are licensed under the Apache License Version 2.0.
// This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2022-Present Datadog, Inc.

use crate::{AsBytes, CharSlice};
use crate::{slice::AsBytes, slice::CharSlice};
use ddcommon::tag::{parse_tags, Tag};

#[must_use]
#[no_mangle]
pub extern "C" fn ddprof_ffi_Vec_tag_new() -> crate::Vec<Tag> {
pub extern "C" fn ddog_Vec_tag_new() -> crate::Vec<Tag> {
crate::Vec::default()
}

#[no_mangle]
pub extern "C" fn ddprof_ffi_Vec_tag_drop(_: crate::Vec<Tag>) {}
pub extern "C" fn ddog_Vec_tag_drop(_: crate::Vec<Tag>) {}

#[repr(C)]
pub enum PushTagResult {
Expand All @@ -20,7 +20,7 @@ pub enum PushTagResult {
}

#[no_mangle]
pub extern "C" fn ddprof_ffi_PushTagResult_drop(_: PushTagResult) {}
pub extern "C" fn ddog_PushTagResult_drop(_: PushTagResult) {}

/// Creates a new Tag from the provided `key` and `value` by doing a utf8
/// lossy conversion, and pushes into the `vec`. The strings `key` and `value`
Expand All @@ -32,7 +32,7 @@ pub extern "C" fn ddprof_ffi_PushTagResult_drop(_: PushTagResult) {}
/// `.len` properties claim.
#[must_use]
#[no_mangle]
pub unsafe extern "C" fn ddprof_ffi_Vec_tag_push(
pub unsafe extern "C" fn ddog_Vec_tag_push(
vec: &mut crate::Vec<Tag>,
key: CharSlice,
value: CharSlice,
Expand All @@ -59,7 +59,7 @@ pub struct ParseTagsResult {
/// .len property.
#[must_use]
#[no_mangle]
pub unsafe extern "C" fn ddprof_ffi_Vec_tag_parse(string: CharSlice) -> ParseTagsResult {
pub unsafe extern "C" fn ddog_Vec_tag_parse(string: CharSlice) -> ParseTagsResult {
let string = string.to_utf8_lossy();
let (tags, error) = parse_tags(string.as_ref());
ParseTagsResult {
Expand All @@ -75,22 +75,21 @@ mod tests {
#[test]
fn empty_tag_name() {
unsafe {
let mut tags = ddprof_ffi_Vec_tag_new();
let result =
ddprof_ffi_Vec_tag_push(&mut tags, CharSlice::from(""), CharSlice::from("woof"));
let mut tags = ddog_Vec_tag_new();
let result = ddog_Vec_tag_push(&mut tags, CharSlice::from(""), CharSlice::from("woof"));
assert!(!matches!(result, PushTagResult::Ok));
}
}

#[test]
fn test_lifetimes() {
let mut tags = ddprof_ffi_Vec_tag_new();
let mut tags = ddog_Vec_tag_new();
unsafe {
// make a string here so it has a scoped lifetime
let key = String::from("key1");
{
let value = String::from("value1");
let result = ddprof_ffi_Vec_tag_push(
let result = ddog_Vec_tag_push(
&mut tags,
CharSlice::from(key.as_str()),
CharSlice::from(value.as_str()),
Expand All @@ -106,12 +105,9 @@ mod tests {
#[test]
fn test_get() {
unsafe {
let mut tags = ddprof_ffi_Vec_tag_new();
let result = ddprof_ffi_Vec_tag_push(
&mut tags,
CharSlice::from("sound"),
CharSlice::from("woof"),
);
let mut tags = ddog_Vec_tag_new();
let result =
ddog_Vec_tag_push(&mut tags, CharSlice::from("sound"), CharSlice::from("woof"));
assert!(matches!(result, PushTagResult::Ok));
assert_eq!(1, tags.len());
assert_eq!("sound:woof", tags.get(0).unwrap().to_string());
Expand All @@ -123,7 +119,7 @@ mod tests {
let dd_tags = "env:staging:east, tags:, env_staging:east"; // contains an error

// SAFETY: CharSlices from Rust strings are safe.
let result = unsafe { ddprof_ffi_Vec_tag_parse(CharSlice::from(dd_tags)) };
let result = unsafe { ddog_Vec_tag_parse(CharSlice::from(dd_tags)) };
assert_eq!(2, result.tags.len());
assert_eq!("env:staging:east", result.tags.get(0).unwrap().to_string());
assert_eq!("env_staging:east", result.tags.get(1).unwrap().to_string());
Expand Down
2 changes: 1 addition & 1 deletion ddprof-ffi/src/vec.rs → ddcommon-ffi/src/vec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

extern crate alloc;

use crate::Slice;
use crate::slice::Slice;
use std::io::Write;
use std::marker::PhantomData;
use std::mem::ManuallyDrop;
Expand Down
1 change: 1 addition & 0 deletions ddprof-ffi/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -18,5 +18,6 @@ ddprof-exporter = { path = "../ddprof-exporter", version = "0.7.0-rc.1" }
ddprof-profiles = { path = "../ddprof-profiles", version = "0.7.0-rc.1" }
hyper = {version = "0.14", default-features = false}
ddcommon = { path = "../ddcommon", version = "0.7.0-rc.1" }
ddcommon-ffi = { path = "../ddcommon-ffi", version = "0.7.0-rc.1" }
libc = "0.2"
tokio-util = "0.7.1"
30 changes: 5 additions & 25 deletions ddprof-ffi/cbindgen.toml
Original file line number Diff line number Diff line change
Expand Up @@ -6,35 +6,15 @@ tab_width = 2
header = """// Unless explicitly stated otherwise all files in this repository are licensed under the Apache License Version 2.0.
// This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2021-Present Datadog, Inc.
"""
include_guard = "DDPROF_FFI_H"
include_guard = "DDOG_PROFILING_H"
style = "both"

no_includes = true
sys_includes = ["stdbool.h", "stddef.h", "stdint.h"]

after_includes = """

#if defined(_MSC_VER)
#define DDPROF_FFI_CHARSLICE_C(string) \\
/* NOTE: Compilation fails if you pass in a char* instead of a literal */ {.ptr = "" string, .len = sizeof(string) - 1}
#else
#define DDPROF_FFI_CHARSLICE_C(string) \\
/* NOTE: Compilation fails if you pass in a char* instead of a literal */ ((ddprof_ffi_CharSlice){ .ptr = "" string, .len = sizeof(string) - 1 })
#endif

#if defined(__cplusplus) && (__cplusplus >= 201703L)
# define DD_CHECK_RETURN [[nodiscard]]
#elif defined(_Check_return_) /* SAL */
# define DD_CHECK_RETURN _Check_return_
#elif (defined(__has_attribute) && __has_attribute(warn_unused_result)) || \\
(defined(__GNUC__) && (__GNUC__ >= 4))
# define DD_CHECK_RETURN __attribute__((__warn_unused_result__))
#else
# define DD_CHECK_RETURN
#endif"""
includes = ["datadog/common.h"]

[export]
prefix = "ddprof_ffi_"
prefix = "ddog_"

[export.mangle]
rename_types="SnakeCase"
Expand All @@ -44,8 +24,8 @@ prefix_with_name = true
rename_variants = "ScreamingSnakeCase"

[fn]
must_use = "DD_CHECK_RETURN"
must_use = "DDOG_CHECK_RETURN"

[parse]
parse_deps = true
include = ["ddcommon", "ddprof-exporter", "ddprof-profiles", "ux"]
include = ["ddcommon", "ddcommon-ffi", "ddprof-exporter", "ddprof-profiles", "ux"]
Loading