Rework process attaching - #199
Conversation
|
...In the original description I mistakenly said that the armv4t examples don't have As a meta-note, have you considered something like pre-commit and/or pre-push hooks? I've gotten used to them (perhaps too used to them, as seen above), it's an easy way to make sure that you don't forget a certain test/check, and there's the easy escape hatch of Anyways, I figure this might be a good time/place to address the issue of the Did you want to go through with the new |
This removes the need for the `CurrentActivePid` trait. As discussed in the issue tracker for daniel5151#124 (multiprocess support), we're willing to trade a needless `usize` in targets that don't need it for simpler multiprocess support.
d37eff1 to
4259a2b
Compare
|
FYI, I prob won't be able to take a look here until ~Wednesday (got a bit of a hectic week). Apologies! EDIT: Looking like this weekend at this point. I'm traveling cross-country at the moment, but should hopefully have some free time ~Sat / Sun. |
There was a problem hiding this comment.
I know that previous threads were discussing the notion of current_tid, but now that I've had a chance to page back in context about this code... it's not going to be as simple as adding a new field. fortunately, I think the recent T::Tid + IsValidTid rework stuff has laid a solid foundation for generically handling pid tracking across the 3 classes of targets we have today.
notably, with the current design, it seems pretty easy to "desync" the pid being tracked in this independent new type vs. in the other current_*_tid types (and ofc, having the same info in two places is unfortunate to begin with)
lmk if this feedback makes sense!
| _connection: PhantomData<C>, | ||
|
|
||
| // The most recently attached PID | ||
| current_active_pid: Pid, |
There was a problem hiding this comment.
so, IIRC, there's not real notion of "active" thread-id in the GDB RSP. The only firm concepts are the currently "selected" thread-id for memory operations, and resume operations.
as such, adding a new type here doesn't seem right. rather, I think the IsValidTid trait will need to grow some new methods for generically setting / getting the pid component of T::Tid.
There was a problem hiding this comment.
I found a few places in the protocol that mention the notion of a "current thread":
- The
qCRSP message is described as this (annoyingly, this is entire thing, and there is no elaboration on what "current" means anywhere else...I know I already complained about this, but it doesn't make it any less annoying)
Return the current thread ID.
- The documentation for
qXfer:exec-file:readincludes this:
If the annex part is empty the remote stub should return the filename corresponding to the currently executing process.
- If a client doesn't support multiprocess extensions, it sends us thread IDs without a process ID, so there needs to be some notion of what process it's referring to; the process would be the "current" process (ie the attached process), which would combine to make the "current" thread.
Unless I'm misreading these, it seems like there's a need to represent the current process/thread somewhere; would you rather close this PR and keep on representing it in the target implementations like they were before this PR?
I do worry that handling it in the targets could be potentially confusing for multiprocess target implementations, who also have to keep track of what the "current" PID is, along with all their "attached" PIDs; like what does it really mean to have a "current" thread when you're keeping track of 3 separate processes that are all running at the same time?
Extending the IsValidTid trait to return the pid component seems like it would have to interact with the stub somehow to get this current pid for multithreaded stubs, since the client can still attach to an arbitrary process; we can't just return FAKE_PID...it would probably be more useful as a method of the stub, where the IsValidTid trait would return None if there was no pid component, and the stub could provide a current pid.
There was a problem hiding this comment.
Yeah, the GDB RSP docs definitely have some annoying ambiguities... sigh.
Inferring the correct behavior typically requires playing around with the code, and maybe cross-referencing with the upstream implementation in the GDB source code (i.e: in remote.c).
Unless I'm misreading these, it seems like there's a need to represent the current process/thread somewhere;
Well yes, of course. I'm not saying we shouldn't track it.
What I'm saying is that the correct place to track this info is in current_{mem,resume}_tid, which can only be done by extending T::Tid to properly track pids.
Or, in other words, to refine my initial comment - the terms "active" thread-id and "selected" thread-id are one and the same, and should be tracked via the same bit of storage.
Again, this all goes back to the fact that you really shouldn't be thinking about pids and tids as separate concepts for the vast majority of the GDB RSP.
Outside of a few narrow packets (e.g: attach, run), the only thing that ever really matters is a specific tuple of (pid, tid) (AKA, what the GDB RSP calls a thread-id). That's the thing that should be tracked via current_{mem,resume}_tid, that's the thing that will be passed to Target IDETs via the tid: T::Tid param, etc...
When you think about it that way, it should be very clear why a separate current_active_pid: Pid type doesn't really make sense. You're splitting out an indivisible aspect of the thread-id data type into its own field.
I do worry that handling it in the targets could be potentially confusing for multiprocess target implementations, who also have to keep track of what the "current" PID is, along with all their "attached" PIDs; like what does it really mean to have a "current" thread when you're keeping track of 3 separate processes that are all running at the same time?
The notion of the selected/active thread-id is something that should never leak out of the gdbstub implementation itself. And indeed, if you look, you'll find that qC doesn't even correspond to an IDET.
Indeed, not leaking "current active thread" semantics is is one of gdbstubs major ergonomic wins over trying to roll your own stub code!
From the target's perspective, there is no such thing as a 'Current PID'. The target is essentially just a set of stateless functions that say 'Read memory from PID X, Thread Y'. The only entity that needs to know which PID is 'current' is gdbstub, so it knows which numbers to pass into those functions.
It will only ever be asked to operate on specific thread-ids (i.e: (pid, tid) tuples), or all tids in a single pid (via some of the resume APIs). Sure, there are some APIs that will ask the target to do something related to a specific pid (i.e: attach), but it's only when the Target affirmatively responds to those packets (via a stop reason) that gdbstub checks to see what (pid, tid) is being reported as part of the stop response, and then updates its internal active-pid tracking.
Or, another example (not related to attach): when the GDB client wants to switch contexts, it sends an Hg (set general thread) or Hc (set continue thread) packet. gdbstub intercepts these and exclusively uses them to update its internal current_{mem,resume}_tid state. The target implementation never sees these H packets directly, and it never needs to maintain a concept of a 'selected' thread.
Instead, when a subsequent packet arrives that relies on this context (like a memory read m or a register read g), gdbstub automatically grabs that stored state and passes the fully-resolved (pid, tid) directly into the relevant Target trait method. Keeping that boundary strict is exactly why we want to avoid leaking a current_active_pid or similar state into the target traits.
Extending the
IsValidTidtrait to return the pid component seems like it would have to interact with the stub somehow to get this current pid for multithreaded stubs, since the client can still attach to an arbitrary process
Hopefully you can now see why this isn't really accurate. IsValidTid doesn't need to 'interact' with the stub to get the PID because the stub is the one that constructs the T::Tid and gives it to the trait in the first place. The trait simply acts as the storage container for that tuple.
i.e: when the client attaches to an arbitrary process, gdbstub will pass that request through to the state machine, and when the request is acknowledged by the Target with a corresponding stop-reason on attach, gdbstub will update its internal tracking to reflect the newly selected thread-id.
Does this all make a bit more sense now? Do you see why IsValidTid will need to be tweaked to also track pid handling?
wrt. handling the FAKE_PID codepaths - look at how I reworked gdbstub's code in #198 to report an error in cases where a single-threaded target (i.e: where T::Tid = ()) somehow runs into a situation where the GDB client is requesting something that isn't SINGLE_THREAD_TID. That same style of handling should be easy to do wrt. FAKE_PID when T::Tid is () or Tid in the single/multi-thread use cases, while transparently converting to/from (tid, pid) in the new multi-process use-cases.
There was a problem hiding this comment.
There was a lot in there; I was already familiar with how tids (and one day, pids) get to the target method, but just to make sure I understood the actionable parts of your response, it's basically:
- We don't need the
current_active_pidfield of theGdbStubImpl(how this this PR does it) or theCurrentActivePidtrait (what v0.8 currently does), because it should be tracked incurrent_{mem,resume}_tid - We need to expand the
IsValidTidto get thepid, and it would look something like this:
impl IsValidTid for () {
def get_pid(&self) -> Pid {
crate::FAKE_PID
}
...
}
impl IsValidTid for Tid {
def get_pid(&self) -> Pid {
crate::FAKE_PID
}
...
}
// Multiprocess name/representation in my local branch
impl IsValidTid for ExtendedThreadId {
def get_pid(&self) -> Pid {
self.pid
}
...
}If I'm mischaracterizing this, please ignore everything after this line and correct me!
I enthusiastically agree that this covers almost everything in the protocol, but I don't see how this handles a response to a qC packet. The client can still attach to any pid in the multithreaded (or even single threaded) case, but if the only candidates we can store that attached pid are in current_mem_tid/current_resume_tid, there's literally nowhere the value of the pid can go. When we use the above implementation for the return value of qC, we return FAKE_PID, and the gdb client crashes after a vAttach followed by a qC, with an assertion failure because the process portion of qC doesn't match the process it originally attached to (at least that's what versions 12.1, and IIRC 17.1, of the gdb client do).
Maybe there's a different path that fixes all of these? Currently the stub unconditionally reports it supports multiprocess features:
https://github.com/daniel5151/gdbstub/blob/dev/0.8/src/stub/core_impl/base.rs#L115-L116
This forces us to return extended thread IDs with a process and a thread in stop replies and qC. What if we only reply that we support multiprocess features in the (soon-to-exist) multiprocess mode? It helps us be more honest with the client about our actual capabilities, and then according to the protocol it's not supposed to even send us pids, or expect to receive pids from us. Then we don't have to worry about conjuring up some kind of pid (FAKE_PID...) to respond with for thread ID types that don't have one (like () or Tid).
There was a problem hiding this comment.
Almost!
You're right on 1, but for 2, what you actually want to do is change the IsValidTid trait itself to operate on this new ExtendedThreadId type (which, should prob just be a (Pid, Tid) tuple, rather than a custom type).
// update trait
pub trait IsValidTid: private::Sealed + PartialEq + Copy {
#[doc(hidden)]
fn into_fully_qualified_tid(self) -> (Pid, Tid);
#[doc(hidden)]
fn from_fully_qualified_tid(pid: Pid, tid: Tid) -> Option<Self>;
#[doc(hidden)]
fn sentinel() -> Self;
}
// update existing impl
impl IsValidTid for Tid {
fn into_fully_qualified_tid(self) -> (Pid, Tid) {
(self, crate::FAKE_PID)
}
fn from_fully_qualified_tid(pid: Pid, tid: Tid) -> Option<Self> {
// notice how the Tid impl now enforces the FAKE_PID?
if pid == crate::FAKE_PID { Some(tid) } else { None }
}
fn sentinel() -> Self {
crate::SINGLE_THREAD_TID
}
}
// create new impl
impl IsValidTid for (Pid, Tid) {
fn into_fully_qualified_tid(self) -> (Pid, Tid) {
self
}
fn from_fully_qualified_tid(pid: Pid, tid: Tid) -> Option<Self> {
Some((pid, tid))
}
fn sentinel() -> Self {
(crate::FAKE_PID, crate::SINGLE_THREAD_TID)
}
}P.S: might be time to rename SINGLE_THREAD_TID to FAKE_TID for consistency, hah. Similarly, IsValidTid can prob be renamed to ValidTid, since it's now far more than just a marker trait. neither of these needs to happen now - just jotting down the thoughts so I don't forget.
I'm not totally following, so apologies if this response is us talking past eachother 😅
You are correct that there is a narrow window of time where gdbstub needs to use a "dummy" thread-id value internally, as it hasn't yet interacted with the Target enough to ascertain the true thread-id it should be reporting. The inline comments in GdbStubImpl::new talk about it briefly.
That said, IIRC, the very first message any GDB client sends over is ?, which will trigger the attach path I talked about above, and would therefore give gdbstub a chance to update its internal current_{mem,resume}_tid value from the stop reply issued by the user's integration.
Similarly, if the GDB client were to start off with a vAttach to a specific PID, same logic applies - it would trigger the same attach path, we would sniff the current thread-if from the stop reply packet, and we'd be all set.
Essentially, my claim is that qC is never sent prior to us having a chance to ascertain a specific thread-id to report from it.
If this turns out to be false in practice / in some narrow edge case... we can always have gdbstub artificially start its state machine into the attach state (instead of starting in the Idle state) in order to force users to declare up-front what thread-id is currently attached (and in that artificial attach state, simply swallow the stop reason they report, as the GDB client wouldn't be expecting a stop reason packet at that point in the GDB RSP sequence). But this isn't something I think we need... since my claim is that all GDB clients are "reasonable" insofar as starting off each conversation with a ? / vAttach / vRun, packet in order to understand what state the stub is in.
your idea of only using multiprocess mode in, well, multi-process mode is intriguing, but comes with a major caveat: it means that single/multi-threaded targets would be excluded from extended mode facilities. See this comment in the docs https://docs.rs/gdbstub/latest/gdbstub/target/ext/extended_mode/trait.ExtendedMode.html#extended-mode-for-singlemulti-threaded-targets
But in any case, hopefully the explanation I offer above makes it a bit clearer why - in practice - things should work fine.
There was a problem hiding this comment.
I've been operating under the assumption extended mode support and multiprocess support are orthogonal(-ish?), where stubs can support extended mode without multiprocess. The client can attach to any process it wants, it just can't attach to more than one of them at the same time. The documentation seems careful about specifying when packets are only available in extended mode versus when packets are only available in multiprocess mode, and I haven't seen anything suggesting that extended mode only applies for multiprocess stubs. The gdb client source code also doesn't seem to check for multiprocess support when it sends an extended mode packet (extended_remote_target::open in gdb/remote.c).
If you find that this isn't the case, please let me know!
Anyways, I agree that qC will always have a chance to snoop a Tid; the issue is that we lose the pid for Tid types that don't have a pid field. Please hear me out; I know there's already a lot written about how we don't need to care about the pid for 99% of the packets, and I agree: I'm saying that the response for qC is the 1% that does. Here's the exact scenario I'm worried about:
For the gdb clients I've tested, when the gdb client attaches to a process P, it sends vAttach P, Hgp0.0, qC, and then Hg<thread returned by qC>. The biggest note here is that the gdb client will crash with an assertion failure if the pid returned by qC doesn't match the pid passed by vAttach (aka P).
And here's how it would play out based on how I understand what you're proposing:
- gdbstub receives the
vAttach Pmessage, and passesPto the target'sattachmethod. - gdbstub snoops the stop reply that goes out to the client [0], and sets its internal
current_{mem,resume}_tidfields. The stop reply takes aTid(or()), the stop reply's pid value isFAKE_PIDafter fully qualifying it, and we've totally lost track ofPafter this point. - gdbstub receives the
Hgp0.0message, nowcurrent_mem_tidis an arbitrary thread. Not super relevant here. - gdbstub receives the
qCmessage. If it does not reply with a pid that isP, the gdb client will crash. When we fully qualifycurrent_{mem,resume}_tidin our response, we send backFAKE_PID, which is probably not equal toP. There's nothing else we can do; gdbstub lost track ofPin step 2! The gdb client crashes.
As always, please let me know if I'm misunderstanding what you're proposing and how it fits in here!
This is also why I'm saying not advertising multiprocess support might be the fix here: we don't have to include a process field in the response to qC anymore. We can reply with just a Tid value, and gdb doesn't lose its mind when we don't keep track of information we shouldn't have to care about.
I'm not sure what you mean about how this would break single/multi-threaded extended mode; the linked documentation seems to agree that we can have extended mode without needing to be multiprocess.
[0] I assume this is a future enhancement; gdbstub doesn't currently snoop the stop reply: vAttach and ? use report_reasonable_stop_reason, which doesn't call write_stop_common, which is where the current_{mem,resume}_tid get set.
There was a problem hiding this comment.
Yeah... that's what I get for drafting a response before the morning coffee has fully kicked in, hah.
Yes, multi process extensions are orthogonal to extended mode, whoops.
[0] I assume this is a future enhancement;
gdbstubdoesn't currently snoop the stop reply:vAttachand?usereport_reasonable_stop_reason, which doesn't callwrite_stop_common, which is where thecurrent_{mem,resume}_tidget set.
yes, all my comments are refer to a world post #194, where report_reasonable_stop_reason is in the rear-view mirror.
- gdbstub receives the
vAttach Pmessage, and passesPto the target'sattachmethod.- gdbstub snoops the stop reply that goes out to the client [0], and sets its internal
current_{mem,resume}_tidfields. The stop reply takes aTid(or()), the stop reply's pid value isFAKE_PIDafter fully qualifying it, and we've totally lost track ofPafter this point.
Lets zoom in on these steps.
In a nutshell, here is my thesis wrt multi-process in 0.8: once proper multi-process support lands, any time the GDB client attempts to attach / interact with a PID that isn't FAKE_PID (i.e: 1), gdbstub raises a runtime error (likely including some error text that nudges users towards implementing proper support for multi-process). This is similar to the current behavior in single-threaded mode, in cases where a non-1 tid gets sent/recv'd by gdbstub.
And in this world, I see two ways this scenario you're describing could play out:
- with a descriptive runtime failure (as discussed above)
- if we simply respond to the
vAttach Pmessage with a stop reason that usesFAKE_PID... maybe the GDB client itself simply disregards thePit sent, and only cares that the effect of the vAttach (i.e: the stop reply packet) was that we attached to a process withpid = FAKE_PID?
If the behavior is 2, I feel like we're totally in the clear with my thesis, since it sidesteps the crash scenario entirely. If the behavior is 1, that's less fun for the user... but it doesn't seem unreasonable that if they want to multi-process drift, they should support multi-process extensions?
One thing to note about the multiprocess+ feature:
Note that reporting this feature indicates support for the syntactic extensions only, not that the stub necessarily supports debugging of more than one process at a time.
I bring this up because I'd be curious to understand how a stub would communicate to GDB that it only supports connecting to a single process at a time?
This ties into the question of enabling/disable this feature, as well as my thesis above... as this implies there is a class of targets that support multi-threaded debugging, and can switch between debugging a set of processes... but only one at a time. This is essentially the only kind of "multi-process" target that gdbstub currently supports (via the various hax that have landed in 0.7).
On one hand, it seems a bit sad to lose support for modeling these sorts of targets in multi-thread mode... but on the other hand, maybe it's fine if there's a "complexity jump" in gdbstub's API if you need to support jumping between processes as a multi-thread target?
But again, the crux of my question is how does such a target tell GDB / how does GDB infer that a target that supports multiprocess+ is a target that supports debugging multiple processes simultaneously, vs. one at a time
Here is one theory (which, full disclosure - the robot helped me draft):
GDB's architecture for multi-process debugging is entirely "try it and find out" (optimistic execution). It never infers the stub's capacity upfront because the protocol has no mechanism to communicate "I am a simultaneous target" vs "I am a one-at-a-time target".
Here is how the distinction actually plays out using that remote.c logic:
- The "Simultaneous" Target:
- You attach to
P1. GDB sendsvAttach;P1. The stub replies with a stop packet. Success. - You type
add-inferiorandattach P2. GDB sendsvAttach;P2. - The stub supports simultaneous debugging, so it attaches to
P2and sends another stop packet. GDB is now debugging both.
- The "One-at-a-Time" Target:
- You attach to
P1. GDB sendsvAttach;P1. The stub replies with a stop packet. Success. - You type
add-inferiorandattach P2. GDB sendsvAttach;P2. - The stub only supports one at a time. Because it is already occupied by
P1, it rejects the packet by returningE01. - GDB parses the
Enn, hits thedefault:case, triggers theerror()macro, and aborts the attach. It tells the user "Attaching to P2 failed." - However, if the user had sent a
D(detach) orvKillforP1before trying to attach toP2, the stub would be "empty" again, and would accept thevAttach;P2request.
So, maybe gdbstub can include some extra logic in single/multi-thread mode to enforce disconnect prior to re-attach?
There was a problem hiding this comment.
Aha! Okay, I think that's the root cause of all this discussion! I was trying to keep the ability to attach to non-FAKE_PID processes in non-multiprocess targets, and it sounds like you're okay with dropping it.
Nice catch on the fact that multiprocess is just syntactic and means that the client/stub can send process IDs back and forth. Maybe this is still a compelling reason to not advertise multiprocess+ though? If the intention is to ignore any thread ID with pid != FAKE_PID, then telling the client "don't bother sending us a PID, we're only using the TID anyways" helps keep the client from sending us messages formatted in a way we don't care about...which ironically would actually help preserve the behavior of being able to attach to arbitrary processes.
There was a problem hiding this comment.
I was trying to keep the ability to attach to non-
FAKE_PIDprocesses in non-multiprocess targets, and it sounds like you're okay with dropping it.
Yes. Well... mostly yes.
All the current code related to overriding FAKE_PID is only really there as a transient hack until proper multi-process support can land. I suspect that things "work" today in a pretty jank sense, and it was never totally clear to me what the existing semantics are / what they should be. It worked "well enough" in #129, and that's about it, hah.
So, while thinking about this problem (multi process, attach semantics, etc...), I strongly suggest imagining a codebase before #129.
Now, with that mindset, the question at hand - how to handle attaching?
Honestly, the more I think about it... the more I think that we should just straight up disallow vAttach when running in single/multi-threaded mode, eh? I think it just complicates the problem space way too much.
If someone wants to multi-process drift, they should go ahead and implement the multi-process handlers - simple as that. And whether or not they support attaching to multiple processes simultaneously or not is up to them (and the optionality of supporting simultaneous process debugging can be pointed out in the docs).
That said, I do want to preserve support for vRun when running in single/multi-threaded mode, as its super useful for swapping out the currently running code in, say, emulation contexts. I think that's more tractable, since I think the GDB client understands that the current process has been "swapped out" when you respond with a stop reason that has the same FAKE_PID?
To achieve these new attach / run semantics, we'll need to execute on that ExtendedMode trait rework that I hinted at a while back (recall that it was designed a long time ago, and isn't really aligned with modern gdbstub API design wrt. how it bundles all those ops together), since the current organization really isn't cutting it.
And honestly... now that I think about it... why does "extended mode" even need to be a thing that consumers are aware of?
Concepts of attach, run, etc... can all just be modeled as their own IDETs that hang directly off of {Single,Multi}ThreadBase/MultiProcessBase, and gdbstub can simply infer how to respond to the ! packet based on whether the user has implemented any of those IDETs.
Yeah... it really feels like the ExtendedMode trait should just go away, and its API surface re-allocated across the *Base traits appropriately. No reason to leak this RSP-ism to end users, right?
Maybe this is still a compelling reason to not advertise
multiprocess+though?
I'm open to the idea!
Forcing the feature on was a choice I made a loooooong time ago, and if I had to guess why, it was likely related to future proofing / improving client compatibility... but honestly, I don't totally know.
I do know that at some point, multiprocess+ feature negation was added (bfe83e1) to work around WinDbg being a really dumb GDB RSP client (at the time), but I wager most targets still want multiprocess+ extensions.
Indeed, I'd be interested to see how the changes we're making affect LLDB. The stance on LLDB compat is fuzzy (see #99), but the tl;dr is that I certainly don't want to break LLDB (especially since we just landed some juicy LLDB-only WASM extensions, hah).
If the intention is to ignore any thread ID with
pid != FAKE_PID, then telling the client "don't bother sending us a PID, we're only using the TID anyways" helps keep the client from sending us messages formatted in a way we don't care about...which ironically would actually help preserve the behavior of being able to attach to arbitrary processes.
This doesn't really apply if we just straight up say "don't even expose vAttach in multi-threaded mode", but assuming you think that's a bad idea on my part, I'd be interested to see what the GDB client does if you force multiprocess+ off and then try doing some multi-process shenanigans with vAttach.
| .features | ||
| .multiprocess() | ||
| .then_some(SpecificIdKind::WithId(self.get_current_pid(target)?)), | ||
| .then_some(SpecificIdKind::WithId(self.current_active_pid)), |
There was a problem hiding this comment.
this seems wrong. this should be using the pid specified by the user via the tid parameter?
this ties into my other comment wrt. how current_active_pid shouldn't really be a "thing".
There was a problem hiding this comment.
Hah nice catch! That looks like a remnant that's been there for awhile, but nothing exposed the bug because only one process at a time has been able to be attached to (and before the changes from last week, it was only ever passed a Tid anyways), so the current_active_pid was never different from the PID that the gdb client sent.
I recently refactored CI into a runnable Feel free to add it as a pre-commit / pre-push hook :)
As you can tell from my response in #199 (comment) - this is def something to be thinking about in relation to this work.
Yeah, I'm thinkin' about it... Presumably, the flow would look something like this:
One question I have is whether this new *ish? can you send a ctrl-c interrupt while waiting for a stop reply packet in response to a |
|
Hey @jonathanzetier, any updates? No rush or anything, just checking in. I just got back from a ~2 weeks of vacation myself, so it's not like I've been putting much time into my various projects in the past couple weeks, hah. |
|
No major updates, it's the last hurrah of my parental leave (baby starts daycare next week), so I've been more "barely time schedule" than "part time schedule" :-) One smaller update is that before I proposed dropping the When I start working again, my plan is to basically push what I have for multiprocess support as-is (after rebasing it onto the other v0.8 changes you pushed), so we have something less hypothetical to talk about. I know you had mentioned somewhere else that you preferred a impl IsValidTid for Tid {
fn into_fully_qualified_tid(self) -> (Pid, Tid) {
(self, crate::FAKE_PID)
}
....
}Not to mention that as a target implementor, I'd much rather work with Anyways, that's my pitch for the new type over a tuple, but I can also use a tuple if you still prefer it. |
|
Haha, no worries - the fact you've got time at all to poke around with some OSS right now is quite the feat in and of itself lol. And your point wrt. type alias type confusion is very valid, and if I'm being honest - I was already thinking about turning That said, whether it's a tuple or a struct isn't something we need to settle on now - I can easily tweak it before cutting 0.8, so feel free to take whatever approach you'd like. |
2 participants
This removes the need for the
CurrentActivePidtrait. As discussed in the issue tracker for #124 (multiprocess support), we're willing to trade a needlessusizein targets that don't need it for simpler multiprocess support.Description
As discussed #124 (comment) and #124 (comment), we're doing away with the
CurrentActivePidtrait.The total size reported by the bloat checker surprisingly reported a total size decrease of 7 bytes, but less surprisingly the
.textsection increased about 150 bytes, and the relro_padding section decreased slightly more than 150 bytes (likely for 8-byte padding reasons).API Stability
This is going into v0.8, which is already breaking the API.
Checklist
rustdocformatting looks good (viacargo doc)examples/armv4twithRUST_LOG=trace+ any relevant GDB output under the "Validation" section below./example_no_std/check_size.shbefore/after changes under the "Validation" section belowexamples/armv4t./example_no_std/check_size.sh)ArchimplementationValidation
GDB output
TRACE log
Before/After `./example_no_std/check_size.sh` output
Before
After