Page MenuHomePhabricator

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

Authored by ki.stfu on Apr 16 2015, 9:19 AM.

Details

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 ewan@codeplay.com

Diff Detail

Repository
rL LLVM

Event Timeline

EwanCrawford retitled this revision from to MI fix allowing multiple logging instances of lldb-mi to run simultaneously..
EwanCrawford updated this object.
EwanCrawford edited the test plan for this revision. (Show Details)
EwanCrawford added a reviewer: ki.stfu.
EwanCrawford set the repository for this revision to rL LLVM.
EwanCrawford added subscribers: deepak2427, Unknown Object (MLST).
ki.stfu requested changes to this revision.Apr 16 2015, 10:37 AM
ki.stfu edited edge metadata.
ki.stfu added a subscriber: zturner.
ki.stfu added inline comments.
tools/lldb-mi/MICmnLogMediumFile.cpp
222–224 ↗(On Diff #23861)

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 ↗(On Diff #23861)

Use the PATH_MAX instead

139–143 ↗(On Diff #23861)

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.

This revision now requires changes to proceed.Apr 16 2015, 10:37 AM
EwanCrawford edited edge metadata.

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.

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?

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 requested changes to this revision.Apr 17 2015, 8:15 AM
ki.stfu edited edge metadata.

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 ↗(On Diff #23918)

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")
This revision now requires changes to proceed.Apr 17 2015, 8:15 AM
EwanCrawford edited edge metadata.

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

ki.stfu requested changes to this revision.Apr 21 2015, 12:50 PM
ki.stfu edited edge metadata.

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 ↗(On Diff #24108)

Please, use the m_constMediumFileNameFormat as was suggested above.

, m_constMediumFileNameFormat("lldb-mi-%s.log")
32 ↗(On Diff #24108)

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 ↗(On Diff #24108)

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

222 ↗(On Diff #24108)
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 ↗(On Diff #24108)

see line #222

249–252 ↗(On Diff #24108)

revert these changes

255–259 ↗(On Diff #24108)
const CMIUtilString &
CMICmnLogMediumFile::GetFileName(void) const
{
    return m_strMediumFileName;
}
433 ↗(On Diff #24108)

rename

435 ↗(On Diff #24108)
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 ↗(On Diff #24108)
const bool bOk = GetExecutablesPath(vrwFileNamePath);
This revision now requires changes to proceed.Apr 21 2015, 12:50 PM

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 added a comment.EditedApr 22 2015, 6:04 AM

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.

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

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.

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 edited edge metadata.Apr 22 2015, 7:41 AM

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.

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 edited edge metadata.

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 requested changes to this revision.Apr 22 2015, 11:26 AM
ki.stfu edited edge metadata.

lgtm, apart from a few comments.

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")

This revision now requires changes to proceed.Apr 22 2015, 11:26 AM
EwanCrawford edited edge metadata.

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 commandeered this revision.Apr 23 2015, 5:47 AM
ki.stfu edited reviewers, added: EwanCrawford; removed: ki.stfu.
ki.stfu updated this revision to Diff 24295.Apr 23 2015, 5:48 AM
ki.stfu updated this object.

Remase against ToT

ki.stfu updated this object.Apr 23 2015, 5:49 AM
This revision was automatically updated to reflect the committed changes.