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

Add libstd Cargo feature "panic_immediate_abort" #55011

Merged
merged 3 commits into from Nov 30, 2018

Conversation

Projects
None yet
@vi
Contributor

vi commented Oct 12, 2018

It stop asserts and panics from libstd to automatically
include string output and formatting code.

Use case: developing static executables smaller than 50 kilobytes,
where usual formatting code is excessive while keeping debuggability
in debug mode.

May resolve #54981.

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Oct 12, 2018

r? @KodrAus

(rust_highfive has picked a reviewer for you, use r? to override)

@Mark-Simulacrum

This comment has been minimized.

Member

Mark-Simulacrum commented Oct 12, 2018

I suspect that if you're willing to compile std etc yourself using -Cpanic=abort would be the preferred approach, but I'm not sure.

@vi

This comment has been minimized.

Contributor

vi commented Oct 12, 2018

This is the next step after -Cpanic=abort and force_alloc_system and opt-level="z".

Even with -Cpanic=abort it places all those messages and formatting code into executable.


For info: stripped file size of a {simple program that checks password against sha512 hash and executes supplied program on success} after each step, for arm-unknown-linux-musleabi:

Step Size
Usual debug 506K
Release (no LTO) 470K
Release (with LTO, codegen-units=1) 466K
All above + opt-level=z 438K
All above + panic=abort 426K
All above + crate-type staticlib no jemalloc 211K
All above + Xargo force_alloc_system 69K
All above + panic_immediate_abort 29K
@bors

This comment has been minimized.

Contributor

bors commented Oct 13, 2018

☔️ The latest upstream changes (presumably #54951) made this pull request unmergeable. Please resolve the merge conflicts.

@estebank

This comment has been minimized.

Contributor

estebank commented Oct 15, 2018

Sidenote: @vi do not merge master when merge conflicts arise, rebase against master on your PRs.

@vi vi force-pushed the vi:panic_immediate_abort branch from 44b0e23 to d48a11a Oct 15, 2018

@KodrAus

This comment has been minimized.

KodrAus commented Oct 21, 2018

Thanks for your patience @vi!

Hmm, am I reading right that the case you've got is where you've been careful not to interact with the formatting machinery at all because it's heavy, but it's still finding its way in through panic, even though you just want those panics to abort and so there's no need to do any formatting?

On the surface adding a knob to patch up this case seems like it would be brittle, but I don't work in environments where this is really important so don't really understand your workflow.

Even with -Cpanic=abort it places all those messages and formatting code into executable.

This seems a little surprising to me. Is that what we expect?

cc @alexcrichton @japaric who would understand this much better than I do.

2 similar comments
@KodrAus

This comment was marked as resolved.

KodrAus commented Oct 21, 2018

Thanks for your patience @vi!

Hmm, am I reading right that the case you've got is where you've been careful not to interact with the formatting machinery at all because it's heavy, but it's still finding its way in through panic, even though you just want those panics to abort and so there's no need to do any formatting?

On the surface adding a knob to patch up this case seems like it would be brittle, but I don't work in environments where this is really important so don't really understand your workflow.

Even with -Cpanic=abort it places all those messages and formatting code into executable.

This seems a little surprising to me. Is that what we expect?

cc @alexcrichton @japaric who would understand this much better than I do.

@KodrAus

This comment was marked as resolved.

KodrAus commented Oct 21, 2018

Thanks for your patience @vi!

Hmm, am I reading right that the case you've got is where you've been careful not to interact with the formatting machinery at all because it's heavy, but it's still finding its way in through panic, even though you just want those panics to abort and so there's no need to do any formatting?

On the surface adding a knob to patch up this case seems like it would be brittle, but I don't work in environments where this is really important so don't really understand your workflow.

Even with -Cpanic=abort it places all those messages and formatting code into executable.

This seems a little surprising to me. Is that what we expect?

cc @alexcrichton @japaric who would understand this much better than I do.

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Oct 22, 2018

Thanks for the PR here! I think, though, that for this problem we probably want to tackle this a slightly different way. I've been thinking that if we want to avoid panicking/formatting infrastructure then we should generate binaries that, at the codegen level, never emit the information. As written this still relies on LLVM's LTO to get rid of formatting infrastructure, although this does get rid of the panicking infrastructure.

Is there a way we could perhaps guarantee both get eliminated?

@TimNN

This comment has been minimized.

Contributor

TimNN commented Nov 6, 2018

Ping from triage @everyone! How do you think this PR can move forward? Does the suggested approach seem feasible at all?

@vi

This comment has been minimized.

Contributor

vi commented Nov 6, 2018

S-waiting-on-author

Shall I try doing something myself? Shall the patch change more things, but avoid relying on TCOLTO?

I also expect formatting to be still available if one explicitly uses some format!, println! or log (without release_max_level_off), so relying on some form of dead code removal seems to be good idea.

Also simple and easy-ish approach like this patch can be made available now, with proper solution available someday.

@TimNN

This comment has been minimized.

Contributor

TimNN commented Nov 6, 2018

S-waiting-on-author

Shall I try doing something myself? Shall the patch change more things, but avoid relying on TCO?

I added this label last week, IIRC mainly because I had seen @alexcrichton's question in his last comment.

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Nov 7, 2018

I would personally either prefer a solution which adds this feature at the source, not even passing panic information into libstd, or one which is a little less invasive than sprinkling a few #[cfg] throughout libstd.

For the former solution I think it's more work to get done because it would require changing the definition of the panic! macro itself. This would probably compile out a number of modules in core/std.

For the latter solution I think this can probably get by with:

if cfg!(feature = "panic_immediate_abort") {
    unsafe { ::intrinsics::abort() }
}

in a few locations.

@Dylan-DPC

This comment has been minimized.

Member

Dylan-DPC commented Nov 19, 2018

Ping from triage, @alexcrichton @vi what's the status on this?

@vi

This comment has been minimized.

Contributor

vi commented Nov 19, 2018

Idling, not sure what to do next.

I though about changing panic! itself when I was experimenting, but didn't find it in libstd itself and considered that it's too tricky to dig in further into rustc.

reason = "used by the panic! macro",
issue = "0")]
#[cfg(feature="panic_immediate_abort")]
#[inline(never)] #[cold] // avoid code bloat at the call sites as much as possible

