Skip to content

Commit de42550

Browse files
borsbors
committed
Auto merge of #83302 - camsteffen:write-piece-unchecked, r=dtolnay
Get piece unchecked in `write` We already use specialized `zip`, but it seems like we can do a little better by not checking `pieces` length at all. `Arguments` constructors are now unsafe. So the `format_args!` expansion now includes an `unsafe` block. <details> <summary>Local Bench Diff</summary> ```text name before ns/iter after ns/iter diff ns/iter diff % speedup fmt::write_str_macro1 22,967 19,718 -3,249 -14.15% x 1.16 fmt::write_str_macro2 35,527 32,654 -2,873 -8.09% x 1.09 fmt::write_str_macro_debug 571,953 575,973 4,020 0.70% x 0.99 fmt::write_str_ref 9,579 9,459 -120 -1.25% x 1.01 fmt::write_str_value 9,573 9,572 -1 -0.01% x 1.00 fmt::write_u128_max 176 173 -3 -1.70% x 1.02 fmt::write_u128_min 138 134 -4 -2.90% x 1.03 fmt::write_u64_max 139 136 -3 -2.16% x 1.02 fmt::write_u64_min 129 135 6 4.65% x 0.96 fmt::write_vec_macro1 24,401 22,273 -2,128 -8.72% x 1.10 fmt::write_vec_macro2 37,096 35,602 -1,494 -4.03% x 1.04 fmt::write_vec_macro_debug 588,291 589,575 1,284 0.22% x 1.00 fmt::write_vec_ref 9,568 9,732 164 1.71% x 0.98 fmt::write_vec_value 9,516 9,625 109 1.15% x 0.99 ``` </details>
2 parents a49e38e + b1e07b8 commit de42550

File tree

14 files changed

+165
-65
lines changed

14 files changed

+165
-65
lines changed

Diff for: ‎compiler/rustc_builtin_macros/src/format.rs

+23-7
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,8 @@ use Position::*;
33

