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

[Hafiz Abid Qadeer]

> 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 http://reviews.llvm.org/D7746#126540, @ki.stfu wrote:

> In http://reviews.llvm.org/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.

In http://reviews.llvm.org/D7746#126634, @ki.stfu wrote:

> 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.


================
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];
 };
----------------
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?

================
Comment at: tools/lldb-mi/MIDriver.cpp:968
@@ -1095,3 +967,3 @@
     {
-        InjectMICommand("-exec-interrupt");
+        InterpretCommand("-exec-interrupt");
         return;
----------------
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.

================
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);
 }
----------------
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.

================
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();
 }
----------------
ki.stfu wrote:
> Should we remove it too?
I did not understand. What should we remove?

================
Comment at: tools/lldb-mi/MIDriverMgr.h:82
@@ -81,2 +81,3 @@
         virtual const CMIUtilString &GetId(void) const = 0;
+        virtual void DeliverSignal(int) = 0;
 
----------------
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.

http://reviews.llvm.org/D7746

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