On 1/25/21 3:26 PM, Alex Bennée wrote: > > Claudio Fontana <cfont...@suse.de> writes: > >> On 1/25/21 1:43 PM, Claudio Fontana wrote: >>> On 1/25/21 1:12 PM, Alex Bennée wrote: >>>> >>>> Paolo Bonzini <pbonz...@redhat.com> writes: >>>> >>>>> In general I agree, but != means that rr disabled returns true. In general >>>>> it seems to me that rr disabled should work more or less the same as >>>>> record >>>>> mode, because there is no replay log to provide the checkpoints. >>>> >>>> Is this not an argument to combine the mode and check into replay.h >>>> inline helpers with some clear semantic documentation and the call sites >>>> become self documenting? >>>> >>>> if (deadline == 0 && replay_recording_or_checkpoint()) >>>> >>>> which also makes things easier to compile away if replay isn't there? >>> >>> >>> Seems that the TCG build faces a similar issue to the issue I was facing >>> with the non-TCG build, >>> before the non-TCG build got functional again (for x86). >>> >>> We solved the non-TCG build problem, by not compiling replay at all for >>> non-TCG, plus closing our nose and stubbing away what couldn't be >>> completely removed (yet). >>> >>> But the CONFIG_TCG build has the same legitimate requirement towards a >>> non-CONFIG_REPLAY build. >>> >>> ie, like we have tcg_enabled(), should we have replay_enabled()? Maybe it >>> could be reworked starting from replay_events_enabled()? >>> >>> And then when things are refactored properly for replay_enabled(), a >>> non-REPLAY TCG build can basically ignore all the inner workings of replay. >>> >> >> I guess to summarize the above, should there be a CONFIG_REPLAY, dependent >> on CONFIG_TCG, by default on, >> but which could be disabled with >> >> --disable-replay >> >> ? > > I'm not sure - fundamentally having replay is one of those cool things > you can do when you have TCG and I suspect there is less pressure to > have a finely tuned TCG enabled build compared to our HW accelerated > brethren using hypervisors. TCG plugins are a configure option purely > because there is a small but non-marginal cost in having it enabled. I > doubt replay figures that much if you are not using it. > > That said if it makes it easier to make sure our abstractions are clean > and the layering is good then maybe it is worth having a build that
I think that cleaning up these aspects would be beneficial in replay itself, but maybe this could be done without forcing the --disable-replay option. > allows us to check that. But if it's going to be super fiddly and > involve large amounts of code motion I doubt the "win" is big enough for > the effort. > > Also I suspect the config option would be CONFIG_ICOUNT because replay > is just one of the features on the spectrum of: > > deterministic icount -> record/replay -> reverse debugging > > which all require the base support for icount. > Right, icount_enabled() is already fundamentally able to check whether replay functionality is available or not. Probably there is already some cleanup possible by applying this consistently. Other clean up opportunities I see are in making consistent checks for whether the functionality is active, and consolidate all the spread state, including (but not limited to): replay_mode, events_enabled, replay_is_debugging, in_checkpoint, ... (into a single struct replay ?) ..and then I guess clean up the namespace from all the replay_ functions for which we need a gazillion stubs for non-TCG code.. Ciao, Claudio
