MI fix allowing multiple logging instances of lldb-mi to run simultaneously.

[ki.stfu]

Summary

Currently if two instances of lldb-mi are running with logging enabled using '--log' the log file conflicts. This produces the following error
MI: Error: File Handler. Error Permission denied opening 'C:\Users\Ewan\LLVM\build\Debug\bin\lldb-mi-log.txt'

Fixed in this patch by renaming lldb-mi-log.txt based on the date, e.g. lldb-mi-log.txt-20150316163631.log, and moving the file into the temp directory by using the --log-dir option.

Regrading previous review comments the P_tmpdir macro is defined in Windows but always points to "\", which doesn't help much. Also when using the Windows API for GetTempPath() dynamic memory seems much more messy.

Patch from [e-mail]

[ki.stfu]

tools/lldb-mi/MICmnLogMediumFile.cpp

222–224

I'd prefer to use base name followed by random prefix (like date-time or something else):

lldb-mi-log--2015-03-16--16-36-31.txt (1)

Also, I think the format should be reduced like the following:

lldb-m-20150316163631.log (2)

Another good way is to use the process id as prefix:

lldb-mi_37551.log (3)

So, use the (2) or (3) format please.

tools/lldb-mi/MIUtilSystemWindows.cpp

139

Use the PATH_MAX instead

139–143

I already have discussed it with @zturner. We should avoid allocation of large structures on the thread stack and use dynamic memory instead:

std::unique_ptr<char[]> apPath(new char[PATH_MAX]);
const DWORD nLen = ::GetTempPath(PATH_MAX, apPath.get());
const CMIUtilString strLastErr(GetOSLastError());
if ((nLen != 0) && (strLastErr == "Unknown OS error"))
    vrwFileNamePath = apPath.get();

Also, make sure you didn't forget to include the Platform.h for PATH_MAX const.

[EwanCrawford]

Updated log naming convention to lldb-mi-20150317122547.log

In order to include "Platform.h" for PATH_MAX I had to remove the timeval struct since it was multiply defined and appears to be unused.

[ki.stfu]

BTW, maybe it's more reasonable to place a log file in the current folder? Why you want to move it to the temp dir?

[EwanCrawford]

The temp directory was selected since in some cases the directory with the binary is read only and the user may not have permission to create the log file there.

[ki.stfu]

In D9054#157585, @EwanCrawford wrote:

The temp directory was selected since in some cases the directory with the binary is read only and the user may not have permission to create the log file there.

In such case we should add a new lldb-mi option like --log-dir=/tmp etc. Can you do that?

tools/lldb-mi/MICmnLogMediumFile.cpp

30

I think the m_constMediumFileName should be renamed to something like m_constMediumBaseFileName. or it would be better to use that as pattern of name:

, m_constMediumFileNameFormat("lldb-mi-%s.log")

[EwanCrawford]

Added --log-dir option to lldb-mi command line arguments, specifying the directory to place the log file into.

[ki.stfu]

rework this patch according to my remarks please. BTW, it will be nice to have a test for that option in the "test/tools/lldb-mi/startup_options/TestMiStartupOptions.py".

tools/lldb-mi/MICmnLogMediumFile.cpp

30

Please, use the m_constMediumFileNameFormat as was suggested above.

, m_constMediumFileNameFormat("lldb-mi-%s.log")

32

move 1 line above the current position and rename to:

, m_strMediumFileDirectory(MIRSRC(IDS_MEDIUMFILE_ERR_INVALID_PATH))

Also, add the following (before m_strMediumFileDirectory):

, m_strMediumFileName(MIRSRC(IDS_MEDIUMFILE_ERR_INVALID_PATH))

217

move the CMIUtilSystem().GetLogFilesPath(m_strMediumFileDirectory) to the CMICmnLogMediumFile::Initialize to avoid this if-branch

222

