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

patchable-function-entry: Add unstable compiler flag and attribute #124741

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
31 changes: 30 additions & 1 deletion compiler/rustc_codegen_llvm/src/attributes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

use rustc_codegen_ssa::traits::*;
use rustc_hir::def_id::DefId;
use rustc_middle::middle::codegen_fn_attrs::CodegenFnAttrFlags;
use rustc_middle::middle::codegen_fn_attrs::{CodegenFnAttrFlags, PatchableFunctionEntry};
use rustc_middle::ty::{self, TyCtxt};
use rustc_session::config::{FunctionReturn, OptLevel};
use rustc_span::symbol::sym;
Expand Down Expand Up @@ -53,6 +53,34 @@ fn inline_attr<'ll>(cx: &CodegenCx<'ll, '_>, inline: InlineAttr) -> Option<&'ll
}
}

#[inline]
fn patchable_function_entry_attrs<'ll>(
cx: &CodegenCx<'ll, '_>,
attr: Option<PatchableFunctionEntry>,
) -> SmallVec<[&'ll Attribute; 2]> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is only ever two, instead of "two but sometimes it needs to be more", why not this?

Suggested change
) -> SmallVec<[&'ll Attribute; 2]> {
) -> [Option<&'ll Attribute>; 2] {

At most it would add a .filter_map(|x| x) at the callsite. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personally I don't think it is worth it. Code would become a more cluttered, while only saving a bit of space on the stack.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking more in the "embed our expectations in the types" rather than as an optimization, but yeah, it does make things a bit messier I guess. It was mostly a vibes-based question, and I'm fine with that not being the vibe~

let mut attrs = SmallVec::new();
let patchable_spec = attr.unwrap_or_else(|| {
PatchableFunctionEntry::from_config(cx.tcx.sess.opts.unstable_opts.patchable_function_entry)
});
let entry = patchable_spec.entry();
let prefix = patchable_spec.prefix();
if entry > 0 {
attrs.push(llvm::CreateAttrStringValue(
cx.llcx,
"patchable-function-entry",
&format!("{}", entry),
));
}
if prefix > 0 {
attrs.push(llvm::CreateAttrStringValue(
cx.llcx,
"patchable-function-prefix",
&format!("{}", prefix),
));
}
attrs
}

/// Get LLVM sanitize attributes.
#[inline]
pub fn sanitize_attrs<'ll>(
Expand Down Expand Up @@ -420,6 +448,7 @@ pub fn from_fn_attrs<'ll, 'tcx>(
llvm::set_alignment(llfn, align);
}
to_add.extend(sanitize_attrs(cx, codegen_fn_attrs.no_sanitize));
to_add.extend(patchable_function_entry_attrs(cx, codegen_fn_attrs.patchable_function_entry));

