Updated code for changes to RFC, added additional error handling, added

tests

Author: nebulark

compiler/rustc_codegen_ssa/src/codegen_attrs.rs

@@ -1,5 +1,6 @@
 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;
@@ -467,24 +468,55 @@ fn codegen_fn_attrs(tcx: TyCtxt<'_>, did: LocalDefId) -> CodegenFnAttrs {
             }
             sym::patchable_function_entry => {
                 codegen_fn_attrs.patchable_function_entry = attr.meta_item_list().and_then(|l| {
-                    let mut prefix = 0;
-                    let mut entry = 0;
+                    let mut prefix = None;
+                    let mut entry = None;
                     for item in l {
-                        if let Some((sym, lit)) = item.name_value_literal() {
-                            let val = match lit.kind {
-                                // FIXME emit error if too many nops requested
-                                rustc_ast::LitKind::Int(i, _) => i as u8,
-                                _ => continue,
-                            };
-                            match sym {
-                                sym::prefix => prefix = val,
-                                sym::entry => entry = val,
-                                // FIXME possibly emit error here?
-                                _ => continue,
+                        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 parameter name. Allowed names: {}, {}",
+                                        sym::prefix_nops,
+                                        sym::entry_nops
+                                    ),
+                                );
+                                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.",
+                            );
+                            continue;
+                        };
+
+                        *attrib_to_write = Some(val.try_into().unwrap());
                     }
-                    Some(PatchableFunctionEntry::from_prefix_and_entry(prefix, entry))
+
+                    if let (None, None) = (prefix, entry) {
+                        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),
+                    ))
                 })
             }
             _ => {}

compiler/rustc_feature/src/builtin_attrs.rs

@@ -585,11 +585,11 @@ pub const BUILTIN_ATTRIBUTES: &[BuiltinAttribute] = &[
         EncodeCrossCrate::No, derive_smart_pointer, experimental!(pointee)
     ),
 
-    // FIXME RFC
-    // `#[patchable_function_entry(prefix(n), entry(n))]`
+    // RFC 3543
+    // `#[patchable_function_entry(prefix_nops = m, entry_nops = n)]`
     gated!(
-        patchable_function_entry, Normal, template!(List: "prefix(n), entry(n)"), ErrorPreceding,
-        experimental!(patchable_function_entry)
+        patchable_function_entry, Normal, template!(List: "prefix_nops = m, entry_nops = n"), ErrorPreceding,
+        EncodeCrossCrate::Yes, experimental!(patchable_function_entry)
     ),
 
     // ==========================================================================

compiler/rustc_feature/src/unstable.rs

@@ -566,8 +566,7 @@ declare_features! (
     /// Allows using `#[optimize(X)]`.
     (unstable, optimize_attribute, "1.34.0", Some(54882)),
     /// Allows specifying nop padding on functions for dynamic patching.
-    // FIXME this needs an RFC #
-    (unstable, patchable_function_entry, "CURRENT_RUSTC_VERSION", Some(9999)),
+    (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.

compiler/rustc_interface/src/tests.rs

@@ -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;
@@ -813,7 +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_nop_count_and_offset(3, 4));
+    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);

compiler/rustc_session/src/config.rs

@@ -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,
-        PatchableFunctionEntry, Polonius, RemapPathScopeComponents, ResolveDocLinks, SourceFileHashAlgorithm,
-        SplitDwarfKind, SwitchWithOptPath, SymbolManglingVersion, WasiExecModel,
+        PatchableFunctionEntry, Polonius, RemapPathScopeComponents, ResolveDocLinks,
+        SourceFileHashAlgorithm, SplitDwarfKind, SwitchWithOptPath, SymbolManglingVersion,
+        WasiExecModel,
     };
     use crate::lint;
     use crate::utils::NativeLib;
