This is an archive of the discontinued LLVM Phabricator instance.

Reduce number of threads in lldb-mi.
ClosedPublic

Authored by abidh on Feb 19 2015, 2:40 AM.

Details

Reviewers
emaste
ki.stfu
Summary

LLDB-mi have 3 threads.

  1. Wait for input.
  2. Process commands.
  3. Process events.

This revision merges 1 & 2. Same thread waits on input and then process the
command. This way, no synchronization is needed between first and 2nd. Also it is
easy to check when to exit.

This revision just makes the minimal changes to use one thread for reading intput
and then processnig the commands. A lot of code has become redundant now. I will
clean it up gradually.

All lldb-mi tests pass with gcc and clang as test compiler. Also did minimal testing
on command line and works ok. The "quit" and "-gdb-exit" command close the application
without needing any further return.

After this revision goes in, it will not be possible to give the name of the
executable on command line and lldb-mi accepting it. This functionality is already
stubbed out in the code by he following #ifdef so we will not lose anything.
MICONFIG_ENABLE_MI_DRIVER_MI_MODE_CMDLINE_ARG_EXECUTABLE_DEBUG_SESSION

For posix system, it is even possible to make lldb-mi a single thread app. But on
windows, it is difficult to wait on stdin with timeout like what you can do with
select. So next best thing is to use 2 threads instead of 3.

Diff Detail

Event Timeline

abidh updated this revision to Diff 20265.Feb 19 2015, 2:40 AM
abidh retitled this revision from to Reduce number of threads in lldb-mi..
abidh updated this object.
abidh edited the test plan for this revision. (Show Details)
abidh added reviewers: ki.stfu, emaste.
abidh added a subscriber: Unknown Object (MLST).
abidh planned changes to this revision.Feb 19 2015, 2:43 AM
abidh updated this revision to Diff 20266.Feb 19 2015, 2:48 AM

Lock mutex before processing command.

ki.stfu edited edge metadata.Feb 19 2015, 4:49 AM

Extra cleaning is needed.

tools/lldb-mi/MIDriver.cpp
556

Why you don't remove CMICmnStreamStdin::ThreadRun() and CMICmnStreamStdin::MonitorStdin()?

588–609

I think you should use MICmnStreamStdin module. For example, m_rStdin.ReadLine() function.

Also you should check if we need IOSStdinHandler::InputAvailable and IOSStdinHandler::InterruptReadLine functions and use them (or remove).

631

We should remove ReadStdinLineQueue if it not used any more.

tools/lldb-mi/MIDriver.h
184

I think you should use existing code from MICmnStreamStdin.

ki.stfu requested changes to this revision.Feb 19 2015, 4:50 AM
ki.stfu edited edge metadata.
This revision now requires changes to proceed.Feb 19 2015, 4:50 AM

I have tested the Diff 2 on OS X, all tests passed.

abidh added inline comments.Feb 19 2015, 5:29 AM
tools/lldb-mi/MIDriver.cpp
588–609

As I said in my comments above. Not only these functions but a lot of other code will be redundant after this and I will remove them gradually.

631

I will surly do in a separate revision. There will be a lot of code cleanup.

tools/lldb-mi/MIDriver.h
184

A lot of those classes will be unnecessary after this revision and I plan to remove them. So only this Readline will be kept.

ki.stfu added a comment.EditedFeb 19 2015, 5:56 AM

After this revision goes in, it will not be possible to give the name of the executable on command line and lldb-mi accepting it. This functionality is already stubbed out in the code by he following #ifdef so we will not lose anything.

Why? We can use ExecuteCommand instead of InjectCommand in LocalDebugSessionStartupInjectCommands. I have tested it, and it works.

For posix system, it is even possible to make lldb-mi a single thread app. But on windows, it is difficult to wait on stdin with timeout like what you can do with select. So next best thing is to use 2 threads instead of 3.

Can you explain me, why? I don't mind, I'm just wondering.

abidh added a comment.Feb 19 2015, 6:06 AM

After this revision goes in, it will not be possible to give the name of the executable on command line and lldb-mi accepting it. This functionality is already stubbed out in the code by he following #ifdef so we will not lose anything.