This comment has been minimized.

@bjorn3

bjorn3 Nov 19, 2018

Contributor

Shouldn't this be #[inline]. A call may be longer than ud2.

This comment has been minimized.

@vi

vi Nov 19, 2018

Contributor

Probably. Inline line is just a copy-paste from the other variant of the function.

@bjorn3

This comment has been minimized.

Contributor

bjorn3 commented Nov 19, 2018

I though about changing panic! itself when I was experimenting, but didn't find it in libstd itself

Its in libcore.

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Nov 20, 2018

I've already outlined my thoughts on this. @vi do you have questions about my thinking?

@vi

This comment has been minimized.

Contributor

vi commented Nov 20, 2018

(two solutions)

Is it worth to do the second, easier approach first, postponing the first, complexier approach to possible further pull requests?

For the latter solution I think this can probably get by with (if cfg!/abort) in a few locations.

Isn't it what this pull request attempts to do? Or do you mean the pull request should be changed to use cfg! instead of #[cfg]?

in a few locations

Or maybe I got those locations wrong and abort should be somewhere else?

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Nov 21, 2018

I think it's fine to start here, but yes I would prefer to switch to cfg! instead of #[cfg] so this can be tested on CI if it's the "quick and dirty" approach

@vi

This comment has been minimized.

Contributor

vi commented Nov 22, 2018

Patching panic! macro itself in libstd and libcore seems to be cleaner solution.

But if cfg! does not work there (it checks for features of current crate using panic! instead of features of libstd), so duplication may still be necessary.

@vi vi force-pushed the vi:panic_immediate_abort branch from d48a11a to aae24d4 Nov 23, 2018

@vi

This comment has been minimized.

Contributor

vi commented Nov 23, 2018

@alexcrichton Prepared second version of the easy approach.

  • Primary decision switching point is shifted to the panic! macro itself;
  • libcore is now also involved;
  • Macro is duplicated (cfg vs no cfg) because of if cfg! inside macro_rules checks target crate's feature, not libstd/libcore's feature.
  • Some core::panicking:: functions are hacked if if cfg! because of cases like let q = [1,2,3]; q[4] triggering panic without involvement of panic! macro. I'm not sure if it is enough (no missing cases) and necessary (no excessive unneeded if cfg!s).

Currently recommended Xargo.toml to take advantage of the feature:

[dependencies]
std = {default-features=false, features=["panic_immediate_abort"]}
core = {default-features=false, features=["panic_immediate_abort"]}  

and #[no_main] (otherwise it includes formatting even when fn main() -> !).


Question: should there be non-default panic_immediate_abort feature to turn off usual Rust panic handling or default feature panic_handling that turns on usual Rust panic handling?

@bors

This comment has been minimized.

Contributor

bors commented Nov 30, 2018

📌 Commit f18a8c6 has been approved by alexcrichton

@RalfJung

This comment has been minimized.

Member

RalfJung commented Nov 30, 2018

May resolve #54981.

Note: Currently, this PR (when merged) will close that issue (because GH thinks it is smart). Considering that you said "may resolve", you may want to change that.

bors added a commit that referenced this pull request Nov 30, 2018

Auto merge of #55011 - vi:panic_immediate_abort, r=alexcrichton
Add libstd Cargo feature "panic_immediate_abort"