@@ -3260,11 +3261,14 @@ pub struct PatchableFunctionEntry {
 }
 
 impl PatchableFunctionEntry {
-    pub fn from_nop_count_and_offset(nop_count: u8, offset: u8) -> Option<PatchableFunctionEntry> {
-        if nop_count < offset {
+    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: offset, entry: nop_count - offset })
+            Some(Self { prefix: prefix_nops, entry: total_nops - prefix_nops })
         }
     }
     pub fn prefix(&self) -> u8 {

compiler/rustc_session/src/options.rs

@@ -379,8 +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 =
-        "nop_count,entry_offset or nop_count (defaulting entry_offset=0)";
+    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`";
@@ -725,7 +724,6 @@ mod parse {
         true
     }
 
-
     pub(crate) fn parse_on_broken_pipe(slot: &mut OnBrokenPipe, v: Option<&str>) -> bool {
         match v {
             // OnBrokenPipe::Default can't be explicitly specified
@@ -741,20 +739,22 @@ mod parse {
         slot: &mut PatchableFunctionEntry,
         v: Option<&str>,
     ) -> bool {
-        let mut nop_count = 0;
-        let mut offset = 0;
+        let mut total_nops = 0;
+        let mut prefix_nops = 0;
 
-        if !parse_number(&mut nop_count, v) {
+        if !parse_number(&mut total_nops, v) {
             let parts = v.and_then(|v| v.split_once(',')).unzip();
-            if !parse_number(&mut nop_count, parts.0) {
+            if !parse_number(&mut total_nops, parts.0) {
                 return false;
             }
-            if !parse_number(&mut offset, parts.1) {
+            if !parse_number(&mut prefix_nops, parts.1) {
                 return false;
             }
         }
 
-        if let Some(pfe) = PatchableFunctionEntry::from_nop_count_and_offset(nop_count, offset) {
+        if let Some(pfe) =
+            PatchableFunctionEntry::from_total_and_prefix_nops(total_nops, prefix_nops)
+        {
             *slot = pfe;
             return true;
         }

compiler/rustc_span/src/symbol.rs

@@ -768,7 +768,7 @@ symbols! {
         enable,
         encode,
         end,
-        entry,
+        entry_nops,
         enumerate_method,
         env,
         env_CFG_RELEASE: env!("CFG_RELEASE"),
@@ -1423,7 +1423,7 @@ symbols! {
         prefetch_read_instruction,
         prefetch_write_data,
         prefetch_write_instruction,
-        prefix,
+        prefix_nops,
         preg,
         prelude,
         prelude_import,

src/doc/unstable-book/src/compiler-flags/patchable-function-entry.md

@@ -2,14 +2,14 @@
 
 --------------------
 
-The `-Z patchable-function-entry=M,N` or `-Z patchable-function-entry=M`
-compiler flag enables nop padding of function entries with M nops, with
-an offset for the entry of the function at N nops. In the second form,
-N defaults to 0.
+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:
@@ -18,7 +18,7 @@ nop
 ```
 
 This flag is used for hotpatching, especially in the Linux kernel. The flag
-arguments are modeled after hte `-fpatchable-function-entry` flag as defined
+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.

tests/codegen/patchable-function-entry.rs

@@ -0,0 +1,64 @@
+//@ compile-flags: -Z patchable-function-entry=15,10
+
+#![feature(patchable_function_entry)]
+#![crate_type = "lib"]
+
+// This should have the default, as set by the compile flags
+#[no_mangle]
+pub fn fun0() {}
+
+// The attribute should override the compile flags
+#[no_mangle]
+#[patchable_function_entry(prefix_nops = 1, entry_nops = 2)]
+pub fn fun1() {}
+
+// If we override an attribute to 0 or unset, the attribute should go away
+#[no_mangle]
+#[patchable_function_entry(entry_nops = 0)]
+pub fn fun2() {}
+
+// The attribute should override the compile flags
+#[no_mangle]
+#[patchable_function_entry(prefix_nops = 20, entry_nops = 1)]
+pub fn fun3() {}
+
+// The attribute should override the compile flags
+#[no_mangle]
+#[patchable_function_entry(prefix_nops = 2, entry_nops = 19)]
+pub fn fun4() {}
+
+// The attribute should override patchable-function-entry to 3 and
+// patchable-function-prefix to the default of 0, clearing it entirely
+#[no_mangle]
+#[patchable_function_entry(entry_nops = 3)]
+pub fn fun5() {}
+
+// The attribute should override patchable-function-prefix to 4
+// and patchable-function-entry to the default of 0, clearing it entirely
+#[no_mangle]
+#[patchable_function_entry(prefix_nops = 4)]
+pub fn fun6() {}
+
+// CHECK: @fun0() unnamed_addr #0
+// CHECK: @fun1() unnamed_addr #1
+// CHECK: @fun2() unnamed_addr #2
+// CHECK: @fun3() unnamed_addr #3
+// CHECK: @fun4() unnamed_addr #4
+// CHECK: @fun5() unnamed_addr #5
+// CHECK: @fun6() unnamed_addr #6
+
+// CHECK: attributes #0 = { {{.*}}"patchable-function-entry"="5"{{.*}}"patchable-function-prefix"="10" {{.*}} }
+// CHECK: attributes #1 = { {{.*}}"patchable-function-entry"="2"{{.*}}"patchable-function-prefix"="1" {{.*}} }
+
+// CHECK-NOT: attributes #2 = { {{.*}}patchable-function-entry{{.*}} }
+// CHECK-NOT: attributes #2 = { {{.*}}patchable-function-prefix{{.*}} }
+// CHECK: attributes #2 = { {{.*}} }
+
+// CHECK: attributes #3 = { {{.*}}"patchable-function-entry"="1"{{.*}}"patchable-function-prefix"="20" {{.*}} }
+// CHECK: attributes #4 = { {{.*}}"patchable-function-entry"="19"{{.*}}"patchable-function-prefix"="2" {{.*}} }
+
+// CHECK: attributes #5 = { {{.*}}"patchable-function-entry"="3"{{.*}} }
+// CHECK-NOT: attributes #5 = { {{.*}}patchable-function-prefix{{.*}} }
+
+// CHECK: attributes #6 = { {{.*}}"patchable-function-prefix"="4"{{.*}} }
+// CHECK-NOT: attributes #6 = { {{.*}}patchable-function-entry{{.*}} }

tests/codegen/patchable-function-entry/patchable-function-entry-both-flags.rs

@@ -0,0 +1,39 @@
+#![feature(patchable_function_entry)]
+#![crate_type = "lib"]
+
+// No patchable function entry should be set
+#[no_mangle]
+pub fn fun0() {}
+
+// The attribute should work even without compiler flags
+#[no_mangle]
+#[patchable_function_entry(prefix_nops = 1, entry_nops = 2)]
+pub fn fun1() {}
+
+// The attribute should work even without compiler flags
+// and only set patchable-function-entry to 3.
+#[no_mangle]
+#[patchable_function_entry(entry_nops = 3)]
+pub fn fun2() {}
+
+// The attribute should work even without compiler flags
+// and only set patchable-function-prefix to 4.
+#[no_mangle]
+#[patchable_function_entry(prefix_nops = 4)]
+pub fn fun3() {}
+
+// CHECK: @fun0() unnamed_addr #0
+// CHECK: @fun1() unnamed_addr #1
+// CHECK: @fun2() unnamed_addr #2
+// CHECK: @fun3() unnamed_addr #3
+
+// CHECK-NOT: attributes #0 = { {{.*}}patchable-function-entry{{.*}} }
+// CHECK-NOT: attributes #0 = { {{.*}}patchable-function-prefix{{.*}} }
+
+// CHECK: attributes #1 = { {{.*}}"patchable-function-entry"="2"{{.*}}"patchable-function-prefix"="1" {{.*}} }
+
+// CHECK: attributes #2 = { {{.*}}"patchable-function-entry"="3"{{.*}} }
+// CHECK-NOT: attributes #2 = { {{.*}}patchable-function-prefix{{.*}} }
+
+// CHECK: attributes #3 = { {{.*}}"patchable-function-prefix"="4"{{.*}} }
+// CHECK-NOT: attributes #3 = { {{.*}}patchable-function-entry{{.*}} }