Why? We can use ExecuteCommand instead of InjectCommand in LocalDebugSessionStartupInjectCommands. I have tested it, and it works.

You are right. We can keep this functionality. But I dont like code being stubbed out with random #ifdef. If it is needed then it should be enabled all times and tested. Otherwise it should be removed.

For posix system, it is even possible to make lldb-mi a single thread app. But on windows, it is difficult to wait on stdin with timeout like what you can do with select. So next best thing is to use 2 threads instead of 3.

Can you explain me, why? I don't mind, I'm just wondering.

On linux, I have experimented with a single-threaded lldb-mi and it works. The sequence is

  1. Wait for stdin with a timeout.
  2. If a command, process it and go to 4.
  3. If time out, go to 4.
  4. Process events if any and go to 1.

This worked on Linux. But problem is that 'select' call works only with sockets on Windows. I tried other Windows APIs but nothing provides a simple interface as select. So I abandoned the idea of making it single threaded.

ki.stfu added inline comments.Feb 19 2015, 6:44 AM
tools/lldb-mi/MIDriver.cpp
631

If be honest I'd prefer to do it simultaneously. It gives a chance to review all your changes at once and it will help me (and possible us) adopt a more exact solution.

abidh updated this revision to Diff 20294.Feb 19 2015, 6:53 AM
abidh edited edge metadata.

Remove ReadStdinLineQueue. Move ReadLine to the end of the file.

In D7746#126426, @abidh wrote:

After this revision goes in, it will not be possible to give the name of the executable on command line and lldb-mi accepting it. This functionality is already stubbed out in the code by he following #ifdef so we will not lose anything.

Why? We can use ExecuteCommand instead of InjectCommand in LocalDebugSessionStartupInjectCommands. I have tested it, and it works.

You are right. We can keep this functionality. But I dont like code being stubbed out with random #ifdef. If it is needed then it should be enabled all times and tested. Otherwise it should be removed.

We can enable this ability by default. It will not require additional efforts on startup.

For posix system, it is even possible to make lldb-mi a single thread app. But on windows, it is difficult to wait on stdin with timeout like what you can do with select. So next best thing is to use 2 threads instead of 3.

Can you explain me, why? I don't mind, I'm just wondering.

On linux, I have experimented with a single-threaded lldb-mi and it works. The sequence is

  1. Wait for stdin with a timeout.
  2. If a command, process it and go to 4.
  3. If time out, go to 4.
  4. Process events if any and go to 1.

This worked on Linux. But problem is that 'select' call works only with sockets on Windows. I tried other Windows APIs but nothing provides a simple interface as select. So I abandoned the idea of making it single threaded.

A small interval will load CPU. 2 threads is a good solution.

ki.stfu added inline comments.Feb 19 2015, 7:01 AM
tools/lldb-mi/MIDriver.h
184

I think we should move this functionality to CMICmnStreamStdin class and keep it (if you want to delete entire class).

ki.stfu requested changes to this revision.Feb 19 2015, 7:08 AM
ki.stfu edited edge metadata.

