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

fix compilation on some CPUs and Windows #1901

Merged
merged 2 commits into from
Oct 8, 2024
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
20 changes: 11 additions & 9 deletions pgrx-bindgen/src/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -794,6 +794,7 @@ fn run_bindgen(
.default_non_copy_union_style(NonCopyUnionStyle::ManuallyDrop)
.wrap_static_fns(enable_cshim)
.wrap_static_fns_path(out_path.join("pgrx-cshim-static"))
.wrap_static_fns_suffix("__pgrx_cshim")
.generate()
.wrap_err_with(|| format!("Unable to generate bindings for pg{major_version}"))?;
let mut binding_str = bindings.to_string();
Expand Down Expand Up @@ -877,6 +878,14 @@ fn add_blocklists(bind: bindgen::Builder) -> bindgen::Builder {
.blocklist_function("range_table_walker")
.blocklist_function("raw_expression_tree_walker")
.blocklist_function("type_is_array")
// These structs contains array that is larger than 32
// so that `derive(Default)` would fail.
.blocklist_type("tagMONITORINFOEXA")
.blocklist_type("MONITORINFOEXA")
.blocklist_type("LPMONITORINFOEXA")
.blocklist_type("MONITORINFOEX")
.blocklist_type("LPMONITORINFOEX")
.blocklist_function("ua_.*") // this should be Windows's names
}

fn add_derives(bind: bindgen::Builder) -> bindgen::Builder {
Expand Down Expand Up @@ -1116,18 +1125,11 @@ fn apply_pg_guard(items: &Vec<syn::Item>) -> eyre::Result<proc_macro2::TokenStre
match item {
Item::ForeignMod(block) => {
let abi = &block.abi;
let (mut extern_funcs, mut others) = (Vec::new(), Vec::new());
block.items.iter().filter(|&item| !is_blocklisted_item(item)).cloned().for_each(
|item| match item {
ForeignItem::Fn(func) => extern_funcs.push(func),
item => others.push(item),
},
);
let items = block.items.iter().filter(|&item| !is_blocklisted_item(item));
out.extend(quote! {
#[pgrx_macros::pg_guard]
#abi { #(#extern_funcs)* }
#abi { #(#items)* }
});
out.extend(quote! { #abi { #(#others)* } });
}
_ => {
out.extend(item.into_token_stream());
Expand Down
32 changes: 29 additions & 3 deletions pgrx-macros/src/rewriter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ use std::str::FromStr;
use syn::punctuated::Punctuated;
use syn::spanned::Spanned;
use syn::{
FnArg, ForeignItem, ForeignItemFn, GenericParam, ItemFn, ItemForeignMod, Pat, Signature, Token,
Visibility,
Expr, ExprLit, FnArg, ForeignItem, ForeignItemFn, ForeignItemStatic, GenericParam, ItemFn,
ItemForeignMod, Lit, Pat, Signature, Token, Visibility,
};

macro_rules! format_ident {
Expand Down Expand Up @@ -126,6 +126,7 @@ fn foreign_item(item: ForeignItem, abi: &syn::Abi) -> syn::Result<proc_macro2::T

foreign_item_fn(&func, abi)
}
ForeignItem::Static(variable) => foreign_item_static(&variable, abi),
_ => Ok(quote! { #abi { #item } }),
}
}
Expand All @@ -135,19 +136,44 @@ fn foreign_item_fn(func: &ForeignItemFn, abi: &syn::Abi) -> syn::Result<proc_mac
let arg_list = rename_arg_list(&func.sig)?;
let arg_list_with_types = rename_arg_list_with_types(&func.sig)?;
let return_type = func.sig.output.clone();
let link_with_cshim = func.attrs.iter().any(|attr| match &attr.meta {
syn::Meta::NameValue(kv) if kv.path.get_ident().filter(|x| *x == "link_name").is_some() => {
if let Expr::Lit(ExprLit { lit: Lit::Str(value), .. }) = &kv.value {
value.value().ends_with("__pgrx_cshim")
} else {
false
}
}
_ => false,
});
let link = if link_with_cshim {
quote! {}
} else {
quote! { #[cfg_attr(target_os = "windows", link(name = "postgres"))] }
};

Ok(quote! {
#[inline]
#[track_caller]
pub unsafe fn #func_name ( #arg_list_with_types ) #return_type {
crate::ffi::pg_guard_ffi_boundary(move || {
#abi { #func }
#link #abi { #func }
#func_name(#arg_list)
})
}
})
}

fn foreign_item_static(
variable: &ForeignItemStatic,
abi: &syn::Abi,
) -> syn::Result<proc_macro2::TokenStream> {
let link = quote! { #[cfg_attr(target_os = "windows", link(name = "postgres"))] };
Ok(quote! {
#link #abi { #variable }
})
}

#[allow(clippy::cmp_owned)]
fn build_arg_list(sig: &Signature, suffix_arg_name: bool) -> syn::Result<proc_macro2::TokenStream> {
let mut arg_list = proc_macro2::TokenStream::new();
Expand Down
1 change: 1 addition & 0 deletions pgrx-pg-sys/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ serde.workspace = true # impls on pub types
# polyfill until #![feature(strict_provenance)] stabilizes
sptr = "0.3"

[target.'cfg(all(any(target_os = "linux", target_os = "macos"), any(target_arch = "x86_64", target_arch = "aarch64")))'.dependencies]
# Safer `sigsetjmp` and `siglongjmp`
cee-scape = "0.2"

Expand Down
42 changes: 14 additions & 28 deletions pgrx-pg-sys/pgrx-cshim.c
Original file line number Diff line number Diff line change
Expand Up @@ -10,42 +10,28 @@

#include "pgrx-cshim-static.c"

PGDLLEXPORT void *pgrx_list_nth(List *list, int nth);
void *pgrx_list_nth(List *list, int nth) {
return list_nth(list, nth);
}

PGDLLEXPORT int pgrx_list_nth_int(List *list, int nth);
int pgrx_list_nth_int(List *list, int nth) {
return list_nth_int(list, nth);
}

PGDLLEXPORT Oid pgrx_list_nth_oid(List *list, int nth);
Oid pgrx_list_nth_oid(List *list, int nth) {
return list_nth_oid(list, nth);
}

PGDLLEXPORT ListCell *pgrx_list_nth_cell(List *list, int nth);
ListCell *pgrx_list_nth_cell(List *list, int nth) {
return list_nth_cell(list, nth);
}

PGDLLEXPORT void pgrx_SpinLockInit(volatile slock_t *lock);
void pgrx_SpinLockInit(volatile slock_t *lock) {
void SpinLockInit__pgrx_cshim(volatile slock_t *lock) {
SpinLockInit(lock);
}

PGDLLEXPORT void pgrx_SpinLockAcquire(volatile slock_t *lock);
void pgrx_SpinLockAcquire(volatile slock_t *lock) {
void SpinLockAcquire__pgrx_cshim(volatile slock_t *lock) {
SpinLockAcquire(lock);
}

PGDLLEXPORT void pgrx_SpinLockRelease(volatile slock_t *lock);
void pgrx_SpinLockRelease(volatile slock_t *lock) {
void SpinLockRelease__pgrx_cshim(volatile slock_t *lock) {
SpinLockRelease(lock);
}

PGDLLEXPORT bool pgrx_SpinLockFree(slock_t *lock);
bool pgrx_SpinLockFree(slock_t *lock) {
bool SpinLockFree__pgrx_cshim(slock_t *lock) {
return SpinLockFree(lock);
}

int call_closure_with_sigsetjmp(int savemask, void* closure_env_ptr, int (*closure_code)(sigjmp_buf jbuf, void *env_ptr)) {
sigjmp_buf jbuf;
int val;
if (0 == (val = sigsetjmp(jbuf, savemask))) {
return closure_code(jbuf, closure_env_ptr);
} else {
return val;
}
}
14 changes: 4 additions & 10 deletions pgrx-pg-sys/src/cshim.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,21 +2,15 @@
#![allow(deprecated)]

use crate as pg_sys;
use core::ffi;

#[pgrx_macros::pg_guard]
extern "C" {
pub fn pgrx_list_nth(list: *mut pg_sys::List, nth: i32) -> *mut ffi::c_void;
pub fn pgrx_list_nth_int(list: *mut pg_sys::List, nth: i32) -> i32;
pub fn pgrx_list_nth_oid(list: *mut pg_sys::List, nth: i32) -> pg_sys::Oid;
pub fn pgrx_list_nth_cell(list: *mut pg_sys::List, nth: i32) -> *mut pg_sys::ListCell;

#[link_name = "pgrx_SpinLockInit"]
#[link_name = "SpinLockInit__pgrx_cshim"]
pub fn SpinLockInit(lock: *mut pg_sys::slock_t);
#[link_name = "pgrx_SpinLockAcquire"]
#[link_name = "SpinLockAcquire__pgrx_cshim"]
pub fn SpinLockAcquire(lock: *mut pg_sys::slock_t);
#[link_name = "pgrx_SpinLockRelease"]
#[link_name = "SpinLockRelease__pgrx_cshim"]
pub fn SpinLockRelease(lock: *mut pg_sys::slock_t);
#[link_name = "pgrx_SpinLockFree"]
#[link_name = "SpinLockFree__pgrx_cshim"]
pub fn SpinLockFree(lock: *mut pg_sys::slock_t) -> bool;
}
61 changes: 59 additions & 2 deletions pgrx-pg-sys/src/submodules/ffi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,64 @@
//LICENSE Use of this source code is governed by the MIT license that can be found in the LICENSE file.
#![deny(unsafe_op_in_unsafe_fn)]

use cee_scape::SigJmpBufFields;
#[cfg(not(all(
any(target_os = "linux", target_os = "macos"),
any(target_arch = "x86_64", target_arch = "aarch64")
)))]
mod cee_scape {
#[cfg(not(feature = "cshim"))]
compile_error!("target platform cannot work without feature cshim");
Comment on lines +12 to +18
Copy link
Member

Choose a reason for hiding this comment

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

Hm. Is there a reason we're not just using the use_c_to_interface_with_setjmp feature of cee_scape?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. To support more CPUs and Windows, we need a patch of cee_scape.
  2. In postgres/src/include/c.h, sigsetjmp is defined as:
/*
 * When there is no sigsetjmp, its functionality is provided by plain
 * setjmp.  We now support the case only on Windows.  However, it seems
 * that MinGW-64 has some longstanding issues in its setjmp support,
 * so on that toolchain we cheat and use gcc's builtins.
 */
#ifdef WIN32
#ifdef __MINGW64__
typedef intptr_t sigjmp_buf[5];
#define sigsetjmp(x,y) __builtin_setjmp(x)
#define siglongjmp __builtin_longjmp
#else							/* !__MINGW64__ */
#define sigjmp_buf jmp_buf
#define sigsetjmp(x,y) setjmp(x)
#define siglongjmp longjmp
#endif							/* __MINGW64__ */
#endif							/* WIN32 */

I feel it's better to include the header and use sigsetjmp in pgrx-cshim.c than adding a __builtin_setjmp version of call_closure_with_setjmp in cee-scape and gating the variants by cfg...

Copy link
Member

Choose a reason for hiding this comment

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

Ah, interesting.


use libc::{c_int, c_void};
use std::marker::PhantomData;

#[repr(C)]
pub struct SigJmpBufFields {
_internal: [u8; 0],
_neither_send_nor_sync: PhantomData<*const u8>,
}

pub fn call_with_sigsetjmp<F>(savemask: bool, mut callback: F) -> c_int
where
F: for<'a> FnOnce(&'a SigJmpBufFields) -> c_int,
{
extern "C" {
fn call_closure_with_sigsetjmp(
savemask: c_int,
closure_env_ptr: *mut c_void,
closure_code: extern "C" fn(
jbuf: *const SigJmpBufFields,
env_ptr: *mut c_void,
) -> c_int,
) -> c_int;
}

extern "C" fn call_from_c_to_rust<F>(
jbuf: *const SigJmpBufFields,
closure_env_ptr: *mut c_void,
) -> c_int
where
F: for<'a> FnOnce(&'a SigJmpBufFields) -> c_int,
{
let closure_env_ptr: *mut F = closure_env_ptr as *mut F;
unsafe { (closure_env_ptr.read())(&*jbuf) }
}

let savemask: libc::c_int = if savemask { 1 } else { 0 };

unsafe {
let closure_env_ptr = core::ptr::addr_of_mut!(callback);
core::mem::forget(callback);
call_closure_with_sigsetjmp(
savemask,
closure_env_ptr as *mut libc::c_void,
call_from_c_to_rust::<F>,
)
}
}
}

use cee_scape::{call_with_sigsetjmp, SigJmpBufFields};

/**
Given a closure that is assumed to be a wrapped Postgres `extern "C"` function, [pg_guard_ffi_boundary]
Expand Down Expand Up @@ -113,7 +170,7 @@ unsafe fn pg_guard_ffi_boundary_impl<T, F: FnOnce() -> T>(f: F) -> T {
let prev_exception_stack = pg_sys::PG_exception_stack;
let prev_error_context_stack = pg_sys::error_context_stack;
let mut result: std::mem::MaybeUninit<T> = MaybeUninit::uninit();
let jump_value = cee_scape::call_with_sigsetjmp(false, |jump_buffer| {
let jump_value = call_with_sigsetjmp(false, |jump_buffer| {
// Make Postgres' error-handling system aware of our new
// setjmp/longjmp restore point.
pg_sys::PG_exception_stack = std::mem::transmute(jump_buffer as *const SigJmpBufFields);
Expand Down
4 changes: 4 additions & 0 deletions pgrx-pg-sys/src/submodules/panic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -395,6 +395,7 @@ where
match unsafe { run_guarded(AssertUnwindSafe(f)) } {
GuardAction::Return(r) => r,
GuardAction::ReThrow => {
#[cfg_attr(target_os = "windows", link(name = "postgres"))]
extern "C" /* "C-unwind" */ {
fn pg_re_throw() -> !;
}
Expand Down Expand Up @@ -509,6 +510,7 @@ fn do_ereport(ereport: ErrorReportWithLevel) {
// `build.rs` and we'd prefer pgrx users not have access to them at all
//

#[cfg_attr(target_os = "windows", link(name = "postgres"))]
extern "C" {
fn errcode(sqlerrcode: ::std::os::raw::c_int) -> ::std::os::raw::c_int;
fn errmsg(fmt: *const ::std::os::raw::c_char, ...) -> ::std::os::raw::c_int;
Expand All @@ -524,6 +526,7 @@ fn do_ereport(ereport: ErrorReportWithLevel) {
#[cfg(any(feature = "pg13", feature = "pg14", feature = "pg15", feature = "pg16", feature = "pg17"))]
fn do_ereport_impl(ereport: ErrorReportWithLevel) {

#[cfg_attr(target_os = "windows", link(name = "postgres"))]
extern "C" {
fn errstart(elevel: ::std::os::raw::c_int, domain: *const ::std::os::raw::c_char) -> bool;
fn errfinish(filename: *const ::std::os::raw::c_char, lineno: ::std::os::raw::c_int, funcname: *const ::std::os::raw::c_char);
Expand Down Expand Up @@ -588,6 +591,7 @@ fn do_ereport(ereport: ErrorReportWithLevel) {
#[cfg(feature = "pg12")]
fn do_ereport_impl(ereport: ErrorReportWithLevel) {

#[cfg_attr(target_os = "windows", link(name = "postgres"))]
extern "C" {
fn errstart(elevel: ::std::os::raw::c_int, filename: *const ::std::os::raw::c_char, lineno: ::std::os::raw::c_int, funcname: *const ::std::os::raw::c_char, domain: *const ::std::os::raw::c_char) -> bool;
fn errfinish(dummy: ::std::os::raw::c_int, ...);
Expand Down
3 changes: 3 additions & 0 deletions pgrx-pg-sys/src/submodules/thread_check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ fn init_active_thread(tid: NonZeroUsize) {
panic!("Attempt to initialize `pgrx` active thread from a thread other than the main");
}
match ACTIVE_THREAD.compare_exchange(0, tid.get(), Ordering::Relaxed, Ordering::Relaxed) {
#[cfg(not(target_os = "windows"))]
Ok(_) => unsafe {
// We won the race. Register an atfork handler to clear the atomic
// in any child processes we spawn.
Expand All @@ -102,6 +103,8 @@ fn init_active_thread(tid: NonZeroUsize) {
}
libc::pthread_atfork(None, None, Some(clear_in_child));
},
#[cfg(target_os = "windows")]
Ok(_) => (),
Err(_) => {
thread_id_check_failed();
}
Expand Down
8 changes: 4 additions & 4 deletions pgrx/src/bgworkers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -343,7 +343,7 @@ impl DynamicBackgroundWorker {
/// return [`BackgroundWorkerStatus::Untracked`] error
pub fn wait_for_startup(&self) -> Result<Pid, BackgroundWorkerStatus> {
unsafe {
if self.notify_pid != pg_sys::MyProcPid {
if self.notify_pid != pg_sys::MyProcPid as pg_sys::pid_t {
return Err(BackgroundWorkerStatus::Untracked { notify_pid: self.notify_pid });
}
}
Expand Down Expand Up @@ -382,7 +382,7 @@ impl TerminatingDynamicBackgroundWorker {
/// return [`BackgroundWorkerStatus::Untracked`] error
pub fn wait_for_shutdown(self) -> Result<(), BackgroundWorkerStatus> {
unsafe {
if self.notify_pid != pg_sys::MyProcPid {
if self.notify_pid != pg_sys::MyProcPid as pg_sys::pid_t {
return Err(BackgroundWorkerStatus::Untracked { notify_pid: self.notify_pid });
}
}
Expand Down Expand Up @@ -580,7 +580,7 @@ impl BackgroundWorkerBuilder {
/// to wait for the worker to start up. Otherwise, it should be initialized to
/// `pgrx::pg_sys::MyProcPid`
pub fn set_notify_pid(mut self, input: i32) -> Self {
self.bgw_notify_pid = input;
self.bgw_notify_pid = input as pg_sys::pid_t;
self
}

Expand Down Expand Up @@ -634,7 +634,7 @@ impl<'a> From<&'a BackgroundWorkerBuilder> for pg_sys::BackgroundWorker {
bgw_name: RpgffiChar::from(&builder.bgw_name[..]).0,
bgw_type: RpgffiChar::from(&builder.bgw_type[..]).0,
bgw_flags: builder.bgw_flags.bits(),
bgw_start_time: builder.bgw_start_time as u32,
bgw_start_time: builder.bgw_start_time as _,
bgw_restart_time: match builder.bgw_restart_time {
None => -1,
Some(d) => d.as_secs() as i32,
Expand Down
Loading
Loading