Closed qwandor closed 1 year ago
One of the major changes between bitflags
1 and 2 is that it introduces the BitFlags
trait to work with flags generically, rather than just implementing the same methods on each bitflags type. I wonder if that's causing some extra code or vtable to be included somehow, even if it's not really needed here?
Do you have a pressing need to merge this PR in?
If so, I don't mind landing it in dev/0.7
for now, but if not, I'd like to leave it open until I have a chance to dig into this binary size regression myself.
Thanks for the reply.
For context, I work on Android. We vendor in all the Rust crates that we use (you can see them at https://cs.android.com/android/platform/superproject/+/master:external/rust/crates/ if you're curious), and gdbstub
is among them. As much as possible, we try to avoid having multiple versions of a crate in the tree, both because of the maintenance overhead and because of the impact on overall binary size.
bitflags
has caused a problem, because we now have some crates updated upstream to version 2, while some are still using version 1. We can maintain downstream patches to work around this, but this is more maintenance burden so I'd like if possible to get every crate we care about using bitflags
2 upstream.
So from my point of view the sooner this change is in and released the better, but there are still a number of other crates in the same position or otherwise blocking the update so if we have to maintain local patches then we will.
Thanks for the context!
As someone who often takes point manicuring the dependency tree at $work, I completely understand. I've sent out my fair share of PRs and DMs asking projects to bump syn
to version 2 myself...
Unfortunately, I don't have a great estimate for when I'll be cutting 0.7
release, as there are quite a few small-to-medium sized issues that'll that need to get resolved as part of the 0.7 milestone, and I'm not sure when I'll find the time to sit down and hammer those out.
Let me know if you think it'd be helpful to land this PR in dev/0.7
(as opposed to just leaving it open for now).
Okay, I had a chance to dig in here, and I've found the culprit.
bitflags!
emits a FromStr
impl, which includes a call to .trim()
gdbstub
only contained a single call to .trim()
(as part of its target.xml
handling).trim()
, LLVM no longer inlines .trim()
, even though that new use of .trim()
is then dead-code-eliminated!I've tested this theory by reverting the bitflags changes, and adding a simple call to core::hint::black_box("foo").trim()
into the binary, and lo and behold - there's a massive binary size jump!
Not gonna lie, this is pretty funny, and I'm glad I could figure this out haha.
Anyways, in terms of "action items" - I think the best thing to do would be to swap out the current .trim()
call in gdbstub
to a hand-rolled one that will get inlined. Assuming that does the trick, I'll go ahead and merge that in as an unrelated commit into dev/0.7
, and then apply this PR on-top of it.
Thanks for digging into that! It sounds rather like a compiler bug, might be worth filing it somewhere?
It sounds rather like a compiler bug, might be worth filing it somewhere?
Maybe... but this might be niche enough that I'm not gonna do it myself. Activation energy and whatnot ¯\_(ツ)_/¯
In any case... I think we should leave this PR open until https://github.com/bitflags/bitflags/issues/348#issuecomment-1535532991 resolves, at which point, we can switch over to the more lightweight macro, and sidestep this weird compiler interaction entirely.
Okay, thanks.
I've updated to bitflags 2.3.1 and the external version of the macro.
Sweet, that's awesome.
I'll go ahead and merge this into dev/0.7
then.
Thanks again!
FYI, gdbstub 0.7 has just been released. Cheers!
Great, thanks!
Description
This updates the
bitflags
dependency to the latest version. As it is a major version change, some small code changes are required.API Stability
This is a breaking API change, because it changes the API of the public types
HostIoOpenFlags
andHostIoOpenMode
which are generated by thebitflags!
macro.Checklist
rustdoc
formatting looks good (viacargo doc
)examples/armv4t
withRUST_LOG=trace
+ any relevant GDB output under the "Validation" section below./example_no_std/check_size.sh
before/after changes under the "Validation" section belowexamples/armv4t
./example_no_std/check_size.sh
)Arch
implementationValidation
GDB output
``` (gdb) info mem Using memory regions provided by the target. Num Enb Low Addr High Addr Attrs 0 y 0x00000000 0x100000000 rw nocache ```armv4t output
``` $ RUST_LOG=trace cargo run --example armv4t --features=std Finished dev [unoptimized + debuginfo] target(s) in 0.02s Running `/usr/local/google/home/qwandor/rust/gdbstub/target/debug/examples/armv4t` loading section ".text" into memory from [0x55550000..0x55550078] Setting PC to 0x55550000 Waiting for a GDB connection on "127.0.0.1:9001"... Debugger connected from 127.0.0.1:52612 TRACE gdbstub::protocol::recv_packet > <-- + TRACE gdbstub::protocol::recv_packet > <-- $qSupported:multiprocess+;swbreak+;hwbreak+;qRelocInsn+;fork-events+;vfork-events+;exec-events+;vContSupported+;QThreadEvents+;no-resumed+;memory-tagging+;xmlRegisters=i386#77 TRACE gdbstub::protocol::response_writer > --> $PacketSize=1000;vContSupported+;multiprocess+;QStartNoAckMode+;ReverseContinue+;ReverseStep+;QDisableRandomization+;QEnvironmentHexEncoded+;QEnvironmentUnset+;QEnvironmentReset+;QStartupWithShell+;QSetWorkingDir+;swbreak+;hwbreak+;QCatchSyscalls+;qXfer:features:read+;qXfer:memory-map:read+;qXfer:exec-file:read+;qXfer:auxv:read+#fa TRACE gdbstub::protocol::recv_packet > <-- + TRACE gdbstub::protocol::recv_packet > <-- $vMustReplyEmpty#3a INFO gdbstub::stub::core_impl > Unknown command: Ok("vMustReplyEmpty") TRACE gdbstub::protocol::response_writer > --> $#00 TRACE gdbstub::protocol::recv_packet > <-- + TRACE gdbstub::protocol::recv_packet > <-- $QStartNoAckMode#b0 TRACE gdbstub::protocol::response_writer > --> $OK#9a TRACE gdbstub::protocol::recv_packet > <-- + TRACE gdbstub::protocol::recv_packet > <-- $Hgp0.0#ad TRACE gdbstub::protocol::response_writer > --> $OK#9a TRACE gdbstub::protocol::recv_packet > <-- $qXfer:features:read:target.xml:0,ffb#79 TRACE gdbstub::protocol::response_writer > --> $mBefore/After `./example_no_std/check_size.sh` output
### Before ``` target/release/gdbstub-nostd : section size addr .interp 28 792 .note.gnu.property 32 824 .note.gnu.build-id 36 856 .note.ABI-tag 32 892 .gnu.hash 36 928 .dynsym 360 968 .dynstr 204 1328 .gnu.version 30 1532 .gnu.version_r 64 1568 .rela.dyn 408 1632 .init 23 4096 .plt 16 4128 .plt.got 8 4144 .text 14955 4160 .fini 9 19116 .rodata 922 20480 .eh_frame_hdr 260 21404 .eh_frame 1340 21664 .init_array 8 28072 .fini_array 8 28080 .dynamic 448 28088 .got 136 28536 .data 8 28672 .bss 8 28680 .comment 31 0 Total 19410 ``` ### After ``` target/release/gdbstub-nostd : section size addr .interp 28 792 .note.gnu.property 32 824 .note.gnu.build-id 36 856 .note.ABI-tag 32 892 .gnu.hash 36 928 .dynsym 360 968 .dynstr 204 1328 .gnu.version 30 1532 .gnu.version_r 64 1568 .rela.dyn 408 1632 .init 23 4096 .plt 16 4128 .plt.got 8 4144 .text 15355 4160 .fini 9 19516 .rodata 1178 20480 .eh_frame_hdr 268 21660 .eh_frame 1396 21928 .init_array 8 28072 .fini_array 8 28080 .dynamic 448 28088 .got 136 28536 .data 8 28672 .bss 8 28680 .comment 31 0 Total 20130 ```