Let's summarize:

  • remove unnecessary code in this patch (don't do it in different revision)
  • keep CMICmnStreamStdin/CMICmnStreamStdout/CMICmnStreamStderr class
  • use CMICmnStreamStdin::ReadLine
  • use ExecuteCommand instead of InjectCommand in LocalDebugSessionStartupInjectCommands (and rename it to LocalDebugSessionStartupExecuteCommands)
This revision now requires changes to proceed.Feb 19 2015, 7:08 AM
abidh added a comment.Feb 19 2015, 7:42 AM

Let's summarize:

  • remove unnecessary code in this patch (don't do it in different revision)

Ok. I have done more cleanup. But a lot of things are connected. I prefer to go in relatively small patches. In that cases, it is easier to figure out which commit caused a breakage.

  • keep CMICmnStreamStdin/CMICmnStreamStdout/CMICmnStreamStderr class

I dont intend to remove them in this patch anyway. But longer term, we will see.

  • use CMICmnStreamStdin::ReadLine

I dont think that is correct. CMICmnStreamStdin::ReadLine delegate to classes for Windows and Linux. Windows class especailly touches some members which are set in InputAvailable which we are not using anymore.

When we have removed the input monitoring thread, most of the function in this class are redundant. Longer term, I think this class will go.

  • use ExecuteCommand instead of InjectCommand in LocalDebugSessionStartupInjectCommands (and rename it to LocalDebugSessionStartupExecuteCommands)

I have done that and removed Queue and InjectMICommands functions.

I will post updated patch shortly.

abidh added a comment.Feb 19 2015, 7:50 AM

The above patch does not handle the signal correctly. I will fix that too.

In D7746#126524, @abidh wrote:
  • use CMICmnStreamStdin::ReadLine

I dont think that is correct. CMICmnStreamStdin::ReadLine delegate to classes for Windows and Linux. Windows class especailly touches some members which are set in InputAvailable which we are not using anymore.
When we have removed the input monitoring thread, most of the function in this class are redundant. Longer term, I think this class will go.

We can reduce its logic and remove CMICmnStreamStdinWindows and CMICmnStreamStdinLinux. I just want to keep the stdin operations in separate file, as it were done for stdout/stderr. It looks good for me.

abidh updated this revision to Diff 20306.Feb 19 2015, 8:56 AM
abidh edited edge metadata.

Used CMICmnStreamStdin::ReadLine to read input.
Changed the scheme of how singal are handled.
Removed some un-used function and data from CMIDriver.

Looks good. What about further cleaning? We can clean CMICmnStreamStdin and remove MICmnStreamStdinLinux MICmnStreamStdinWindows classes. You want to do it in separate review?

tools/lldb-mi/MICmnStreamStdin.h
125–126 ↗(On Diff #20306)

Let's do it the same way as in MICmnStreamStdinLinux.cpp:

  • Specify m_constBufferSize and m_pCmdBuffer in header file:
67     // Attributes:
68   private:
69     const MIuint m_constBufferSize;
70     FILE *m_pStdin;
71     MIchar *m_pCmdBuffer;
  • Initialize in ctor:
42 CMICmnStreamStdinLinux::CMICmnStreamStdinLinux(void)
43     : m_constBufferSize(1024)
44     , m_pStdin(nullptr)
45     , m_pCmdBuffer(nullptr)
46     , m_waitForInput(true)
  • Create buffer in CMICmnStreamStdinLinux::Initialize:
83     // Other resources required
84     if (bOk)
85     {
86         m_pCmdBuffer = new MIchar[m_constBufferSize];
87         m_pStdin = stdin;
88     }
  • Use it in fgets:
199     // Read user input
200     const MIchar *pText = ::fgets(&m_pCmdBuffer[0], m_constBufferSize, stdin);
  • Don't forget to free it in CMICmnStreamStdinLinux::Shutdown:
126     // Tidy up
127     if (m_pCmdBuffer != nullptr)
128     {
129         delete[] m_pCmdBuffer;
130         m_pCmdBuffer = nullptr;
131     }
tools/lldb-mi/MIDriver.cpp
1131

The InjectMICommand prints the passed command using m_rStdOut.WriteMIResponse. You should do the same.

1340–1342

It should be like:

const CMIUtilString strCommand(CMIUtilString::Format("-file-exec-and-symbols \"%s\"", m_strCmdLineArgExecuteableFileNamePath.c_str()));
const bool bOk = CMICmnStreamStdout::TextToStdout(strCommand);
return (bOk && InterpretCommand(strCommand));
tools/lldb-mi/MIDriverMain.cpp
137 ↗(On Diff #20306)

Should we remove it too?

tools/lldb-mi/MIDriverMgr.cpp
792 ↗(On Diff #20306)

void DeliverSignal(const int vnSignal)

tools/lldb-mi/MIDriverMgr.h
82 ↗(On Diff #20306)

Please, specify an argument name and let's use lldb-mi style:

virtual void DeliverSignal(const int vnSignal) = 0;
110 ↗(On Diff #20306)

void DeliverSignal(const int vnSignal);

We can reduce its logic and remove CMICmnStreamStdinWindows and CMICmnStreamStdinLinux. I just want to keep the stdin operations in separate file, as it were done for stdout/stderr. It looks good for me.

In D7746#126524, @abidh wrote:
  • use CMICmnStreamStdin::ReadLine

I dont think that is correct. CMICmnStreamStdin::ReadLine delegate to classes for Windows and Linux. Windows class especailly touches some members which are set in InputAvailable which we are not using anymore.
When we have removed the input monitoring thread, most of the function in this class are redundant. Longer term, I think this class will go.

We can reduce its logic and remove CMICmnStreamStdinWindows and CMICmnStreamStdinLinux. I just want to keep the stdin operations in separate file, as it were done for stdout/stderr. It looks good for me.

OK I will move it CMICmnStreamStdin with a different name. There is already a function with similar name throughout the hierarchy with different signature.

Looks good. What about further cleaning? We can clean CMICmnStreamStdin and remove MICmnStreamStdinLinux MICmnStreamStdinWindows classes. You want to do it in separate review?

Yes. As I said earlier, there is a lot of code to cleanup. But we should do it in steps.

tools/lldb-mi/MICmnStreamStdin.h
125–126 ↗(On Diff #20306)

What benefits that gives us to use dynamic buffer?

tools/lldb-mi/MIDriver.cpp
1131

I did it intentionally. Eclipse uses signal to stop the execution. It we print that command on the output, it makes eclipse switch view and show it in console which does not look nice. It is an internal details that we stopped the execution using -exec-interrupt command. We could have called the lldb API directly. So we should not be printing that information anyway.

1340–1342

Why is TextToStdout necessary? There are many small clean-up and fixes that can be done. But not all in one revision.

tools/lldb-mi/MIDriverMain.cpp
137 ↗(On Diff #20306)

I did not understand. What should we remove?

tools/lldb-mi/MIDriverMgr.h
82 ↗(On Diff #20306)

Ok I will add the name, not sure about the lldb-mi style. I think we should slowly move the codebase to look similar to lldb proper.

abidh updated this revision to Diff 20317.Feb 19 2015, 10:41 AM
abidh edited edge metadata.

Add name of some function arguments.
Some smaller cleanup suggested in review.

ki.stfu accepted this revision.Feb 19 2015, 11:02 AM
ki.stfu edited edge metadata.

Fix lines 125-126 and 1,177-1,179 and feel free to commit.

tools/lldb-mi/MICmnStreamStdin.h
125–126 ↗(On Diff #20306)

Very large buffers on stack may cause an segfault. But if dynamic memory fails it will throw bad_alloc exception (what is better :))

tools/lldb-mi/MIDriver.cpp
1131

ok

1340–1342

Agree, but a cleaning of CMICmnStreamStdin class not affect to lldb-mi functionality unlike this case. Please fix it in this commit.

tools/lldb-mi/MIDriverMain.cpp
137 ↗(On Diff #20306)

I meant CMICmnStreamStdin::SetCtrlCHit

tools/lldb-mi/MIDriverMgr.h
82 ↗(On Diff #20306)

lldb-mi is too big. I'm not sure about it.

This revision is now accepted and ready to land.Feb 19 2015, 11:02 AM
zturner added inline comments.
tools/lldb-mi/MICmnStreamStdin.h
125–126 ↗(On Diff #20306)

Use dynamic buffer IMO. If your program is out of heap space good luck doing anything else anyway. Maybe this call doesn't fail by using stack, but the next call will. We just had this discussion with Greg the other day on lldb-dev, and we agreed that we should reduce the usage of stack buffers in LLDB as a whole. Performance of 1 heap allocation is not a concern, but using stack buffer reduces the amount of stack LLDB uses, and we don't know if it decides to do a deep recursion or something.

(BTW, if dynamic memory fails it won't throw bad_alloc exception, it will just terminate the program, because we compile with exceptions disabled. So whether it's here, or 5 lines of code later, doesn't really make a difference :)

abidh closed this revision.Feb 20 2015, 2:25 AM

Fix lines 125-126 and 1,177-1,179 and feel free to commit.

OK I have made the changes requested and committed it in 230003.