m_fileNamePath = CMIUtilString::Format("%s\\%s", m_strMediumFileDirectory.c_str(), m_strMediumFileName.c_str());

and init the m_strMediumFileName in CMICmnLogMediumFile::Initialize as follows:

CMIUtilDateTimeStd date;
m_strMediumFileName = CMIUtilString::Format(m_constMediumFileNameFormat.c_str(), date.GetDateTimeLogFilename().c_str());

224

see line #222

249–252

revert these changes

255–259

const CMIUtilString &
CMICmnLogMediumFile::GetFileName(void) const
{
    return m_strMediumFileName;
}

433

rename

435

m_strMediumFileDirectory = vPath;

tools/lldb-mi/MICmnLogMediumFile.h

41

revert this

46

bool SetDirectory(const CMIUtilString &vPath);

75

const CMIUtilString m_constMediumFileNameFormat;

77–78

CMIUtilString m_strMediumFileName;
CMIUtilString m_strMediumFileDirectory;
CMIUtilString m_fileNamePath;

tools/lldb-mi/MICmnResources.cpp

60

thx for catching but it was done deliberately (like --longOption):

-s<space>hortOption

so that, please revert.

67

I think we should specify /tmp dir as a separate argument which follows after the --log-dir option (the same way as the --source option works in lldb).

tools/lldb-mi/MIDriverMgr.cpp

519–523

if (0 == strArg.compare("--log-dir"))
{
    strLogDir = <nextArg>;
    bHaveArgLogDir = true;
}

630

revert this

tools/lldb-mi/MIUtilSystemWindows.cpp

140

const bool bOk = GetExecutablesPath(vrwFileNamePath);

[EwanCrawford]

Thanks for the feedback, I'll make these changes and add a test.

Should the log file default to the temp directory if the --log-dir option isn't specified?
This would prevent the write permission error from happening at all.

[ki.stfu]

In D9054#159829, @EwanCrawford wrote:

Should the log file default to the temp directory if the --log-dir option isn't specified?
This would prevent the write permission error from happening at all.

I think the default should be "." (i.e. current directory).

UPDATE:
I meant the current working directory, not dir of executable.

[deepak2427]

I think the default being the current directory "." would not be a good idea, if lldb-mi does not have the permission to write there. Unlike lldb, this is more important for lldb-mi, especially while debugging from a GUI, since the user wouldn't know what's happening and why lldb-mi failed.

The temp directory seems to be an appropriate place for the logs. Also since we are adding the --log-dir flag, the user can explicitly specify whichever directory they wish, including the current directory.

Thoughts?

[ki.stfu]

In D9054#159972, @deepak2427 wrote:

I think the default being the current directory "." would not be a good idea, if lldb-mi does not have the permission to write there. Unlike lldb, this is more important for lldb-mi, especially while debugging from a GUI, since the user wouldn't know what's happening and why lldb-mi failed.

The temp directory seems to be an appropriate place for the logs. Also since we are adding the --log-dir flag, the user can explicitly specify whichever directory they wish, including the current directory.

Thoughts?

Ok. I have no objections to change destination folder. But why /tmp? Have you thought about the /var/log folder, or to use the syslog.h API?

[deepak2427]

The temp directory was chosen since it's portable. There's no syslog.h in Windows and also no directory like /var/log I think.

[ki.stfu]

In D9054#159999, @deepak2427 wrote:

The temp directory was chosen since it's portable. There's no syslog.h in Windows and also no directory like /var/log I think.

AFAIK the syslog.h is supported on Windows platform... Anyway, it doesn't concern to this CL. So that let's use the /tmp folder for lldb-mi log, I don't mind.

[abidh]

In D9054#159972, @deepak2427 wrote:

I think the default being the current directory "." would not be a good idea, if lldb-mi does not have the permission to write there. Unlike lldb, this is more important for lldb-mi, especially while debugging from a GUI, since the user wouldn't know what's happening and why lldb-mi failed.

