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 ↗

(On Diff #24108)

revert this

46 ↗

(On Diff #24108)

bool SetDirectory(const CMIUtilString &vPath);

75 ↗

(On Diff #24108)

const CMIUtilString m_constMediumFileNameFormat;

77–78 ↗

(On Diff #24108)

CMIUtilString m_strMediumFileName;
CMIUtilString m_strMediumFileDirectory;
CMIUtilString m_fileNamePath;

tools/lldb-mi/MICmnResources.cpp

60 ↗

(On Diff #24108)

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

-s<space>hortOption

so that, please revert.

67 ↗

(On Diff #24108)

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 ↗

(On Diff #24108)

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

630 ↗

(On Diff #24108)

revert this

tools/lldb-mi/MIUtilSystemWindows.cpp

139

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 ↗

(On Diff #24242)

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

191 ↗

(On Diff #24242)

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

tools/lldb-mi/MICmnLogMediumFile.cpp

 
     m_fileNamePath = MIRSRC(IDS_MEDIUMFILE_ERR_INVALID_PATH);
 
-    CMIUtilString strPathName;
-    if (CMIUtilSystem().GetLogFilesPath(strPathName))
+    CMIUtilString strPath;
+    if (CMIUtilSystem().GetLogFilesPath(strPath))
     {
-        const CMIUtilString strPath = CMIUtilFileStd::StripOffFileName(strPathName);
+        CMIUtilDateTimeStd date;
 
-// 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--%s", strPath.c_str(), date.GetDateTimeLogFilename().c_str(), m_constMediumFileName.c_str());
 #else
-        m_fileNamePath = CMIUtilString::Format("%s", m_constMediumFileName.c_str());
+        m_fileNamePath = CMIUtilString::Format("%s/%s--%s", strPath.c_str(), date.GetDateTimeLogFilename().c_str(), m_constMediumFileName.c_str());
 #endif // defined ( _MSC_VER )
 
         return MIstatus::success;

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/MIUtilSystemLinux.cpp

 bool
 CMIUtilSystemLinux::GetLogFilesPath(CMIUtilString &vrwFileNamePath) const
 {
-    vrwFileNamePath = CMIUtilString(".");
+    vrwFileNamePath = CMIUtilString(P_tmpdir);
     return MIstatus::success;
 }
 

tools/lldb-mi/MIUtilSystemWindows.cpp

 bool
 CMIUtilSystemWindows::GetLogFilesPath(CMIUtilString &vrwFileNamePath) const
 {
-    return GetExecutablesPath(vrwFileNamePath);
+    bool bOk = MIstatus::success;
+    char pPath[MAX_PATH];
+    const DWORD nLen = ::GetTempPath(MAX_PATH, &pPath[0]);
+    const CMIUtilString strLastErr(GetOSLastError());
+    if ((nLen != 0) && (strLastErr == "Unknown OS error"))
+        vrwFileNamePath = &pPath[0];
+    else
+    {
+        bOk = MIstatus::failure;
+        vrwFileNamePath = strLastErr;
+    }
+
+    return bOk;
 }
 
 #endif // #if defined( _MSC_VER )