Skip to content

Address sanitizers for Rust #325

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

Merged
merged 4 commits into from
Jul 6, 2023
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
29 changes: 24 additions & 5 deletions .github/composite/godot-itest/action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,11 @@ inputs:
default: ''
description: "Extra values for the RUSTFLAGS env var"

rust-target:
required: false
default: ''
description: "If provided, acts as an argument for '--target', and results in output files written to ./target/{target}"

with-llvm:
required: false
default: ''
Expand Down Expand Up @@ -118,11 +123,23 @@ runs:
shell: bash

- name: "Build gdext (itest)"
run: |
cargo build -p itest ${{ inputs.rust-extra-args }}
shell: bash
env:
RUSTFLAGS: ${{ inputs.rust-env-rustflags }}
TARGET: ${{ inputs.rust-target }}
run: |
targetArgs=""
if [[ -n "$TARGET" ]]; then
targetArgs="--target $TARGET"
fi

cargo build -p itest ${{ inputs.rust-extra-args }} $targetArgs

# Instead of modifying .gdextension, rename the output directory
if [[ -n "$TARGET" ]]; then
rm -rf target/debug
mv target/$TARGET/debug target
fi
shell: bash

# This step no longer fails if there's a diff, as we expect header to be forward-compatible; instead issues a warning
# However, still fails if patch cannot be applied (conflict).
Expand Down Expand Up @@ -162,8 +179,10 @@ runs:
run: |
cd itest/godot
echo "OUTCOME=itest" >> $GITHUB_ENV
$GODOT4_BIN --headless ${{ inputs.godot-args }} 2>&1 | tee "${{ runner.temp }}/log.txt" | tee >(grep "SCRIPT ERROR:" -q && {
printf "\n -- Godot engine encountered error, abort...\n";
$GODOT4_BIN --headless ${{ inputs.godot-args }} 2>&1 \
| tee "${{ runner.temp }}/log.txt" \
| tee >(grep -E "SCRIPT ERROR:|Can't open dynamic library" -q && {
printf "\n::error::godot-itest: unrecoverable Godot error, abort...\n";
pkill godot
echo "OUTCOME=godot-runtime" >> $GITHUB_ENV
exit 2
Expand Down
6 changes: 3 additions & 3 deletions .github/composite/rust/action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,11 @@ runs:
steps:
- name: "Configure"
id: configure
env:
CS: ${{ inputs.components }}
run: |
echo "Rust cache shared-key: '${{ runner.os }}-${{ inputs.rust }}${{ inputs.cache-key }}'"
echo "components=$( for c in ${cs//,/ }; do echo -n ' --component' $c; done )" >> $GITHUB_OUTPUT
env:
cs: ${{ inputs.components }}
echo "components=$( for c in ${CS//,/ }; do echo -n ' --component' $c; done )" >> $GITHUB_OUTPUT
shell: bash

- name: "Rustup"
Expand Down
15 changes: 11 additions & 4 deletions .github/workflows/full-ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -202,17 +202,22 @@ jobs:
godot-prebuilt-patch: '4.1'

# Special Godot binaries compiled with AddressSanitizer/LeakSanitizer to detect UB/leaks.
# See also https://rustc-dev-guide.rust-lang.org/sanitizers.html.
#
# Additionally, the Godot source is patched to make dlclose() a no-op, as unloading dynamic libraries loses stacktrace and
# cause false positives like println!. See https://github.com/google/sanitizers/issues/89.
# The gcc version can possibly be removed later, as it is slower and needs a larger artifact than the clang one.
#
# There is also a gcc variant besides clang, which is currently not used.
- name: linux-memcheck-nightly
os: ubuntu-20.04
artifact-name: linux-memcheck-clang-nightly
godot-binary: godot.linuxbsd.editor.dev.x86_64.llvm.san
godot-args: -- --disallow-focus
rust-toolchain: nightly
rust-env-rustflags: -Zrandomize-layout
rust-env-rustflags: -Zrandomize-layout -Zsanitizer=address
rust-extra-args: --features godot/custom-godot
# Sanitizers can't build proc-macros and build scripts; with --target, cargo ignores RUSTFLAGS for those two.
rust-target: x86_64-unknown-linux-gnu
godot-prebuilt-patch: '4.1'

# Linux under Godot 4.0.x
Expand Down Expand Up @@ -246,8 +251,9 @@ jobs:
godot-binary: godot.linuxbsd.editor.dev.x86_64.llvm.san
godot-args: -- --disallow-focus
rust-toolchain: nightly
rust-env-rustflags: -Zrandomize-layout

rust-env-rustflags: -Zrandomize-layout -Zsanitizer=address
# Sanitizers can't build proc-macros and build scripts; with --target, cargo ignores RUSTFLAGS for those two.
rust-target: x86_64-unknown-linux-gnu

steps:
- uses: actions/checkout@v3
Expand All @@ -262,6 +268,7 @@ jobs:
rust-extra-args: ${{ matrix.rust-extra-args }}
rust-toolchain: ${{ matrix.rust-toolchain || 'stable' }}
rust-env-rustflags: ${{ matrix.rust-env-rustflags }}
rust-target: ${{ matrix.rust-target }}
with-llvm: ${{ contains(matrix.name, 'macos') && contains(matrix.rust-extra-args, 'custom-godot') }}
godot-check-header: ${{ matrix.godot-check-header }}

Expand Down
4 changes: 2 additions & 2 deletions godot-codegen/src/central_generator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -385,7 +385,7 @@ fn make_central_items(api: &ExtensionApi, build_config: &str, ctx: &mut Context)
.push(ident(&util::shout_to_pascal(name)));
result
.variant_op_enumerators_ord
.push(Literal::i32_unsuffixed(op.value));
.push(util::make_enumerator_ord(op.value));
}

for enum_ in api.global_enums.iter() {
Expand Down Expand Up @@ -499,7 +499,7 @@ fn make_enumerator(
let enumerator_name = &type_names.json_builtin_name;
let pascal_name = to_pascal_case(enumerator_name);
let rust_ty = to_rust_type(enumerator_name, None, ctx);
let ord = Literal::i32_unsuffixed(value);
let ord = util::make_enumerator_ord(value);

(ident(&pascal_name), rust_ty.to_token_stream(), ord)
}
Expand Down
2 changes: 1 addition & 1 deletion godot-codegen/src/class_generator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -684,7 +684,7 @@ fn make_notification_enum(
let mut notification_enumerators_ord = Vec::new();
for (constant_ident, constant_value) in all_constants {
notification_enumerators_pascal.push(constant_ident);
notification_enumerators_ord.push(constant_value);
notification_enumerators_ord.push(util::make_enumerator_ord(constant_value));
}

let code = quote! {
Expand Down
8 changes: 6 additions & 2 deletions godot-codegen/src/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ pub fn make_enum_definition(enum_: &Enum) -> TokenStream {

for enumerator in values {
let name = make_enumerator_name(&enumerator.name, &enum_.name);
let ordinal = Literal::i32_unsuffixed(enumerator.value);
let ordinal = make_enumerator_ord(enumerator.value);

enumerators.push(quote! {
pub const #name: Self = Self { ord: #ordinal };
Expand Down Expand Up @@ -120,7 +120,7 @@ pub fn make_enum_definition(enum_: &Enum) -> TokenStream {
let err = sys::PrimitiveConversionError::new(via);
let ord = i32::try_from(via).map_err(|_| err)?;
<Self as crate::obj::EngineEnum>::try_from_ord(ord).ok_or(err)
}
}

fn try_into_via(self) -> std::result::Result<Self::Via, Self::IntoViaError> {
Ok(<Self as crate::obj::EngineEnum>::ord(self).into())
Expand Down Expand Up @@ -486,6 +486,10 @@ pub fn parse_native_structures_format(input: &str) -> Option<Vec<NativeStructure
.collect()
}

pub(crate) fn make_enumerator_ord(ord: i32) -> Literal {
Literal::i32_unsuffixed(ord)
}

pub(crate) fn to_rust_expr(expr: &str, ty: &RustTy) -> TokenStream {
// println!("\n> to_rust_expr({expr}, {ty:?})");

Expand Down
9 changes: 4 additions & 5 deletions godot-core/src/builtin/meta/signature.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ pub trait VarcallSignatureTuple: PtrcallSignatureTuple {
// TODO(uninit) - can we use this for varcall/ptrcall?
// ret: sys::GDExtensionUninitializedVariantPtr
// ret: sys::GDExtensionUninitializedTypePtr
unsafe fn varcall<C: GodotClass>(
unsafe fn varcall(
instance_ptr: sys::GDExtensionClassInstancePtr,
args_ptr: *const sys::GDExtensionConstVariantPtr,
ret: sys::GDExtensionVariantPtr,
Expand All @@ -35,7 +35,7 @@ pub trait PtrcallSignatureTuple {

// Note: this method imposes extra bounds on GodotFfi, which may not be implemented for user types.
// We could fall back to varcalls in such cases, and not require GodotFfi categorically.
unsafe fn ptrcall<C: GodotClass>(
unsafe fn ptrcall(
instance_ptr: sys::GDExtensionClassInstancePtr,
args_ptr: *const sys::GDExtensionConstTypePtr,
ret: sys::GDExtensionTypePtr,
Expand Down Expand Up @@ -63,7 +63,6 @@ pub trait PtrcallSignatureTuple {
//
use crate::builtin::meta::*;
use crate::builtin::{FromVariant, ToVariant, Variant};
use crate::obj::GodotClass;

macro_rules! impl_varcall_signature_for_tuple {
(
Expand Down Expand Up @@ -102,7 +101,7 @@ macro_rules! impl_varcall_signature_for_tuple {


#[inline]
unsafe fn varcall<C : GodotClass>(
unsafe fn varcall(
instance_ptr: sys::GDExtensionClassInstancePtr,
args_ptr: *const sys::GDExtensionConstVariantPtr,
ret: sys::GDExtensionVariantPtr,
Expand Down Expand Up @@ -135,7 +134,7 @@ macro_rules! impl_ptrcall_signature_for_tuple {
type Params = ($($Pn,)*);
type Ret = $R;

unsafe fn ptrcall<C : GodotClass>(
unsafe fn ptrcall(
instance_ptr: sys::GDExtensionClassInstancePtr,
args_ptr: *const sys::GDExtensionConstTypePtr,
ret: sys::GDExtensionTypePtr,
Expand Down
6 changes: 2 additions & 4 deletions godot-core/src/macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,12 @@ macro_rules! gdext_call_signature_method {
(
ptrcall,
$Signature:ty,
$Class:ty,
$instance_ptr:ident, $args:ident, $ret:ident,
$func:expr,
$method_name:ident,
$ptrcall_type:path
) => {
<$Signature as $crate::builtin::meta::PtrcallSignatureTuple>::ptrcall::<$Class>(
<$Signature as $crate::builtin::meta::PtrcallSignatureTuple>::ptrcall(
$instance_ptr,
$args,
$ret,
Expand All @@ -31,12 +30,11 @@ macro_rules! gdext_call_signature_method {
(
varcall,
$Signature:ty,
$Class:ty,
$instance_ptr:ident, $args:ident, $ret:ident, $err:ident,
$func:expr,
$method_name:ident
) => {
<$Signature as $crate::builtin::meta::VarcallSignatureTuple>::varcall::<$Class>(
<$Signature as $crate::builtin::meta::VarcallSignatureTuple>::varcall(
$instance_ptr,
$args,
$ret,
Expand Down
8 changes: 2 additions & 6 deletions godot-macros/src/method_registration/register_method.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@ pub fn gdext_register_method(class_name: &Ident, method_signature: &TokenStream)

let wrapped_method = wrap_with_unpacked_params(class_name, &signature_info);

let varcall_func = get_varcall_func(class_name, method_name, &sig, &wrapped_method);
let ptrcall_func = get_ptrcall_func(class_name, method_name, &sig, &wrapped_method);
let varcall_func = get_varcall_func(method_name, &sig, &wrapped_method);
let ptrcall_func = get_ptrcall_func(method_name, &sig, &wrapped_method);

quote! {
{
Expand Down Expand Up @@ -72,7 +72,6 @@ pub fn gdext_register_method(class_name: &Ident, method_signature: &TokenStream)
}

fn get_varcall_func(
class_name: &Ident,
method_name: &Ident,
sig: &TokenStream,
wrapped_method: &TokenStream,
Expand All @@ -93,7 +92,6 @@ fn get_varcall_func(
godot::private::gdext_call_signature_method!(
varcall,
#sig,
#class_name,
instance_ptr,
args,
ret,
Expand All @@ -119,7 +117,6 @@ fn get_varcall_func(
}

fn get_ptrcall_func(
class_name: &Ident,
method_name: &Ident,
sig: &TokenStream,
wrapped_method: &TokenStream,
Expand All @@ -138,7 +135,6 @@ fn get_ptrcall_func(
godot::private::gdext_call_signature_method!(
ptrcall,
#sig,
#class_name,
instance_ptr,
args,
ret,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ pub fn gdext_virtual_method_callback(
godot::private::gdext_call_signature_method!(
ptrcall,
#sig,
#class_name,
instance_ptr,
args,
ret,
Expand Down