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

Merged
merged 4 commits into from
Jun 28, 2024
Merged
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
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]> {
workingjubilee marked this conversation as resolved.
Show resolved Hide resolved
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 @@ -421,6 +449,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
58 changes: 57 additions & 1 deletion compiler/rustc_codegen_ssa/src/codegen_attrs.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
use rustc_ast::{ast, attr, MetaItemKind, NestedMetaItem};
use rustc_attr::{list_contains_name, InlineAttr, InstructionSetAttr, OptimizeAttr};
use rustc_data_structures::packed::Pu128;
use rustc_errors::{codes::*, struct_span_code_err};
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 @@ -463,6 +466,59 @@ 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.");
Copy link
Contributor

Choose a reason for hiding this comment

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

For future reference, the message format doesn't use title-case nor periods. Maybe we should, but let's remain consistent with what we already have:

Suggested change
tcx.dcx().span_err(item.span(), "Expected name value pair.");
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.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
tcx.dcx().span_err(item.span(), "Expected name value pair.");
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 parameter name. Allowed names: {}, {}",
sym::prefix_nops,
sym::entry_nops
),
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
tcx.dcx().span_err(
item.span(),
format!(
"Unexpected parameter name. Allowed names: {}, {}",
sym::prefix_nops,
sym::entry_nops
),
);
tcx.dcx().struct_span_err(
item.span(),
"unexpected parameter name"
).span_label(
item.span(),
format!(
"expected {} or {}",
sym::prefix_nops,
sym::entry_nops
),
).emit();

continue;
}
};

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

Choose a reason for hiding this comment

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

Suggested change
tcx.dcx().span_err(
name_value_lit.span,
"Expected integer value between 0 and 255.",
);
tcx.dcx().struct_span_err(
name_value_lit.span,
"integer value out of range",
).span_label(
name_value_lit.span,
"value must be between `0` and `255`",
).emit();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This error message would also be triggered when passing in a string, so the suggested wording might be a bit weird in that case. Do you think it makes sense to make the check for the string seperate again or is this just a minor issue?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is worth it to be more accurate whenever possible. you can make this be

Suggested change
tcx.dcx().span_err(
name_value_lit.span,
"Expected integer value between 0 and 255.",
);
let mut err = tcx.dcx().struct_span_err(
name_value_lit.span,
"invalid literal value",
);
if is_numeric {
err.span_label(
name_value_lit.span,
"value must be between `0` and `255`"
);
} else if is_str {
err.span_label(
name_value_lit.span,
"some appropriate message"
);
}
// Are there other cases to handle? can we hit this error without a numeric or str literal?
err.emit();

continue;
};

*attrib_to_write = Some(val.try_into().unwrap());
}

if let (None, None) = (prefix, entry) {
tcx.dcx().span_err(attr.span, "Must specify at least one parameter.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
tcx.dcx().span_err(attr.span, "Must specify at least one parameter.");
tcx.dcx().span_err(attr.span, "must specify at least one parameter");

}

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 @@ -585,6 +585,13 @@ pub const BUILTIN_ATTRIBUTES: &[BuiltinAttribute] = &[
EncodeCrossCrate::No, derive_smart_pointer, experimental!(pointee)
),

// 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 @@ -565,6 +565,8 @@ declare_features! (
(unstable, offset_of_slice, "CURRENT_RUSTC_VERSION", Some(126151)),
/// 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 Trait + use<...>` 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")
Copy link
Contributor

Choose a reason for hiding this comment

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

The message in expect needs to be the description of the condition that wasn't met.

Suggested change
PatchableFunctionEntry::from_total_and_prefix_nops(10, 5).expect("total >= prefix")
PatchableFunctionEntry::from_total_and_prefix_nops(10, 5).expect("total < prefix")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't know if I understood you correctly, but to get a value the condition 'total >=prefix' must be true. The panic would happen if that condition is false. So I'd think it would be correct as written?

Copy link
Contributor

Choose a reason for hiding this comment

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

What will happen if total < prefix is that at runtime we'll get a panic that looks like the following:

thread 'main' panicked at compiler/rustc_interface/src/tests.rs:818:8:
total >= prefix

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=7e6fd3fe91aec1a58fdd5b198388ec23

Instead we should provide additional context beyond that. Having said that, this is a nitpick to make it easier for people to understand that happened in this test, it's not a deal breaker.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
PatchableFunctionEntry::from_total_and_prefix_nops(10, 5).expect("total >= prefix")
PatchableFunctionEntry::from_total_and_prefix_nops(10, 5).expect("total must be greater than prefix")

Copy link
Member

Choose a reason for hiding this comment

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

maybe?

Copy link
Member

Choose a reason for hiding this comment

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

or just unwrap, this is just a test, really, and the condition is trivially obviously true.

);
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 @@ -2963,8 +2963,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 @@ -3071,6 +3072,7 @@ pub(crate) mod dep_tracking {
OomStrategy,
LanguageIdentifier,
NextSolverConfig,
PatchableFunctionEntry,
Polonius,
InliningThreshold,
FunctionReturn,
Expand Down Expand Up @@ -3248,6 +3250,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 @@ -734,6 +735,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 @@ -1859,6 +1886,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 @@ -768,6 +768,7 @@ symbols! {
enable,
encode,
end,
entry_nops,
enumerate_method,
env,
env_CFG_RELEASE: env!("CFG_RELEASE"),
Expand Down Expand Up @@ -1383,6 +1384,7 @@ symbols! {
passes,
pat,
pat_param,
patchable_function_entry,
path,
pattern_complexity,
pattern_parentheses,
Expand Down Expand Up @@ -1421,6 +1423,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.
Loading
Loading