It stop asserts and panics from libstd to automatically
include string output and formatting code.

Use case: developing static executables smaller than 50 kilobytes,
where usual formatting code is excessive while keeping debuggability
in debug mode.

May resolve #54981.
@bors

This comment has been minimized.

Contributor

bors commented Nov 30, 2018

⌛️ Testing commit f18a8c6 with merge f0458c2...

@vi

This comment has been minimized.

Contributor

vi commented Nov 30, 2018

@RalfJung, It is mostly intentional.

With this feature merged in, the issue becomes workaroundable. I'll add a comment there after this is merged.

@bors

This comment has been minimized.

Contributor

bors commented Nov 30, 2018

💔 Test failed - status-appveyor

@vi

This comment has been minimized.

Contributor

vi commented Nov 30, 2018

Test failed

test [run-make] run-make-fulldeps\c-link-to-rust-va-list-fn ... FAILED
...
thread '<unnamed>' panicked at 'attempt to create unaligned slice', src\libcore\slice\mod.rs:4802:5

Seems unrelated...

@RalfJung

This comment has been minimized.

Member

RalfJung commented Nov 30, 2018

That seems related to #49878, actually. But why was it not caught there?

@dlrobertson

This comment has been minimized.

Contributor

dlrobertson commented Nov 30, 2018

That seems related to #49878, actually.

Yeah, seems to be an issue with the test.

But why was it not caught there?

No idea why it wasn't caught in the many tests I ran.

@kennytm

This comment has been minimized.

Member

kennytm commented Nov 30, 2018

let's retry @bors

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Nov 30, 2018

cc @dlrobertson this failure looks like there may be a spurious bug in the va_list tests added maybe?

@dlrobertson

This comment has been minimized.

Contributor

dlrobertson commented Nov 30, 2018

@alexcrichton Thanks, I've got some tests running just to make sure it is a issue with the tests. If it is, I'll have a fix up shortly.

kennytm added a commit to kennytm/rust that referenced this pull request Nov 30, 2018

Rollup merge of rust-lang#55011 - vi:panic_immediate_abort, r=alexcri…
…chton

Add libstd Cargo feature "panic_immediate_abort"

It stop asserts and panics from libstd to automatically
include string output and formatting code.

Use case: developing static executables smaller than 50 kilobytes,
where usual formatting code is excessive while keeping debuggability
in debug mode.

May resolve rust-lang#54981.

bors added a commit that referenced this pull request Nov 30, 2018

Auto merge of #56381 - kennytm:rollup, r=kennytm
Rollup of 19 pull requests

Successful merges:

 - #55011 (Add libstd Cargo feature "panic_immediate_abort")
 - #55821 (Use sort_by_cached_key when the key function is not trivial/free)
 - #56014 (add test for issue #21335)
 - #56131 (Assorted tweaks)
 - #56214 (Implement chalk unification routines)
 - #56216 (Add TryFrom<&[T]> for [T; $N] where T: Copy)
 - #56268 (Reuse the `P` in `InvocationCollector::fold_{,opt_}expr`.)
 - #56324 (Use raw_entry for more efficient interning)
 - #56336 (Clean up and streamline the pretty-printer)
 - #56337 (Fix const_fn ICE with non-const function pointer)
 - #56339 (Remove not used option)
 - #56341 (Rename conversion util; remove duplicate util in librustc_codegen_llvm.)
 - #56349 (rustc 1.30.0's linker flavor inference is a non-backwards compat change to -Clinker)
 - #56355 (Add inline attributes and add unit to CommonTypes)
 - #56360 (Optimize local linkchecker program)
 - #56364 (Fix panic with outlives in existential type)
 - #56365 (Stabilize self_struct_ctor feature.)
 - #56367 (Moved some feature gate tests to correct location)
 - #56373 (Update books)

@bors bors merged commit f18a8c6 into rust-lang:master Nov 30, 2018

1 of 2 checks passed

homu Test failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

bors added a commit that referenced this pull request Dec 2, 2018

Auto merge of #56396 - dlrobertson:fix_va_list_tests, r=nikic
tests: Simplify VaList run-make test

The va_list tests were too complex and were causing some spurious
test failures on Windows.

Example: #55011 (comment)
@johnthagen

This comment has been minimized.

Contributor

johnthagen commented Dec 11, 2018

@vi / @alexcrichton Is there an example somewhere that shows how to use panic_immediate_abort? And is Xargo required? I'd like to add this (and any other techniques I've missed) to my min-sized-rust repo so that there is an up to date reference on this topic people can reference.

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Dec 11, 2018

@johnthagen I don't know of examples myself, but yeah Xargo would be needed and would be used to enable this feature

@vi

This comment has been minimized.

Contributor

vi commented Dec 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment