592187 - Restore back the log function in VMPI_spyCallback

Status: RESOLVED FIXED

(Tamarin Graveyard :: Garbage Collection (mmGC), defect, P4)

Description

[Ruchi Lohani]

The VMPI_spyCallback in SpyUtilPosix.cpp does this:
RedirectLogOutput(SpyLog);
MMgc::GCHeap::GetGCHeap()->DumpMemoryInfo();
RedirectLogOutput(NULL);

Given that we have a RedirectLogOutput() function which could have been used to set a redirect log function previously, we should be restoring back the previous log function rather than setting it to NULL. The same issue is also present in other Spy utils.

Comment 1

[Ruchi Lohani]

Attachment: Restore back the old log function

Comment 2

[Ruchi Lohani]

I also had a thought that we can probably avoid having a new Get function for the loggging. RedirectLogOutput() can be modified to return the old log function which can then be used to reset it at the end. Though it might require the RedirectLogOutput to be renamed to something else.

Comment 3

[Lars T Hansen]

Comment on attachment 488931 [details] [diff] [review]
Restore back the old log function

Tabstops in the file should be cleaned up.

Comment 4

[Ruchi Lohani]

Attachment: Updated patch.

Comment 5

[Tommy Reilly]

Comment on attachment 489338 [details] [diff] [review]
Updated patch.

Don't like the code duplication, please move the typedef, getter and logFunc to a common file.   In general code in a platform specific file should be platform specific although and not a copy from other platform specific files (I realize other certain large code bases don't follow this rule but it would be nice to adhere to it in the vm).

Comment 6

[Ruchi Lohani]

RedirectLogOutput() also falls in the same duplication category then - common code in all platform specific files. Is there an existing common file where these could be moved?

Comment 7

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #6)
> RedirectLogOutput() also falls in the same duplication category then - common
> code in all platform specific files. Is there an existing common file where
> these could be moved?

I imagine if a file for it exists, it should be in vmbase/.  But I do not see a good .cpp in there for this (that directory is a relatively new addition to the infrastructure).

Comment 8

[Ruchi Lohani]

Attachment: Patch v2

Take 2, with logging functions common to all platforms moved to a separate file.

Comment 9

[Tommy Reilly]

Comment on attachment 501524 [details] [diff] [review]
Patch v2

Looks good, my only concern is that GetCurrentLogFunction, LoggingFunction and RedirectLogOutput should be scoped somehow, ie put them in the vmbase namespace.

Comment 10

[Ruchi Lohani]

Attachment: Patch v3

Updated the scope of the log functions. I just used "using namespace vmbase;", let me know if I should instead just use the vmbase::RedirectlogOutput() etc. Also, the xcode and visual studio project files will need to be changed to add the 2 new files. Should those also be attached to the bug?

Comment 11

[Lars T Hansen]

vmbase is not a dumping ground for code that's common to platform specific files.  So far it has been a place to put code that must be shared between MMgc and AVMPlus because those are compiled separately by the Flash Player.  As Tommy observes, we have no place (yet) to put code that's common in the various platform files.  I'm not particularly allergic to code that's duplicated in the various VMPI implementation files, especially trivialities like these.

SR+, but I'd like to solicit opinions from Steven and Edwin before it lands.

Comment 12

[Steven Johnson]

I generally agree with Lars, but IMHO the patch needs cleanup:

One must-fix issue: declaring "extern LoggingFunction logFunc" in a cpp file is a recipe for pain down the road; extern declarations really *must* live in a header file.

Style nits on this patch:

-- We have a GetCurrentLogFunction(), so why do we also need to expose the global variable?
-- if logFunc is going to be global, use the naming convention used elsewhere, "gLogFunc" or "g_logFunc".

Comment 13

[Edwin Smith]

Comment on attachment 504805 [details] [diff] [review]
Patch v3

Agree with Lars + Steven.  if this is trivial code then lets just duplicate it and not pollute vmbase.  if its nontrivial then lets find a good place to share it between platforms.  (/platform?)

Comment 14

[Ruchi Lohani]

Attachment: Patch v4

Moved the Logging methods to the GenericPortUtils.cpp in VMPI.

Comment 15

[Tamarin Bot]

changeset: 6559:8f002fb91ddd
user:      Ruchi Lohani<[e-mail]>
summary:   Bug 592187 - Restore back the log function in VMPI_spyCallback (r=treilly, sr=lhansen)

http://hg.mozilla.org/tamarin-redux/rev/8f002fb91ddd