// Always annotate functions with the target-cpu they are compiled for.
// Without this, ThinLTO won't inline Rust functions into Clang generated
Expand Down
64 changes: 63 additions & 1 deletion compiler/rustc_codegen_ssa/src/codegen_attrs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@ use rustc_hir as hir;
use rustc_hir::def::DefKind;
use rustc_hir::def_id::{DefId, LocalDefId, LOCAL_CRATE};
use rustc_hir::{lang_items, weak_lang_items::WEAK_LANG_ITEMS, LangItem};
use rustc_middle::middle::codegen_fn_attrs::{CodegenFnAttrFlags, CodegenFnAttrs};
use rustc_middle::middle::codegen_fn_attrs::{
CodegenFnAttrFlags, CodegenFnAttrs, PatchableFunctionEntry,
};
use rustc_middle::mir::mono::Linkage;
use rustc_middle::query::Providers;
use rustc_middle::ty::{self as ty, TyCtxt};
Expand Down Expand Up @@ -465,6 +467,66 @@ fn codegen_fn_attrs(tcx: TyCtxt<'_>, did: LocalDefId) -> CodegenFnAttrs {
None
};
}
sym::patchable_function_entry => {
codegen_fn_attrs.patchable_function_entry = attr.meta_item_list().and_then(|l| {
let mut prefix = None;
let mut entry = None;
for item in l {
let Some(meta_item) = item.meta_item() else {
tcx.dcx().span_err(item.span(), "Expected name value pair.");
continue;
};

let Some(name_value_lit) = meta_item.name_value_literal() else {
tcx.dcx().span_err(item.span(), "Expected name value pair.");
continue;
};

let attrib_to_write = match meta_item.name_or_empty() {
sym::prefix_nops => &mut prefix,
sym::entry_nops => &mut entry,
_ => {
tcx.dcx().span_err(
item.span(),
format!(
"Unexpected name. Allowed names: {}, {}",
sym::prefix_nops,
sym::entry_nops
),
);
continue;
}
};

let rustc_ast::LitKind::Int(val, _) = name_value_lit.kind else {
tcx.dcx().span_err(
name_value_lit.span,
"Expected integer value between 0 and 255.",
);
continue;
};

let Ok(val) = val.get().try_into() else {
tcx.dcx().span_err(
name_value_lit.span,
"Integer value outside range between 0 and 255.",
);
continue;
};

*attrib_to_write = Some(val);
Comment on lines +501 to +517
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this should probably just be

Suggested change
let rustc_ast::LitKind::Int(val, _) = name_value_lit.kind else {
tcx.dcx().span_err(
name_value_lit.span,
"Expected integer value between 0 and 255.",
);
continue;
};
let Ok(val) = val.get().try_into() else {
tcx.dcx().span_err(
name_value_lit.span,
"Integer value outside range between 0 and 255.",
);
continue;
};
*attrib_to_write = Some(val);
let rustc_ast::LitKind::Int(rustc_data_structures::packed::Pu128(val @ 0..255), _) = name_value_lit.kind else {
tcx.dcx().span_err(
name_value_lit.span,
"Expected integer value between 0 and 255.",
);
continue;
};
*attrib_to_write = Some(val as u8);

except... maybe an import would be appropriate for one of those.

}

if let (None, None) = (prefix, entry) {
tcx.dcx().span_err(attr.span, "Must specify at least one parameter.");
}
Comment on lines +476 to +522
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I missed this in my initial review pass because I was focused on the codegen parts, but yes there should probably be a UI test for these.


Some(PatchableFunctionEntry::from_prefix_and_entry(
prefix.unwrap_or(0),
entry.unwrap_or(0),
))
})
}
_ => {}
}
}
Expand Down
7 changes: 7 additions & 0 deletions compiler/rustc_feature/src/builtin_attrs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -527,6 +527,13 @@ pub const BUILTIN_ATTRIBUTES: &[BuiltinAttribute] = &[
EncodeCrossCrate::No, coroutines, experimental!(coroutines)
),

// RFC 3543
// `#[patchable_function_entry(prefix_nops = m, entry_nops = n)]`
gated!(
patchable_function_entry, Normal, template!(List: "prefix_nops = m, entry_nops = n"), ErrorPreceding,
EncodeCrossCrate::Yes, experimental!(patchable_function_entry)
),