44
use rustc_ast as ast;
55
use rustc_ast::ptr::P;
6-
use rustc_ast::token;
76
use rustc_ast::tokenstream::TokenStream;
7+
use rustc_ast::{token, BlockCheckMode, UnsafeSource};
88
use rustc_data_structures::fx::{FxHashMap, FxHashSet};
99
use rustc_errors::{pluralize, Applicability, DiagnosticBuilder};
1010
use rustc_expand::base::{self, *};
@@ -838,12 +838,15 @@ impl<'a, 'b> Context<'a, 'b> {
838838
//
839839
// But the nested match expression is proved to perform not as well
840840
// as series of let's; the first approach does.
841-
let pat = self.ecx.pat_tuple(self.macsp, pats);
842-
let arm = self.ecx.arm(self.macsp, pat, args_array);
843-
let head = self.ecx.expr(self.macsp, ast::ExprKind::Tup(heads));
844-
let result = self.ecx.expr_match(self.macsp, head, vec![arm]);
841+
let args_match = {
842+
let pat = self.ecx.pat_tuple(self.macsp, pats);
843+
let arm = self.ecx.arm(self.macsp, pat, args_array);
844+
let head = self.ecx.expr(self.macsp, ast::ExprKind::Tup(heads));
845+
self.ecx.expr_match(self.macsp, head, vec![arm])
846+
};
845847

846-
let args_slice = self.ecx.expr_addr_of(self.macsp, result);
848+
let ident = Ident::from_str_and_span("args", self.macsp);
849+
let args_slice = self.ecx.expr_ident(self.macsp, ident);
847850

848851
// Now create the fmt::Arguments struct with all our locals we created.
849852
let (fn_name, fn_args) = if self.all_pieces_simple {
@@ -857,7 +860,20 @@ impl<'a, 'b> Context<'a, 'b> {
857860
};
858861

859862
let path = self.ecx.std_path(&[sym::fmt, sym::Arguments, Symbol::intern(fn_name)]);
860-
self.ecx.expr_call_global(self.macsp, path, fn_args)
863+
let arguments = self.ecx.expr_call_global(self.macsp, path, fn_args);
864+
let body = self.ecx.expr_block(P(ast::Block {
865+
stmts: vec![self.ecx.stmt_expr(arguments)],
866+
id: ast::DUMMY_NODE_ID,
867+
rules: BlockCheckMode::Unsafe(UnsafeSource::CompilerGenerated),
868+
span: self.macsp,
869+
tokens: None,
870+
}));
871+
872+
let ident = Ident::from_str_and_span("args", self.macsp);
873+
let binding_mode = ast::BindingMode::ByRef(ast::Mutability::Not);
874+
let pat = self.ecx.pat_ident_binding_mode(self.macsp, ident, binding_mode);
875+
let arm = self.ecx.arm(self.macsp, pat, body);
876+
self.ecx.expr_match(self.macsp, args_match, vec![arm])
861877
}
862878

863879
fn format_arg(

Diff for: ‎library/core/src/fmt/mod.rs

+39-3
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@
44

55
use crate::cell::{Cell, Ref, RefCell, RefMut, UnsafeCell};
66
use crate::char::EscapeDebugExtArgs;
7-
use crate::iter;
87
use crate::marker::PhantomData;
98
use crate::mem;
109
use crate::num::fmt as numfmt;
@@ -334,11 +333,29 @@ enum FlagV1 {
334333
impl<'a> Arguments<'a> {
335334
/// When using the format_args!() macro, this function is used to generate the
336335
/// Arguments structure.
336+
#[cfg(not(bootstrap))]
337+
#[doc(hidden)]
338+
#[inline]
339+
#[unstable(feature = "fmt_internals", reason = "internal to format_args!", issue = "none")]
340+
#[rustc_const_unstable(feature = "const_fmt_arguments_new", issue = "none")]
341+
pub const unsafe fn new_v1(
342+
pieces: &'a [&'static str],
343+
args: &'a [ArgumentV1<'a>],
344+
) -> Arguments<'a> {
345+
if pieces.len() < args.len() || pieces.len() > args.len() + 1 {
346+
panic!("invalid args");
347+
}
348+
Arguments { pieces, fmt: None, args }
349+
}
350+
#[cfg(bootstrap)]
337351
#[doc(hidden)]
338352
#[inline]
339353
#[unstable(feature = "fmt_internals", reason = "internal to format_args!", issue = "none")]
340354
#[rustc_const_unstable(feature = "const_fmt_arguments_new", issue = "none")]
341355
pub const fn new_v1(pieces: &'a [&'static str], args: &'a [ArgumentV1<'a>]) -> Arguments<'a> {
356+
if pieces.len() < args.len() || pieces.len() > args.len() + 1 {
357+
panic!("invalid args");
358+
}
342359
Arguments { pieces, fmt: None, args }
343360
}
344361