The temp directory seems to be an appropriate place for the logs. Also since we are adding the --log-dir flag, the user can explicitly specify whichever directory they wish, including the current directory.

Thoughts?

I don't mind either way. But current directory is more convenient. For example, you can set your working directory when making a connection in eclipse. It may not be very obvious for user on how to change the log directory say from within eclipse.

[ki.stfu]

In D9054#160013, @abidh wrote:

I don't mind either way. But current directory is more convenient. For example, you can set your working directory when making a connection in eclipse. It may not be very obvious for user on how to change the log directory say from within eclipse.

Agreed.

[EwanCrawford]

I kept the default directory as the current one, since for the majority of use cases it makes the most sense.

For having a separate argument which follows after the --log-dir option, I agree that this should be consistent with lldb.
However the way parameters are parsed at the moment any stand alone argument which isn't an option is used as an executable file name. See CMIDriver::ParseArgs() in MIDriver.cpp

Would adding a specific case here for directories following --log-dir option be the best way to avoid this? Seems a bit messy, otherwise rewrite the function?

[ki.stfu]

lgtm, apart from a few comments.

In D9054#160191, @EwanCrawford wrote:

Would adding a specific case here for directories following --log-dir option be the best way to avoid this? Seems a bit messy, otherwise rewrite the function?

Ok. Leave that as is, I'll do it later. FYI: I already have modified this function to accept arguments separately, but I haven't merged it yet. BUT keep in mind that later I'll change the --log-dir format to pass the dest dir as a separate argument.

test/tools/lldb-mi/startup_options/TestMiStartupOptions.py

158

use self.fail("log file not found")

191

use self.fail("log file not found")

[EwanCrawford]

If you've already modified the function then that would be ideal, no problems with the directory becoming a separate argument later.

Could you commit again thanks.

[ki.stfu]

Remase against ToT

test/tools/lldb-mi/startup_options/TestMiStartupOptions.py

 
         # Test that lldb-mi is ready when executable was loaded
         self.expect(self.child_prompt, exactly = True)
+    
+    @lldbmi_test
+    @expectedFailureWindows("llvm.org/pr22274: need a pexpect replacement for windows")
+    @skipIfFreeBSD # llvm.org/pr22411: Failure presumably due to known thread races
+    def test_lldbmi_log_option(self):
+        """Test that 'lldb-mi --log' creates a log file in the current directory."""
+    
+        logDirectory = "."
+        self.spawnLldbMi(args = "%s --log" % self.myexe)
 
+        # Test that lldb-mi is ready after startup
+        self.expect(self.child_prompt, exactly = True)
+
+        # Test that the executable is loaded when file was specified
+        self.expect("-file-exec-and-symbols \"%s\"" % self.myexe)
+        self.expect("\^done")
+
+        # Test that lldb-mi is ready when executable was loaded
+        self.expect(self.child_prompt, exactly = True)
+
+        # Run
+        self.runCmd("-exec-run")
+        self.expect("\^running")
+        self.expect("\*stopped,reason=\"exited-normally\"")
+
+        # Check log file is created
+        import glob,os
+        logFile = glob.glob(logDirectory + "/lldb-mi-*.log")
+
+        if not logFile:
+            self.fail("log file not found")
+
+        # Delete log
+        for f in logFile:
+            os.remove(f)
+
+    @lldbmi_test
+    @expectedFailureWindows("llvm.org/pr22274: need a pexpect replacement for windows")
+    @skipIfFreeBSD # llvm.org/pr22411: Failure presumably due to known thread races
+    def test_lldbmi_log_directory_option(self):
+        """Test that 'lldb-mi --log --log-dir' creates a log file in the directory specified by --log-dir."""
+    
+        # Create log in temp directory
+        import tempfile
+        logDirectory = tempfile.gettempdir()
+
+        self.spawnLldbMi(args = "%s --log --log-dir=%s" % (self.myexe,logDirectory))
+
+        # Test that lldb-mi is ready after startup
+        self.expect(self.child_prompt, exactly = True)
+
+        # Test that the executable is loaded when file was specified
+        self.expect("-file-exec-and-symbols \"%s\"" % self.myexe)
+        self.expect("\^done")
+
+        # Test that lldb-mi is ready when executable was loaded
+        self.expect(self.child_prompt, exactly = True)
+
+        # Run
+        self.runCmd("-exec-run")
+        self.expect("\^running")
+        self.expect("\*stopped,reason=\"exited-normally\"")
+
+        # Check log file is created
+        import glob,os
+        logFile = glob.glob(logDirectory + "/lldb-mi-*.log")
+
+        if not logFile:
+            self.fail("log file not found")             
+   
+        # Delete log
+        for f in logFile:
+            os.remove(f)
+       
 if __name__ == '__main__':
     unittest2.main()

tools/lldb-mi/MICmnLogMediumFile.h

     bool IsOk(void) const;
     bool IsFileExist(void) const;
     const CMIUtilString &GetLineReturn(void) const;
+    bool SetDirectory(const CMIUtilString &vPath);
 
     // Overridden:
   public:

     // Attributes:
   private:
     const CMIUtilString m_constThisMediumName;
-    const CMIUtilString m_constMediumFileName;
+    const CMIUtilString m_constMediumFileNameFormat;
     //
+    CMIUtilString m_strMediumFileName;
+    CMIUtilString m_strMediumFileDirectory;
     CMIUtilString m_fileNamePath;
     MIuint m_eVerbosityType;
     CMIUtilString m_strDate;

tools/lldb-mi/MICmnLogMediumFile.cpp

 //--
 CMICmnLogMediumFile::CMICmnLogMediumFile(void)
     : m_constThisMediumName(MIRSRC(IDS_MEDIUMFILE_NAME))
-    , m_constMediumFileName("lldb-mi-log.txt")
+    , m_constMediumFileNameFormat("lldb-mi-%s.log")
+    , m_strMediumFileName(MIRSRC(IDS_MEDIUMFILE_ERR_INVALID_PATH))
+    , m_strMediumFileDirectory(MIRSRC(IDS_MEDIUMFILE_ERR_INVALID_PATH))
     , m_fileNamePath(MIRSRC(IDS_MEDIUMFILE_ERR_INVALID_PATH))
     , m_eVerbosityType(CMICmnLog::eLogVerbosity_Log)
     , m_strDate(CMIUtilDateTimeStd().GetDate())

 bool
 CMICmnLogMediumFile::Initialize(void)
 {
-    m_bInitialized = FileFormFileNamePath();
+    m_bInitialized = CMIUtilSystem().GetLogFilesPath(m_strMediumFileDirectory);
+    m_bInitialized &= FileFormFileNamePath();
 
     return m_bInitialized;
 }

 
     m_fileNamePath = MIRSRC(IDS_MEDIUMFILE_ERR_INVALID_PATH);
 