// ==========================================================================
// Internal attributes: Stability, deprecation, and unsafe:
// ==========================================================================
Expand Down
2 changes: 2 additions & 0 deletions compiler/rustc_feature/src/unstable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -563,6 +563,8 @@ declare_features! (
(unstable, offset_of_nested, "1.77.0", Some(120140)),
/// Allows using `#[optimize(X)]`.
(unstable, optimize_attribute, "1.34.0", Some(54882)),
/// Allows specifying nop padding on functions for dynamic patching.
(unstable, patchable_function_entry, "CURRENT_RUSTC_VERSION", Some(123115)),
/// Allows postfix match `expr.match { ... }`
(unstable, postfix_match, "1.79.0", Some(121618)),
/// Allows `use<'a, 'b, A, B>` in `impl use<...> Trait` for precise capture of generic args.
Expand Down
8 changes: 6 additions & 2 deletions compiler/rustc_interface/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ use rustc_session::config::{
ErrorOutputType, ExternEntry, ExternLocation, Externs, FunctionReturn, InliningThreshold,
Input, InstrumentCoverage, InstrumentXRay, LinkSelfContained, LinkerPluginLto, LocationDetail,
LtoCli, NextSolverConfig, OomStrategy, Options, OutFileName, OutputType, OutputTypes, PAuthKey,
PacRet, Passes, Polonius, ProcMacroExecutionStrategy, Strip, SwitchWithOptPath,
SymbolManglingVersion, WasiExecModel,
PacRet, Passes, PatchableFunctionEntry, Polonius, ProcMacroExecutionStrategy, Strip,
SwitchWithOptPath, SymbolManglingVersion, WasiExecModel,
};
use rustc_session::lint::Level;
use rustc_session::search_paths::SearchPath;
Expand Down Expand Up @@ -813,6 +813,10 @@ fn test_unstable_options_tracking_hash() {
tracked!(packed_bundled_libs, true);
tracked!(panic_abort_tests, true);
tracked!(panic_in_drop, PanicStrategy::Abort);
tracked!(
patchable_function_entry,
PatchableFunctionEntry::from_total_and_prefix_nops(10, 5).expect("total >= prefix")
);
tracked!(plt, Some(true));
tracked!(polonius, Polonius::Legacy);
tracked!(precise_enum_drop_elaboration, false);
Expand Down
27 changes: 27 additions & 0 deletions compiler/rustc_middle/src/middle/codegen_fn_attrs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,32 @@ pub struct CodegenFnAttrs {
/// The `#[repr(align(...))]` attribute. Indicates the value of which the function should be
/// aligned to.
pub alignment: Option<Align>,
/// The `#[patchable_function_entry(...)]` attribute. Indicates how many nops should be around
/// the function entry.
pub patchable_function_entry: Option<PatchableFunctionEntry>,
}

#[derive(Copy, Clone, Debug, TyEncodable, TyDecodable, HashStable)]
pub struct PatchableFunctionEntry {
/// Nops to prepend to the function
prefix: u8,
/// Nops after entry, but before body
entry: u8,
}

impl PatchableFunctionEntry {
pub fn from_config(config: rustc_session::config::PatchableFunctionEntry) -> Self {
Self { prefix: config.prefix(), entry: config.entry() }
}
pub fn from_prefix_and_entry(prefix: u8, entry: u8) -> Self {
Self { prefix, entry }
}
pub fn prefix(&self) -> u8 {
self.prefix
}
pub fn entry(&self) -> u8 {
self.entry
}
}

#[derive(Clone, Copy, PartialEq, Eq, TyEncodable, TyDecodable, HashStable)]
Expand Down Expand Up @@ -124,6 +150,7 @@ impl CodegenFnAttrs {
no_sanitize: SanitizerSet::empty(),
instruction_set: None,
alignment: None,
patchable_function_entry: None,
}
}

Expand Down
35 changes: 33 additions & 2 deletions compiler/rustc_session/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2933,8 +2933,9 @@ pub(crate) mod dep_tracking {
CrateType, DebugInfo, DebugInfoCompression, ErrorOutputType, FunctionReturn,
InliningThreshold, InstrumentCoverage, InstrumentXRay, LinkerPluginLto, LocationDetail,
LtoCli, NextSolverConfig, OomStrategy, OptLevel, OutFileName, OutputType, OutputTypes,
Polonius, RemapPathScopeComponents, ResolveDocLinks, SourceFileHashAlgorithm,
SplitDwarfKind, SwitchWithOptPath, SymbolManglingVersion, WasiExecModel,
PatchableFunctionEntry, Polonius, RemapPathScopeComponents, ResolveDocLinks,
SourceFileHashAlgorithm, SplitDwarfKind, SwitchWithOptPath, SymbolManglingVersion,
WasiExecModel,
};
use crate::lint;
use crate::utils::NativeLib;
Expand Down Expand Up @@ -3042,6 +3043,7 @@ pub(crate) mod dep_tracking {
OomStrategy,
LanguageIdentifier,
NextSolverConfig,
PatchableFunctionEntry,
Polonius,
InliningThreshold,
FunctionReturn,
Expand Down Expand Up @@ -3219,6 +3221,35 @@ impl DumpMonoStatsFormat {
}
}

/// `-Z patchable-function-entry` representation - how many nops to put before and after function
/// entry.
#[derive(Clone, Copy, PartialEq, Hash, Debug, Default)]
pub struct PatchableFunctionEntry {
/// Nops before the entry
prefix: u8,
/// Nops after the entry
entry: u8,
}

impl PatchableFunctionEntry {
pub fn from_total_and_prefix_nops(
total_nops: u8,
prefix_nops: u8,
) -> Option<PatchableFunctionEntry> {
if total_nops < prefix_nops {
None
} else {
Some(Self { prefix: prefix_nops, entry: total_nops - prefix_nops })
}
}
workingjubilee marked this conversation as resolved.
Show resolved Hide resolved
pub fn prefix(&self) -> u8 {
self.prefix
}
pub fn entry(&self) -> u8 {
self.entry
}
}

/// `-Zpolonius` values, enabling the borrow checker polonius analysis, and which version: legacy,
/// or future prototype.
#[derive(Clone, Copy, PartialEq, Hash, Debug, Default)]
Expand Down
29 changes: 29 additions & 0 deletions compiler/rustc_session/src/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -379,6 +379,7 @@ mod desc {
pub const parse_passes: &str = "a space-separated list of passes, or `all`";
pub const parse_panic_strategy: &str = "either `unwind` or `abort`";
pub const parse_on_broken_pipe: &str = "either `kill`, `error`, or `inherit`";
pub const parse_patchable_function_entry: &str = "either two comma separated integers (total_nops,prefix_nops), with prefix_nops <= total_nops, or one integer (total_nops)";
pub const parse_opt_panic_strategy: &str = parse_panic_strategy;
pub const parse_oom_strategy: &str = "either `panic` or `abort`";
pub const parse_relro_level: &str = "one of: `full`, `partial`, or `off`";
Expand Down Expand Up @@ -721,6 +722,32 @@ mod parse {
true
}

pub(crate) fn parse_patchable_function_entry(
slot: &mut PatchableFunctionEntry,
v: Option<&str>,
) -> bool {
let mut total_nops = 0;
let mut prefix_nops = 0;

if !parse_number(&mut total_nops, v) {
let parts = v.and_then(|v| v.split_once(',')).unzip();
if !parse_number(&mut total_nops, parts.0) {
return false;
}
if !parse_number(&mut prefix_nops, parts.1) {
return false;
}
}

if let Some(pfe) =
PatchableFunctionEntry::from_total_and_prefix_nops(total_nops, prefix_nops)
{
*slot = pfe;
return true;
}
false
}

pub(crate) fn parse_oom_strategy(slot: &mut OomStrategy, v: Option<&str>) -> bool {
match v {
Some("panic") => *slot = OomStrategy::Panic,
Expand Down Expand Up @@ -1844,6 +1871,8 @@ options! {
"panic strategy for panics in drops"),
parse_only: bool = (false, parse_bool, [UNTRACKED],
"parse only; do not compile, assemble, or link (default: no)"),
patchable_function_entry: PatchableFunctionEntry = (PatchableFunctionEntry::default(), parse_patchable_function_entry, [TRACKED],
"nop padding at function entry"),
plt: Option<bool> = (None, parse_opt_bool, [TRACKED],
"whether to use the PLT when calling into shared libraries;
only has effect for PIC code on systems with ELF binaries
Expand Down
3 changes: 3 additions & 0 deletions compiler/rustc_span/src/symbol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -758,6 +758,7 @@ symbols! {
enable,
encode,
end,
entry_nops,
enumerate_method,
env,
env_CFG_RELEASE: env!("CFG_RELEASE"),
Expand Down Expand Up @@ -1369,6 +1370,7 @@ symbols! {
passes,
pat,
pat_param,
patchable_function_entry,
path,
pattern_complexity,
pattern_parentheses,
Expand Down Expand Up @@ -1406,6 +1408,7 @@ symbols! {
prefetch_read_instruction,
prefetch_write_data,
prefetch_write_instruction,
prefix_nops,
preg,
prelude,
prelude_import,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
# `patchable-function-entry`

--------------------

The `-Z patchable-function-entry=total_nops,prefix_nops` or `-Z patchable-function-entry=total_nops`
compiler flag enables nop padding of function entries with 'total_nops' nops, with
an offset for the entry of the function at 'prefix_nops' nops. In the second form,
'prefix_nops' defaults to 0.

As an illustrative example, `-Z patchable-function-entry=3,2` would produce:

```text
nop
nop
function_label:
nop
//Actual function code begins here
```

This flag is used for hotpatching, especially in the Linux kernel. The flag
arguments are modeled after the `-fpatchable-function-entry` flag as defined
for both [Clang](https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang-fpatchable-function-entry)
and [gcc](https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html#index-fpatchable-function-entry)
and is intended to provide the same effect.