From 09b37d743328bd497939bd27135f82350f1b0bd7 Mon Sep 17 00:00:00 2001 From: Cameron Steffen Date: Tue, 21 Sep 2021 00:56:45 -0500 Subject: [PATCH 1/2] Use ZST for fmt unsafety This allows the format_args! macro to keep the pre-expansion code out of the unsafe block without doing gymnastics with nested `match` expressions. This reduces codegen. --- compiler/rustc_builtin_macros/src/format.rs | 32 +++++------ compiler/rustc_span/src/symbol.rs | 1 + library/core/src/fmt/mod.rs | 53 +++++++++++++++--- library/core/src/panicking.rs | 5 +- src/test/pretty/dollar-crate.pp | 10 ++-- src/test/pretty/issue-4264.pp | 56 ++++++++----------- .../ui/attributes/key-value-expansion.stderr | 16 ++---- src/tools/clippy/clippy_utils/src/higher.rs | 35 +++++------- 8 files changed, 108 insertions(+), 100 deletions(-) diff --git a/compiler/rustc_builtin_macros/src/format.rs b/compiler/rustc_builtin_macros/src/format.rs index c7626dec4d7c0..9b9adc2d7f3b8 100644 --- a/compiler/rustc_builtin_macros/src/format.rs +++ b/compiler/rustc_builtin_macros/src/format.rs @@ -845,8 +845,7 @@ impl<'a, 'b> Context<'a, 'b> { self.ecx.expr_match(self.macsp, head, vec![arm]) }; - let ident = Ident::from_str_and_span("args", self.macsp); - let args_slice = self.ecx.expr_ident(self.macsp, ident); + let args_slice = self.ecx.expr_addr_of(self.macsp, args_match); // Now create the fmt::Arguments struct with all our locals we created. let (fn_name, fn_args) = if self.all_pieces_simple { @@ -856,25 +855,22 @@ impl<'a, 'b> Context<'a, 'b> { // nonstandard placeholders, if there are any. let fmt = self.ecx.expr_vec_slice(self.macsp, self.pieces); - ("new_v1_formatted", vec![pieces, args_slice, fmt]) + let path = self.ecx.std_path(&[sym::fmt, sym::UnsafeArg, sym::new]); + let unsafe_arg = self.ecx.expr_call_global(self.macsp, path, Vec::new()); + let unsafe_expr = self.ecx.expr_block(P(ast::Block { + stmts: vec![self.ecx.stmt_expr(unsafe_arg)], + id: ast::DUMMY_NODE_ID, + rules: BlockCheckMode::Unsafe(UnsafeSource::CompilerGenerated), + span: self.macsp, + tokens: None, + could_be_bare_literal: false, + })); + + ("new_v1_formatted", vec![pieces, args_slice, fmt, unsafe_expr]) }; let path = self.ecx.std_path(&[sym::fmt, sym::Arguments, Symbol::intern(fn_name)]); - let arguments = self.ecx.expr_call_global(self.macsp, path, fn_args); - let body = self.ecx.expr_block(P(ast::Block { - stmts: vec![self.ecx.stmt_expr(arguments)], - id: ast::DUMMY_NODE_ID, - rules: BlockCheckMode::Unsafe(UnsafeSource::CompilerGenerated), - span: self.macsp, - tokens: None, - could_be_bare_literal: false, - })); - - let ident = Ident::from_str_and_span("args", self.macsp); - let binding_mode = ast::BindingMode::ByRef(ast::Mutability::Not); - let pat = self.ecx.pat_ident_binding_mode(self.macsp, ident, binding_mode); - let arm = self.ecx.arm(self.macsp, pat, body); - self.ecx.expr_match(self.macsp, args_match, vec![arm]) + self.ecx.expr_call_global(self.macsp, path, fn_args) } fn format_arg( diff --git a/compiler/rustc_span/src/symbol.rs b/compiler/rustc_span/src/symbol.rs index 322bea3806cfa..be6f5fc297890 100644 --- a/compiler/rustc_span/src/symbol.rs +++ b/compiler/rustc_span/src/symbol.rs @@ -253,6 +253,7 @@ symbols! { TyCtxt, TyKind, Unknown, + UnsafeArg, Vec, Yield, _DECLS, diff --git a/library/core/src/fmt/mod.rs b/library/core/src/fmt/mod.rs index 166a8e3f28a41..8fa941c42fc21 100644 --- a/library/core/src/fmt/mod.rs +++ b/library/core/src/fmt/mod.rs @@ -265,6 +265,26 @@ pub struct ArgumentV1<'a> { formatter: fn(&Opaque, &mut Formatter<'_>) -> Result, } +/// This struct represents the unsafety of constructing an `Arguments`. +/// It exists, rather than an unsafe function, in order to simplify the expansion +/// of `format_args!(..)` and reduce the scope of the `unsafe` block. +#[allow(missing_debug_implementations)] +#[doc(hidden)] +#[non_exhaustive] +#[unstable(feature = "fmt_internals", reason = "internal to format_args!", issue = "none")] +pub struct UnsafeArg; + +impl UnsafeArg { + /// See documentation where `UnsafeArg` is required to know when it is safe to + /// create and use `UnsafeArg`. + #[doc(hidden)] + #[unstable(feature = "fmt_internals", reason = "internal to format_args!", issue = "none")] + #[inline(always)] + pub unsafe fn new() -> Self { + Self + } +} + // This guarantees a single stable value for the function pointer associated with // indices/counts in the formatting infrastructure. // @@ -337,10 +357,7 @@ impl<'a> Arguments<'a> { #[inline] #[unstable(feature = "fmt_internals", reason = "internal to format_args!", issue = "none")] #[rustc_const_unstable(feature = "const_fmt_arguments_new", issue = "none")] - pub const unsafe fn new_v1( - pieces: &'a [&'static str], - args: &'a [ArgumentV1<'a>], - ) -> Arguments<'a> { + pub const fn new_v1(pieces: &'a [&'static str], args: &'a [ArgumentV1<'a>]) -> Arguments<'a> { if pieces.len() < args.len() || pieces.len() > args.len() + 1 { panic!("invalid args"); } @@ -348,11 +365,29 @@ impl<'a> Arguments<'a> { } /// This function is used to specify nonstandard formatting parameters. - /// The `pieces` array must be at least as long as `fmt` to construct - /// a valid Arguments structure. Also, any `Count` within `fmt` that is - /// `CountIsParam` or `CountIsNextParam` has to point to an argument - /// created with `argumentusize`. However, failing to do so doesn't cause - /// unsafety, but will ignore invalid . + /// + /// An `UnsafeArg` is required because the following invariants must be held + /// in order for this function to be safe: + /// 1. The `pieces` slice must be at least as long as `fmt`. + /// 2. Every [`rt::v1::Argument::position`] value within `fmt` must be a + /// valid index of `args`. + /// 3. Every [`Count::Param`] within `fmt` must contain a valid index of + /// `args`. + #[cfg(not(bootstrap))] + #[doc(hidden)] + #[inline] + #[unstable(feature = "fmt_internals", reason = "internal to format_args!", issue = "none")] + #[rustc_const_unstable(feature = "const_fmt_arguments_new", issue = "none")] + pub const fn new_v1_formatted( + pieces: &'a [&'static str], + args: &'a [ArgumentV1<'a>], + fmt: &'a [rt::v1::Argument], + _unsafe_arg: UnsafeArg, + ) -> Arguments<'a> { + Arguments { pieces, fmt: Some(fmt), args } + } + + #[cfg(bootstrap)] #[doc(hidden)] #[inline] #[unstable(feature = "fmt_internals", reason = "internal to format_args!", issue = "none")] diff --git a/library/core/src/panicking.rs b/library/core/src/panicking.rs index a6aa4bf43c865..6d3ec6ae8612a 100644 --- a/library/core/src/panicking.rs +++ b/library/core/src/panicking.rs @@ -47,10 +47,7 @@ pub fn panic(expr: &'static str) -> ! { // truncation and padding (even though none is used here). Using // Arguments::new_v1 may allow the compiler to omit Formatter::pad from the // output binary, saving up to a few kilobytes. - panic_fmt( - // SAFETY: Arguments::new_v1 is safe with exactly one str and zero args - unsafe { fmt::Arguments::new_v1(&[expr], &[]) }, - ); + panic_fmt(fmt::Arguments::new_v1(&[expr], &[])); } #[inline] diff --git a/src/test/pretty/dollar-crate.pp b/src/test/pretty/dollar-crate.pp index 4eccba06b134f..f4be3c1c63a84 100644 --- a/src/test/pretty/dollar-crate.pp +++ b/src/test/pretty/dollar-crate.pp @@ -10,11 +10,9 @@ fn main() { { - ::std::io::_print(match match () { () => [], } { - ref args => unsafe { - ::core::fmt::Arguments::new_v1(&["rust\n"], - args) - } - }); + ::std::io::_print(::core::fmt::Arguments::new_v1(&["rust\n"], + &match () { + () => [], + })); }; } diff --git a/src/test/pretty/issue-4264.pp b/src/test/pretty/issue-4264.pp index a21ea520121e3..199aee05622be 100644 --- a/src/test/pretty/issue-4264.pp +++ b/src/test/pretty/issue-4264.pp @@ -32,39 +32,29 @@ ({ let res = ((::alloc::fmt::format as - for<'r> fn(Arguments<'r>) -> String {format})((match (match (() - as - ()) - { - () - => - ([] - as - [ArgumentV1; 0]), - } - as - [ArgumentV1; 0]) - { - ref args - => - unsafe - { - ((::core::fmt::Arguments::new_v1 - as - unsafe fn(&[&'static str], &[ArgumentV1]) -> Arguments {Arguments::new_v1})((&([("test" - as - &str)] - as - [&str; 1]) - as - &[&str; 1]), - (args - as - &[ArgumentV1; 0])) - as - Arguments) - } - } + for<'r> fn(Arguments<'r>) -> String {format})(((::core::fmt::Arguments::new_v1 + as + fn(&[&'static str], &[ArgumentV1]) -> Arguments {Arguments::new_v1})((&([("test" + as + &str)] + as + [&str; 1]) + as + &[&str; 1]), + (&(match (() + as + ()) + { + () + => + ([] + as + [ArgumentV1; 0]), + } + as + [ArgumentV1; 0]) + as + &[ArgumentV1; 0])) as Arguments)) as String); diff --git a/src/test/ui/attributes/key-value-expansion.stderr b/src/test/ui/attributes/key-value-expansion.stderr index 03ca515265cb0..31e93ef54f260 100644 --- a/src/test/ui/attributes/key-value-expansion.stderr +++ b/src/test/ui/attributes/key-value-expansion.stderr @@ -17,16 +17,12 @@ LL | bug!(); error: unexpected token: `{ let res = - ::alloc::fmt::format(match match (&"u8",) { - (arg0,) => - [::core::fmt::ArgumentV1::new(arg0, - ::core::fmt::Display::fmt)], - } { - ref args => unsafe { - ::core::fmt::Arguments::new_v1(&[""], - args) - } - }); + ::alloc::fmt::format(::core::fmt::Arguments::new_v1(&[""], + &match (&"u8",) { + (arg0,) => + [::core::fmt::ArgumentV1::new(arg0, + ::core::fmt::Display::fmt)], + })); res }.as_str()` --> $DIR/key-value-expansion.rs:48:23 diff --git a/src/tools/clippy/clippy_utils/src/higher.rs b/src/tools/clippy/clippy_utils/src/higher.rs index 05a4a01431950..ff55ff35a1343 100644 --- a/src/tools/clippy/clippy_utils/src/higher.rs +++ b/src/tools/clippy/clippy_utils/src/higher.rs @@ -524,28 +524,12 @@ impl FormatArgsExpn<'tcx> { if let ExpnKind::Macro(_, name) = expr.span.ctxt().outer_expn_data().kind; let name = name.as_str(); if name.ends_with("format_args") || name.ends_with("format_args_nl"); - - if let ExprKind::Match(inner_match, [arm], _) = expr.kind; - - // `match match`, if you will - if let ExprKind::Match(args, [inner_arm], _) = inner_match.kind; - if let ExprKind::Tup(value_args) = args.kind; - if let Some(value_args) = value_args - .iter() - .map(|e| match e.kind { - ExprKind::AddrOf(_, _, e) => Some(e), - _ => None, - }) - .collect(); - if let ExprKind::Array(args) = inner_arm.body.kind; - - if let ExprKind::Block(Block { stmts: [], expr: Some(expr), .. }, _) = arm.body.kind; - if let ExprKind::Call(_, call_args) = expr.kind; - if let Some((strs_ref, fmt_expr)) = match call_args { + if let ExprKind::Call(_, args) = expr.kind; + if let Some((strs_ref, args, fmt_expr)) = match args { // Arguments::new_v1 - [strs_ref, _] => Some((strs_ref, None)), + [strs_ref, args] => Some((strs_ref, args, None)), // Arguments::new_v1_formatted - [strs_ref, _, fmt_expr] => Some((strs_ref, Some(fmt_expr))), + [strs_ref, args, fmt_expr, _unsafe_arg] => Some((strs_ref, args, Some(fmt_expr))), _ => None, }; if let ExprKind::AddrOf(BorrowKind::Ref, _, strs_arr) = strs_ref.kind; @@ -561,6 +545,17 @@ impl FormatArgsExpn<'tcx> { None }) .collect(); + if let ExprKind::AddrOf(BorrowKind::Ref, _, args) = args.kind; + if let ExprKind::Match(args, [arm], _) = args.kind; + if let ExprKind::Tup(value_args) = args.kind; + if let Some(value_args) = value_args + .iter() + .map(|e| match e.kind { + ExprKind::AddrOf(_, _, e) => Some(e), + _ => None, + }) + .collect(); + if let ExprKind::Array(args) = arm.body.kind; then { Some(FormatArgsExpn { format_string_span: strs_ref.span, From 2efa9d796981163031a7478734fee561dc3a6da0 Mon Sep 17 00:00:00 2001 From: Cameron Steffen Date: Wed, 22 Sep 2021 11:48:01 -0500 Subject: [PATCH 2/2] Fix test --- .../coverage-reports/expected_show_coverage.issue-84561.txt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.issue-84561.txt b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.issue-84561.txt index b35f3f54de904..f24f7c6940473 100644 --- a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.issue-84561.txt +++ b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.issue-84561.txt @@ -31,15 +31,15 @@ 24| 1| println!("{:?}", Foo(1)); 25| 1| 26| 1| assert_ne!(Foo(0), Foo(5), "{}", if is_true { "true message" } else { "false message" }); - ^0 ^0 ^0 ^0 + ^0 ^0 ^0 27| 1| assert_ne!( 28| | Foo(0) 29| | , 30| | Foo(5) 31| | , 32| 0| "{}" - 33| | , - 34| | if + 33| 0| , + 34| 0| if 35| 0| is_true 36| | { 37| 0| "true message"