[Lldb-commits] [PATCH] Reduce number of threads in lldb-mi.

[Ilia K]

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


================
Comment at: tools/lldb-mi/MICmnStreamStdin.h:125-126
@@ -124,2 +124,4 @@
     IOSStdinHandler *m_pStdinReadHandler;
+    static const int m_constBufferSize = 2048;
+    char m_cmdBuffer[m_constBufferSize];
 };
----------------
abidh wrote:
> ki.stfu wrote:
> > 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     }
> > ```
> What benefits that gives us to use dynamic buffer?
Very large buffers on stack may cause an segfault. But if dynamic memory fails it will throw bad_alloc exception (what is better :))

================
Comment at: tools/lldb-mi/MIDriver.cpp:968
@@ -1095,3 +967,3 @@
     {
-        InjectMICommand("-exec-interrupt");
+        InterpretCommand("-exec-interrupt");
         return;
----------------
abidh wrote:
> ki.stfu wrote:
> > The InjectMICommand prints the passed command using m_rStdOut.WriteMIResponse. You should do the same.
> 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.
ok

================
Comment at: tools/lldb-mi/MIDriver.cpp:1177-1179
@@ -1304,5 +1176,5 @@
 {
     const CMIUtilString strCmd(CMIUtilString::Format("-file-exec-and-symbols %s", m_strCmdLineArgExecuteableFileNamePath.c_str()));
 
-    return InjectMICommand(strCmd);
+    return InterpretCommand(strCmd);
 }
----------------
abidh wrote:
> ki.stfu wrote:
> > 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));
> > ```
> Why is TextToStdout necessary? There are many small clean-up and fixes that can be done. But not all in one revision.
Agree, but a cleaning of CMICmnStreamStdin class not affect to lldb-mi functionality unlike this case. Please fix it in this commit.

================
Comment at: tools/lldb-mi/MIDriverMain.cpp:137
@@ -136,3 +137,1 @@
-    // Signal MI to shutdown or halt a running debug session
-    CMICmnStreamStdin::Instance().SetCtrlCHit();
 }
----------------
abidh wrote:
> ki.stfu wrote:
> > Should we remove it too?
> I did not understand. What should we remove?
I meant CMICmnStreamStdin::SetCtrlCHit

================
Comment at: tools/lldb-mi/MIDriverMgr.h:82
@@ -81,2 +81,3 @@
         virtual const CMIUtilString &GetId(void) const = 0;
+        virtual void DeliverSignal(int) = 0;
 
----------------
abidh wrote:
> ki.stfu wrote:
> > Please, specify an argument name and let's use lldb-mi style:
> > ```
> > virtual void DeliverSignal(const int vnSignal) = 0;
> > ```
> 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.
lldb-mi is too big. I'm not sure about it.

http://reviews.llvm.org/D7746

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/