@@ -348,6 +365,19 @@ impl<'a> Arguments<'a> {
348365
/// `CountIsParam` or `CountIsNextParam` has to point to an argument
349366
/// created with `argumentusize`. However, failing to do so doesn't cause
350367
/// unsafety, but will ignore invalid .
368+
#[cfg(not(bootstrap))]
369+
#[doc(hidden)]
370+
#[inline]
371+
#[unstable(feature = "fmt_internals", reason = "internal to format_args!", issue = "none")]
372+
#[rustc_const_unstable(feature = "const_fmt_arguments_new", issue = "none")]
373+
pub const unsafe fn new_v1_formatted(
374+
pieces: &'a [&'static str],
375+
args: &'a [ArgumentV1<'a>],
376+
fmt: &'a [rt::v1::Argument],
377+
) -> Arguments<'a> {
378+
Arguments { pieces, fmt: Some(fmt), args }
379+
}
380+
#[cfg(bootstrap)]
351381
#[doc(hidden)]
352382
#[inline]
353383
#[unstable(feature = "fmt_internals", reason = "internal to format_args!", issue = "none")]
@@ -1110,7 +1140,10 @@ pub fn write(output: &mut dyn Write, args: Arguments<'_>) -> Result {
11101140
match args.fmt {
11111141
None => {
11121142
// We can use default formatting parameters for all arguments.
1113-
for (arg, piece) in iter::zip(args.args, args.pieces) {
1143+
for (i, arg) in args.args.iter().enumerate() {
1144+
// SAFETY: args.args and args.pieces come from the same Arguments,
1145+
// which guarantees the indexes are always within bounds.
1146+
let piece = unsafe { args.pieces.get_unchecked(i) };
11141147
if !piece.is_empty() {
11151148
formatter.buf.write_str(*piece)?;
11161149
}
@@ -1121,7 +1154,10 @@ pub fn write(output: &mut dyn Write, args: Arguments<'_>) -> Result {
11211154
Some(fmt) => {
11221155
// Every spec has a corresponding argument that is preceded by
11231156
// a string piece.
1124-
for (arg, piece) in iter::zip(fmt, args.pieces) {
1157+
for (i, arg) in fmt.iter().enumerate() {
1158+
// SAFETY: fmt and args.pieces come from the same Arguments,
1159+
// which guarantees the indexes are always within bounds.
1160+
let piece = unsafe { args.pieces.get_unchecked(i) };
11251161
if !piece.is_empty() {
11261162
formatter.buf.write_str(*piece)?;
11271163
}

Diff for: ‎library/core/src/lib.rs

+1
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,7 @@
111111
//
112112
// Language features:
113113
#![feature(abi_unadjusted)]
114+
#![feature(allow_internal_unsafe)]
114115
#![feature(allow_internal_unstable)]
115116
#![feature(asm)]
116117
#![feature(associated_type_bounds)]

Diff for: ‎library/core/src/macros/mod.rs

+1
Original file line numberDiff line numberDiff line change
@@ -828,6 +828,7 @@ pub(crate) mod builtin {
828828
/// assert_eq!(s, format!("hello {}", "world"));
829829
/// ```
830830
#[stable(feature = "rust1", since = "1.0.0")]
831+
#[allow_internal_unsafe]
831832
#[allow_internal_unstable(fmt_internals)]
832833
#[rustc_builtin_macro]
833834
#[macro_export]

Diff for: ‎library/core/src/panicking.rs

+9-1
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,15 @@ pub fn panic(expr: &'static str) -> ! {
4747
// truncation and padding (even though none is used here). Using
4848
// Arguments::new_v1 may allow the compiler to omit Formatter::pad from the
4949
// output binary, saving up to a few kilobytes.
50-
panic_fmt(fmt::Arguments::new_v1(&[expr], &[]));
50+
panic_fmt(
51+
#[cfg(bootstrap)]
52+
fmt::Arguments::new_v1(&[expr], &[]),
53+
#[cfg(not(bootstrap))]
54+
// SAFETY: Arguments::new_v1 is safe with exactly one str and zero args
55+
unsafe {
56+
fmt::Arguments::new_v1(&[expr], &[])
57+
},
58+
);
5159
}
5260

5361
#[inline]

Diff for: ‎src/test/debuginfo/rc_arc.rs

+3-1
Original file line numberDiff line numberDiff line change
@@ -75,5 +75,7 @@ fn main() {
7575
let a1 = Arc::clone(&a);
7676
let w2 = Arc::downgrade(&a);
7777

78-
print!(""); // #break
78+
zzz(); // #break
7979
}
80+
81+
fn zzz() { () }

Diff for: ‎src/test/pretty/dollar-crate.pp

+6-4
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,11 @@
1010

1111
fn main() {
1212
{
13-
::std::io::_print(::core::fmt::Arguments::new_v1(&["rust\n"],
14-
&match () {
15-
() => [],
16-
}));
13+
::std::io::_print(match match () { () => [], } {
14+
ref args => unsafe {
15+
::core::fmt::Arguments::new_v1(&["rust\n"],
16+
args)
17+
}
18+
});
1719
};
1820
}

Diff for: ‎src/test/pretty/issue-4264.pp

+33-23
Original file line numberDiff line numberDiff line change
@@ -32,29 +32,39 @@
3232
({
3333
let res =
3434
((::alloc::fmt::format as
35-
for<'r> fn(Arguments<'r>) -> String {format})(((::core::fmt::Arguments::new_v1
36-
as
37-
fn(&[&'static str], &[ArgumentV1]) -> Arguments {Arguments::new_v1})((&([("test"
38-
as
39-
&str)]
40-
as
41-
[&str; 1])
42-
as
43-
&[&str; 1]),
44-
(&(match (()
45-
as
46-
())
47-
{
48-
()
49-
=>
50-
([]
51-
as
52-
[ArgumentV1; 0]),
53-
}
54-
as
55-
[ArgumentV1; 0])
56-
as
57-
&[ArgumentV1; 0]))
35+
for<'r> fn(Arguments<'r>) -> String {format})((match (match (()
36+
as
37+
())
38+
{
39+
()
40+
=>
41+
([]
42+
as
43+
[ArgumentV1; 0]),
44+
}
45+
as
46+
[ArgumentV1; 0])
47+
{
48+
ref args
49+
=>
50+
unsafe
51+
{
52+
((::core::fmt::Arguments::new_v1
53+
as
54+
unsafe fn(&[&'static str], &[ArgumentV1]) -> Arguments {Arguments::new_v1})((&([("test"
55+
as
56+
&str)]
57+
as
58+
[&str; 1])
59+
as
60+
&[&str; 1]),
61+
(args
62+
as
63+
&[ArgumentV1; 0]))
64+
as
65+
Arguments)
66+
}
67+
}
5868
as
5969
Arguments))
6070
as String);

Diff for: ‎src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.issue-84561.txt

+3-3
Original file line numberDiff line numberDiff line change
@@ -31,15 +31,15 @@
3131
24| 1| println!("{:?}", Foo(1));
3232
25| 1|
3333
26| 1| assert_ne!(Foo(0), Foo(5), "{}", if is_true { "true message" } else { "false message" });
34-
^0 ^0 ^0
34+
^0 ^0 ^0 ^0
3535
27| 1| assert_ne!(
3636
28| | Foo(0)
3737
29| | ,
3838
30| | Foo(5)
3939
31| | ,
4040
32| 0| "{}"
41-
33| 0| ,
42-
34| 0| if
41+
33| | ,
42+
34| | if
4343
35| 0| is_true
4444
36| | {
4545
37| 0| "true message"

Diff for: ‎src/test/ui/attributes/key-value-expansion.stderr

+10-6
Original file line numberDiff line numberDiff line change
@@ -17,12 +17,16 @@ LL | bug!();
1717

1818
error: unexpected token: `{
1919
let res =
20-
::alloc::fmt::format(::core::fmt::Arguments::new_v1(&[""],
21-
&match (&"u8",) {
22-
(arg0,) =>
23-
[::core::fmt::ArgumentV1::new(arg0,
24-
::core::fmt::Display::fmt)],
25-
}));
20+
::alloc::fmt::format(match match (&"u8",) {
21+
(arg0,) =>
22+
[::core::fmt::ArgumentV1::new(arg0,
23+
::core::fmt::Display::fmt)],
24+
} {
25+
ref args => unsafe {
26+
::core::fmt::Arguments::new_v1(&[""],
27+
args)
28+
}
29+
});
2630
res
2731
}.as_str()`
2832
--> $DIR/key-value-expansion.rs:48:23

Diff for: ‎src/test/ui/unsafe/unsafe-around-compiler-generated-unsafe.mir.stderr

+7-1
Original file line numberDiff line numberDiff line change
@@ -10,5 +10,11 @@ note: the lint level is defined here
1010
LL | #![deny(unused_unsafe)]
1111
| ^^^^^^^^^^^^^
1212

13-
error: aborting due to previous error
13+
error: unnecessary `unsafe` block
14+
--> $DIR/unsafe-around-compiler-generated-unsafe.rs:13:5
15+
|
16+
LL | unsafe { println!("foo"); }
17+
| ^^^^^^ unnecessary `unsafe` block
18+
19+
error: aborting due to 2 previous errors
1420

Diff for: ‎src/test/ui/unsafe/unsafe-around-compiler-generated-unsafe.rs

+3
Original file line numberDiff line numberDiff line change
@@ -8,4 +8,7 @@ fn main() {
88
let _ = async {
99
unsafe { async {}.await; } //~ ERROR unnecessary `unsafe`
1010
};
11+
12+
// `format_args!` expands with a compiler-generated unsafe block
13+
unsafe { println!("foo"); } //~ ERROR unnecessary `unsafe`
1114
}

Diff for: ‎src/test/ui/unsafe/unsafe-around-compiler-generated-unsafe.thir.stderr

+7-1
Original file line numberDiff line numberDiff line change
@@ -10,5 +10,11 @@ note: the lint level is defined here
1010
LL | #![deny(unused_unsafe)]
1111
| ^^^^^^^^^^^^^
1212

13-
error: aborting due to previous error
13+
error: unnecessary `unsafe` block
14+
--> $DIR/unsafe-around-compiler-generated-unsafe.rs:13:5
15+
|
16+
LL | unsafe { println!("foo"); }
17+
| ^^^^^^ unnecessary `unsafe` block
18+
19+
error: aborting due to 2 previous errors
1420

0 commit comments

Comments
 (0)