-    CMIUtilString strPathName;
-    if (CMIUtilSystem().GetLogFilesPath(strPathName))
+    if (m_strMediumFileDirectory.compare(MIRSRC(IDS_MEDIUMFILE_ERR_INVALID_PATH)) != 0)
     {
-        const CMIUtilString strPath = CMIUtilFileStd::StripOffFileName(strPathName);
+        CMIUtilDateTimeStd date;
+        m_strMediumFileName = CMIUtilString::Format(m_constMediumFileNameFormat.c_str(), date.GetDateTimeLogFilename().c_str());
 
-// ToDo: Review this LINUX log file quick fix so not hidden
-// AD:
-//      Linux was creating a log file here called '.\log.txt'.  The '.' on linux
-//      signifies that this file is 'hidden' and not normally visible.  A quick fix
-//      is to remove the path component all together.  Linux also normally uses '/'
-//      as directory separators, again leading to the problem of the hidden log.
 #if defined(_MSC_VER)
-        m_fileNamePath = CMIUtilString::Format("%s\\%s", strPath.c_str(), m_constMediumFileName.c_str());
+        m_fileNamePath = CMIUtilString::Format("%s\\%s", m_strMediumFileDirectory.c_str(), m_strMediumFileName.c_str());
 #else
-        m_fileNamePath = CMIUtilString::Format("%s", m_constMediumFileName.c_str());
+        m_fileNamePath = CMIUtilString::Format("%s/%s", m_strMediumFileDirectory.c_str(), m_strMediumFileName.c_str());
 #endif // defined ( _MSC_VER )
 
         return MIstatus::success;

 const CMIUtilString &
 CMICmnLogMediumFile::GetFileName(void) const
 {
-    return m_constMediumFileName;
+    return m_strMediumFileName;
 }
 
 //++ ------------------------------------------------------------------------------------

 {
     return m_file.GetLineReturn();
 }
+
+//++ ------------------------------------------------------------------------------------
+// Details: Set the diretory to place the log file.
+// Type:    Method.
+// Args:    vPath   - (R) Path to log.
+// Return:  MIstatus::success - Functional succeeded.
+//          MIstatus::failure - Functional failed.
+// Throws:  None.
+//--
+bool
+CMICmnLogMediumFile::SetDirectory(const CMIUtilString &vPath)
+{
+    m_strMediumFileDirectory = vPath;
+
+    return FileFormFileNamePath();
+}

tools/lldb-mi/MICmnResources.h

     IDE_MI_APP_ARG_INTERPRETER,
     IDE_MI_APP_ARG_EXECUTEABLE,
     IDE_MI_APP_ARG_APP_LOG,
+    IDE_MI_APP_ARG_APP_LOG_DIR,
     IDE_MI_APP_ARG_EXAMPLE,
     IDE_MI_APP_ARG_EXECUTABLE,
 

tools/lldb-mi/MICmnResources.cpp

     {IDE_MI_APP_ARG_INTERPRETER, "--interpreter\n\t This option is kept for backward compatibility. This executable always run in MI mode"},
     {IDE_MI_APP_ARG_EXECUTEABLE, "--executable\n\tUse the MI Driver in MI mode for the debugging the specified executable." },
     {IDE_MI_APP_ARG_APP_LOG, "--log\n\tUse this argument to tell the MI Driver to update it's log\n\tfile '%s'."},
+    {IDE_MI_APP_ARG_APP_LOG_DIR, "--log-dir\n\tUse this argument to specify the directory the MI Driver\n\twill place the log file in, i.e --log-dir=/tmp." },
     {IDE_MI_APP_ARG_EXAMPLE, "Example MI command:\n\t3-info-gdb-mi-command gdb-set\n\t3^done,command={exists=\"true\"}"},
     {IDE_MI_APP_ARG_EXECUTABLE, "executable (NOT IMPLEMENTED)\n\tThe file path to the executable i.e. '\"C:\\My Dev\\foo.exe\"'."},
     {IDS_STDIN_ERR_INVALID_PROMPT, "Stdin. Invalid prompt description '%s'"},

tools/lldb-mi/MIDriverMgr.cpp

 //              --versionLong
 //              --log
 //              --executable
+//              --log-dir
 //          The above arguments are not handled by any driver object except for --executable.
 //          The options --interpreter and --executable in code act very similar. The
 //          --executable is necessary to differentiate whither the MI Driver is being using

 //          Driver is being called the command line and that the executable argument is indeed
 //          a specified executable an so actions commands to set up the executable for a
 //          debug session. Using --interpreter on the commnd line does not action additional
-//          commands to initialise a debug session and so be able to launch the process.
+//          commands to initialise a debug session and so be able to launch the process. The directory
+//          where the log file is created is specified using --log-dir.
 // Type:    Method.
 // Args:    argc        - (R)   An integer that contains the count of arguments that follow in
 //                              argv. The argc parameter is always greater than or equal to 1.

     bool bHaveArgVersion = false;
     bool bHaveArgVersionLong = false;
     bool bHaveArgLog = false;
+    bool bHaveArgLogDir = false;
     bool bHaveArgHelp = false;
+    CMIUtilString strLogDir;
 
     bHaveArgInterpret = true;
     if (bHaveArgs)

             {
                 bHaveArgLog = true;
             }
+            if (0 == strArg.compare(0,10,"--log-dir="))
+            {
+                strLogDir = strArg.substr(10,CMIUtilString::npos);
+                bHaveArgLogDir = true;
+            }
             if ((0 == strArg.compare("--help")) || (0 == strArg.compare("-h")))
             {
                 bHaveArgHelp = true;

         CMICmnLog::Instance().SetEnabled(true);
     }
 
+    if (bHaveArgLogDir)
+    {
+        bOk = bOk && CMICmnLogMediumFile::Instance().SetDirectory(strLogDir);
+    }
+
     // Todo: Remove this output when MI is finished. It is temporary to persuade Ecllipse plugin to work.
     //       Eclipse reads this literally and will not work unless it gets this exact version text.
     // Handle --version option (ignore the --interpreter option if present)

         MIRSRC(IDE_MI_APP_ARG_INTERPRETER),
         MIRSRC(IDE_MI_APP_ARG_EXECUTEABLE),
         CMIUtilString::Format(MIRSRC(IDE_MI_APP_ARG_APP_LOG), CMICmnLogMediumFile::Instance().GetFileName().c_str()),
+        MIRSRC(IDE_MI_APP_ARG_APP_LOG_DIR),
         MIRSRC(IDE_MI_APP_ARG_EXECUTABLE),
         MIRSRC(IDS_CMD_QUIT_HELP),
         MIRSRC(IDE_MI_APP_ARG_EXAMPLE)};

