You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Default handlers get mono-morphized needlessly #19
Since #17, identical copies of exception::default_handler get included in the binary, thanks to the token generic parameter. It's not a lot of code, but it seems a bit unfortunate. Is there any way around this?
Thanks for all the work you do for embedded Rust, btw! <3
Not that I know of. I was hoping LLVM would merge the identical functions but it doesn't. I don't know if it's capable of doing that at all.
cc @eddyb Do you know if LLVM should be able to optimize something like this and we are not giving it enough information or maybe it's not possible to do at all:
extern"C"fndefault_handler<T>(_:T){// code that doesn't use `T` at all}structFoo;structBar;structBaz;structQuux;#[repr(C)]structExceptions{foo:extern"C"fn(Foo),bar:extern"C"fn(Bar),baz:extern"C"fn(Baz),quux:extern"C"fn(Quux),}#[no_mangle]pubstatic _EXCEPTIONS:Exceptions = Exceptions{foo: default_handler,bar: default_handler,baz: default_handler,quux: default_handler,};
Today, we get something like this (release + LTO):
@japaric You need -C opt-level=3 IIRC, and even then it might be disabled.
You can try -C passes=mergefunc to force it to run, but I'm not sure it will be placed right to do its thing.
Thanks for the suggestion, @eddyb. Unfortunately, it didn't work. We were already using -C opt-level=3 because of Cargo's --release flag and recompiling the world with -C passes didn't help. -C passes actually made one test program 8 bytes larger (originally, 1270 bytes in size) 😆.
Running a debug opt with -debug -debug-only mergefunc reveals the cause:
_ZN8cortex_m9exception15default_handler17h022e0dbc834bf513E is to small to bother merging
_ZN8cortex_m9exception15default_handler17h07a6735a3f66597aE is to small to bother merging
_ZN8cortex_m9exception15default_handler17h0912171e374cbd1fE is to small to bother merging
etc.
Well, turns out it's a FIXME in LLVM:
// Don't merge tiny functions, since it can just end up making the function// larger.// FIXME: Should still merge them if they are unnamed_addr and produce an// alias.if (NewFunction->size() == 1) {
if (NewFunction->front().size() <= 2) {
DEBUG(dbgs() << NewFunction->getName()
<< " is to small to bother merging\n");
returnfalse;
}
}
I've looked it and it seems very straightforward to fix. Take the logic from writeThunkOrAlias, check that !F->isInterposable() && !G->isInterposable(), then writeAlias(F, G);. I'll review and merge a patch if someone writes it (with a test).
Reacted by Eduard-Mihai Burtescu, Jorge Aparicio, Jan Pochyla and Daniel Egger
Another fix has been proposed in rust-embedded/cortex-m-rt#27 and rust-embedded/svd2rust#130. It uses global_asm! to define exception / interrupt handlers as weak aliases of the DEFAUT_HANDLER symbol. The fix saves 4*N + 32 bytes of Flash memory where N is the number of interrupt handlers the device has.
Agreed, but using a hack for this specific case will not cancel the benefits of a proper solution.
And since it's not a public interface, but a private implementation detail, we can always change it back if we need to.
I think weak aliases is the way to solve this. Semantically it is what we want to do: unless overriden a handler will be the DEFAULT_HANDLER. The need for global_asm! is a bit unfortunate but that's because of the lack of a proper aliasing mechanism in rustc (gcc and clang have such mechanism).
Relying on LLVM to merge exactly looking function works too but requires explicitly enabling the LLVM pass -- it's not enabled by default, right? -- and may not work (well) if not compiling with optimizations; I haven't actually tested that scenario. Whereas weak aliases work regardless of the optimization settings.
Of course, making mergefunc better is a welcome addition. It might help in other cases like in the panicking stuff that @pftbest mentioned.
requires explicitly enabling the LLVM pass -- it's not enabled by default, right?
I suspect that it can actually produce better results if enabled in rustc in general, given how many functions may well monomorphize to the same thing. For example, every single time you have phantom types.
@japaric I'm pretty sure LLVM just has a list of passes and you can add as many copies of anything as you'd like (some might break because the IR isn't what they expect but that's it).
The global_asm! fix has landed and it's available in cortex-m-rt v0.3.5 and svd2rust v0.11.2. Note that you'll need to nightly-2017-07-21 or newer if targetting the Cortex-M architecture (or you'll hit an LLVM assertion). If these changes cause problems in the future we can always revert them as they are not part of the public / user-facing API.
@pftbest Yes, I got some of them merged upstream, but the others seem to fail tests in weird ways. I have this on my TODO list and will return to it shortly.
@whitequark that is great, thank you! global_asm hack didn't work that well, and we still have an extra function in binary, so it's not saving us any space compared to your solution.
Well, the issue is fixed for now, the question is do we want to remove dependency on global_asm (and go back to naked functions, which is also not ideal, but already used for other things) when @whitequark 's patches will be merged.
I don't think we should reopen this issue as the problem is already 99% fixed and changing the underlying solution will at most reduce binary size by 4 or 8 bytes (I forget the exact number).
However, in general, I'm 👍 on reducing the number of unstable features this crate, and the whole cortex-m stack, uses so we can open an issue about that (reducing unstable features) and list the global_asm -> naked_function switch there.
Activity
japaric commentedon Mar 19, 2017
Not that I know of. I was hoping LLVM would merge the identical functions but it doesn't. I don't know if it's capable of doing that at all.
cc @eddyb Do you know if LLVM should be able to optimize something like this and we are not giving it enough information or maybe it's not possible to do at all:
Today, we get something like this (release + LTO):
Ideally, we'd like something like this:
eddyb commentedon Mar 19, 2017
@japaric You need
-C opt-level=3IIRC, and even then it might be disabled.You can try
-C passes=mergefuncto force it to run, but I'm not sure it will be placed right to do its thing.japaric commentedon Mar 19, 2017
Thanks for the suggestion, @eddyb. Unfortunately, it didn't work. We were already using
-C opt-level=3because of Cargo's--releaseflag and recompiling the world with-C passesdidn't help.-C passesactually made one test program 8 bytes larger (originally, 1270 bytes in size) 😆.whitequark commentedon Apr 30, 2017
Running a debug
optwith-debug -debug-only mergefuncreveals the cause:etc.
Well, turns out it's a FIXME in LLVM:
I've looked it and it seems very straightforward to fix. Take the logic from
writeThunkOrAlias, check that!F->isInterposable() && !G->isInterposable(), thenwriteAlias(F, G);. I'll review and merge a patch if someone writes it (with a test).whitequark commentedon Jun 29, 2017
https://reviews.llvm.org/D34805 + https://reviews.llvm.org/D34806
define handlers as weak aliases of DEFAULT_HANDLER
japaric commentedon Jul 18, 2017
Another fix has been proposed in rust-embedded/cortex-m-rt#27 and rust-embedded/svd2rust#130. It uses
global_asm!to define exception / interrupt handlers as weak aliases of the DEFAUT_HANDLER symbol. The fix saves 4*N + 32 bytes of Flash memory where N is the number of interrupt handlers the device has.pftbest commentedon Jul 18, 2017
Hello, @whitequark! Any updates on your patches? I think they can also save some space on panic handlers as well.
For each panic in a program there is a separate function that just calls the panic_fmt
For example AT2XT project by @cr1901, has 5 of this functions, and that is too much.
whitequark commentedon Jul 18, 2017
@pftbest I will take another look at them soon. Half of the patch is already LGTM'd, there's very little left.
@japaric Let's not merge this hacky workaround. I'll try to be more responsive this time.
pftbest commentedon Jul 18, 2017
But the workaround hack will give 4 bytes shorter result :)
whitequark commentedon Jul 18, 2017
@pftbest The proper solution may well benefit other functions.
pftbest commentedon Jul 18, 2017
Agreed, but using a hack for this specific case will not cancel the benefits of a proper solution.
And since it's not a public interface, but a private implementation detail, we can always change it back if we need to.
japaric commentedon Jul 18, 2017
I think weak aliases is the way to solve this. Semantically it is what we want to do: unless overriden a handler will be the DEFAULT_HANDLER. The need for
global_asm!is a bit unfortunate but that's because of the lack of a proper aliasing mechanism in rustc (gcc and clang have such mechanism).Relying on LLVM to merge exactly looking function works too but requires explicitly enabling the LLVM pass -- it's not enabled by default, right? -- and may not work (well) if not compiling with optimizations; I haven't actually tested that scenario. Whereas weak aliases work regardless of the optimization settings.
Of course, making mergefunc better is a welcome addition. It might help in other cases like in the panicking stuff that @pftbest mentioned.
whitequark commentedon Jul 18, 2017
Take a look at the code generated with svd2rust that's built without optimizations.
whitequark commentedon Jul 18, 2017
I suspect that it can actually produce better results if enabled in rustc in general, given how many functions may well monomorphize to the same thing. For example, every single time you have phantom types.
3 remaining items
japaric commentedon Jul 20, 2017
@therealprof I can't land these patches until rust-lang/rust#43313 makes it way into the nightly channel. Otherwise we'll hit LLVM assertions.
eddyb commentedon Jul 20, 2017
@japaric I'm pretty sure LLVM just has a list of passes and you can add as many copies of anything as you'd like (some might break because the IR isn't what they expect but that's it).
define handlers as weak aliases of DEFAULT_HANDLER
japaric commentedon Jul 22, 2017
The global_asm! fix has landed and it's available in cortex-m-rt v0.3.5 and svd2rust v0.11.2. Note that you'll need to nightly-2017-07-21 or newer if targetting the Cortex-M architecture (or you'll hit an LLVM assertion). If these changes cause problems in the future we can always revert them as they are not part of the public / user-facing API.
pftbest commentedon Aug 22, 2017
Hello, @whitequark . Any news on your patches?
whitequark commentedon Aug 22, 2017
@pftbest Yes, I got some of them merged upstream, but the others seem to fail tests in weird ways. I have this on my TODO list and will return to it shortly.
pftbest commentedon Aug 22, 2017
@whitequark that is great, thank you! global_asm hack didn't work that well, and we still have an extra function in binary, so it's not saving us any space compared to your solution.
whitequark commentedon Aug 22, 2017
@japaric in light of above: reopen this?
pftbest commentedon Aug 22, 2017
Well, the issue is fixed for now, the question is do we want to remove dependency on global_asm (and go back to naked functions, which is also not ideal, but already used for other things) when @whitequark 's patches will be merged.
japaric commentedon Aug 24, 2017
I don't think we should reopen this issue as the problem is already 99% fixed and changing the underlying solution will at most reduce binary size by 4 or 8 bytes (I forget the exact number).
However, in general, I'm 👍 on reducing the number of unstable features this crate, and the whole cortex-m stack, uses so we can open an issue about that (reducing unstable features) and list the global_asm -> naked_function switch there.
whitequark commentedon Oct 15, 2017
@pftbest All done.
Merge pull request #19 from japaric/panic
define handlers as weak aliases of DEFAULT_HANDLER
define handlers as weak aliases of DEFAULT_HANDLER