Page MenuHomePhabricator

Initialize ProcessGDBRemoteLog for LLGS to fix remote platform logging
ClosedPublic

Authored by flackr on Mar 9 2015, 3:57 PM.

Details

Summary

This was previously initialized by ProcessGDBRemote::Initialize but lldb-server does not contain ProcessGDBRemote anymore so this needs to be initialized directly.

Diff Detail

Repository
rL LLVM

Event Timeline

flackr updated this revision to Diff 21525.Mar 9 2015, 3:57 PM
flackr retitled this revision from to Initialize ProcessGDBRemoteLog for LLGS by GDBRemoteCommunication::Initialize.
flackr updated this object.
flackr edited the test plan for this revision. (Show Details)
flackr added reviewers: tberghammer, ovyalov.
flackr set the repository for this revision to rL LLVM.
flackr added a subscriber: Unknown Object (MLST).
ovyalov edited edge metadata.Mar 9 2015, 4:37 PM

Please see my comments.

source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
142 ↗(On Diff #21525)

Maybe introduce ProcessGDBRemoteLog::Initialize() instead of this method?

We can put

Log::Callbacks log_callbacks = {
            ProcessGDBRemoteLog::DisableLog,
            ProcessGDBRemoteLog::EnableLog,
            ProcessGDBRemoteLog::ListLogCategories
        };

        Log::RegisterLogChannel (g_name, log_callbacks);

inside of ProcessGDBRemoteLog::Initialize and call it from ProcessGDBRemote::Initialize and lldb-server.cpp.

147 ↗(On Diff #21525)

Could you use std::call_once here?

tools/lldb-server/lldb-server.cpp
11 ↗(On Diff #21525)

GDBRemoteCommunication.h ?

tberghammer added inline comments.Mar 10 2015, 2:03 AM
source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
142 ↗(On Diff #21525)

I like your idea about creating ProcessGDBRemoteLog::Initialize, but I would prefer to call it from lldb.cpp:initializeLLGS().

flackr updated this revision to Diff 21668.Mar 10 2015, 6:36 PM
flackr edited edge metadata.

Move Log initialization to Log class Initialize functions and use std::call_once to guard against multiple calls.

flackr retitled this revision from Initialize ProcessGDBRemoteLog for LLGS by GDBRemoteCommunication::Initialize to Initialize ProcessGDBRemoteLog for LLGS to fix remote platform logging.Mar 10 2015, 6:37 PM
flackr edited the test plan for this revision. (Show Details)
flackr added inline comments.
source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
142 ↗(On Diff #21525)

Done, for ProcessPOSIXLog as well.

147 ↗(On Diff #21525)

Done.

tools/lldb-server/lldb-server.cpp
11 ↗(On Diff #21525)

Oops, done.

tberghammer edited edge metadata.Mar 11 2015, 3:17 AM

Please update ProcessFreeBSD::Initialize() to use the newly created ProcessPOSIXLog::Initialize() function.

source/Plugins/Process/Linux/ProcessLinux.cpp
64 ↗(On Diff #21668)

Please move this line into ProcessPOSIXLog::Initialize()

source/Plugins/Process/POSIX/ProcessPOSIXLog.cpp
41 ↗(On Diff #21668)

Please make it an argument, instead of a hard coded string.

tberghammer requested changes to this revision.Mar 11 2015, 3:17 AM
tberghammer edited edge metadata.
This revision now requires changes to proceed.Mar 11 2015, 3:17 AM
flackr updated this revision to Diff 21720.Mar 11 2015, 8:08 AM
flackr edited edge metadata.

Added argument to ProcessPOSIXLog to specify log channel name, called from ProcessFreeBSD::Initialize as well.

source/Plugins/Process/Linux/ProcessLinux.cpp
64 ↗(On Diff #21668)

Done.

source/Plugins/Process/POSIX/ProcessPOSIXLog.cpp
41 ↗(On Diff #21668)

Done.

tberghammer accepted this revision.Mar 11 2015, 8:11 AM
tberghammer edited edge metadata.
This revision is now accepted and ready to land.Mar 11 2015, 8:11 AM
ovyalov accepted this revision.Mar 11 2015, 10:46 AM
ovyalov edited edge metadata.

Looks good.

tools/lldb-server/lldb-gdbserver.cpp
490 ↗(On Diff #21720)

We'll need to apply such conditional compilation in lldb-platform.cpp as well https://github.com/llvm-mirror/lldb/blob/master/tools/lldb-server/lldb-platform.cpp#L108 - but not necessary for this CL.

tberghammer added inline comments.Mar 11 2015, 10:51 AM
tools/lldb-server/lldb-gdbserver.cpp
490 ↗(On Diff #21720)

lldb-platform not used on Windows (at least not yet)

This revision was automatically updated to reflect the committed changes.