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.

Sign up for GitHub

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

Use ZST for fmt unsafety #89139

Merged
merged 2 commits into from
Sep 23, 2021
Merged

Use ZST for fmt unsafety #89139

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
09b37d7
Use ZST for fmt unsafety
camsteffen Sep 21, 2021
2efa9d7
Fix test
camsteffen Sep 22, 2021
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
32 changes: 14 additions & 18 deletions compiler/rustc_builtin_macros/src/format.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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(
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_span/src/symbol.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,7 @@ symbols! {
TyCtxt,
TyKind,
Unknown,
UnsafeArg,
Vec,
Yield,
_DECLS,
Expand Down
53 changes: 44 additions & 9 deletions library/core/src/fmt/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
camsteffen marked this conversation as resolved.
Show resolved Hide resolved

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)]
Mark-Simulacrum marked this conversation as resolved.
Show resolved Hide resolved
pub unsafe fn new() -> Self {
Self
}
}

// This guarantees a single stable value for the function pointer associated with
// indices/counts in the formatting infrastructure.
//
Expand Down Expand Up @@ -337,22 +357,37 @@ 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> {
Mark-Simulacrum marked this conversation as resolved.
Show resolved Hide resolved
if pieces.len() < args.len() || pieces.len() > args.len() + 1 {
panic!("invalid args");
}
Arguments { pieces, fmt: None, args }
}

/// 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")]
Expand Down
5 changes: 1 addition & 4 deletions library/core/src/panicking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
10 changes: 4 additions & 6 deletions src/test/pretty/dollar-crate.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 () {
() => [],
}));
};
}
56 changes: 23 additions & 33 deletions src/test/pretty/issue-4264.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
6 changes: 3 additions & 3 deletions src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.issue-84561.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
16 changes: 6 additions & 10 deletions src/test/ui/attributes/key-value-expansion.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
35 changes: 15 additions & 20 deletions src/tools/clippy/clippy_utils/src/higher.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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,
Expand Down