|
From: Julian S. <js...@ac...> - 2009-08-15 23:51:52
|
On Friday 14 August 2009, Josef Weidendorfer wrote: > On Friday 14 August 2009, Nicholas Nethercote wrote: > > On Fri, Aug 14, 2009 at 8:02 AM, Josef > > > > Weidendorfer<Jos...@gm...> wrote: > > > On Wednesday 12 August 2009, Nicholas Nethercote wrote: > > >> Can you post the patch here? The change sounds ok to me in principle, > > >> but I think you'll have to create a new file > > >> include/pub_tool_transtab.h so I'd like to see the patch. Thanks. > > > > > > See attachment. > > > As there is a dependance on m_scheduler, I thought it would be better > > > to make it part of the scheduler. The scheduler has already a > > > dependency on transtab... > > > > > > Julian wanted to rethink whether there could be any further problems > > > for discarding translations from tools. However, there is already a > > > client request for discarding translations. So I think this should be > > > safe. > > > > Three comments: > > > > - I'd still rather see it in pub_tool_transtab.h. I don't see a good > > reason for breaking our convention that m_foo declarations live in > > pub_core_foo.h/pub_tool_foo.h. > > > > - Rather than creating the new VG_(discard_all_translations), I'd > > prefer to just make VG_(discard_translations) visible to tools. It's > > more flexible, and I'm sure at some point some tool will want to only > > discard translations from part of the address space. > > > > - This should go in after 3.5.0, just to minimise changes at this very > > late stage. I figure this isn't a problem as it's just an internal > > clean-up and not visible to the user. > > I am fine with all of your comments. However, for making > VG_(discard_translations) safe for tools, the assertion to check that we > are not inside of translated code is really useful (we do not want to > discard code we currently are executing). This will produce a dependency of > transtab to scheduler :( So I am inclined to agree, it needs to go in m_scheduler, since I really want to keep the assertion but don't want yet another circular module dependency. Josef, maybe file a bug report and put the patch there, so we don't lose it? J |