tools/lldb-mi/MIUtilDateTimeStd.h

 
     CMIUtilString GetDate(void);
     CMIUtilString GetTime(void);
+    CMIUtilString GetDateTimeLogFilename(void);
 
     // Overrideable:
   public:

tools/lldb-mi/MIUtilDateTimeStd.cpp

 
     return strTime;
 }
+
+//++ ------------------------------------------------------------------------------------
+// Details: Retrieve system local current date and time in yyyy-MM-dd--HH-mm-ss format for log file names.
+// Type:    Method.
+// Args:    None.
+// Return:  CMIUtilString - Text description.
+// Throws:  None.
+//--
+CMIUtilString
+CMIUtilDateTimeStd::GetDateTimeLogFilename(void)
+{
+    std::time(&m_rawTime);
+    const std::tm *pTi = std::localtime(&m_rawTime);
+    const CMIUtilString strTime(CMIUtilString::Format("%d%02d%02d%02d%02d%02d", pTi->tm_year + 1900, pTi->tm_mon,
+                                                      pTi->tm_mday, pTi->tm_hour, pTi->tm_min, pTi->tm_sec));
+
+    return strTime;
+}

tools/lldb-mi/MIUtilSystemWindows.cpp

 // In-house headers:
 #include "MIUtilSystemWindows.h"
 #include "MICmnResources.h"
+#include "MIUtilFileStd.h"
 
 //++ ------------------------------------------------------------------------------------
 // Details: CMIUtilSystemWindows constructor.

 bool
 CMIUtilSystemWindows::GetLogFilesPath(CMIUtilString &vrwFileNamePath) const
 {
-    return GetExecutablesPath(vrwFileNamePath);
+    vrwFileNamePath = CMIUtilString(".");
+    return MIstatus::success;
 }
 
 #endif // #if defined( _MSC_VER )