[Lldb-commits] [lldb] r216398 - Change back all paths returns for lldb::PathType enumerations from HostInfo::GetLLDBPath() to return the directories in the FileSpec.m_directory field to match previous implementations. This change previously broke some path stuff in upstream branches.
Zachary Turner
zturner at google.com
Tue Aug 26 12:39:21 PDT 2014
I guess I'm thinking of this as the standard unix basename / dirname
paradigm. Everyone understands this paradigm and there's no confusion
about it. And given that FileSpec already internally stores its components
in this format, it would be a natural fit for m_filename and m_dirname to
correspond to basename and dirname, respectively. In fact, the entire
class is already implemented with this assumption. See, for example,
SetFile() which can be used to set arbitrary paths, which could refer to
directories, but which still splits the last component and stores it in
m_filename. Or AppendPathComponent(), which handles the case where
m_filename is not null, hence assuming that it must refer to a directory.
As for the places that broke as a result of my last change - certainly they
need to be fixed, so "reverting to green" is reasonable until we can decide
if there's a better fix. However, I did run all tests, and none of them
failed. I don't think the new test which checks that GetFilename() is null
is particularly useful, because it just tests an implementation detail of
the class, and not a documented behavior of the class. It should be
possible to write a test against whatever failed, which probably would be a
better test.
In any case, back to my original question: Is there any object to changing
FileSpec::GetDirectory() and FileSpec::GetFilename() to be GetBaseName()
and GetDirName()? This would require changing the call sites of the places
who previously broke as a result of my original change. I don't actually
know what broke since my test runs were all successful, though. If this is
not appropriate, then I think we need to change AppendPathComponent(),
SetFile(), and various other functions to stop allowing situations where
m_directory + m_filename refers to a directory.
On Tue, Aug 26, 2014 at 12:05 PM, <jingham at apple.com> wrote:
> There are some instances where you know something is a directory +
> filename when you make the FileSpec, even though you can't stat the
> FileSpec later on to determine this. For instance, when you get the list
> of loaded libraries from a running binary, the last component IS a file
> name because it would make no sense for it not to be, but you might be
> debugging remotely so you can't stat the file to verify that. Similarly,
> in the FileSpec made from a DWARF compile unit you know what the file and
> directories are, but you might not be on the machine where the binary was
> built, so you can't use stat to tell what the thing.
>
> So it does seem worthwhile to have the FileSpec keep the distinction
> between directories and files. I would not be surprised if we were getting
> sloppy about this and when we had directories storing the last component in
> the "Filename" slot. If there are performance reasons do that, we should
> start setting the type, and maybe change GetFilename to GetLastComponent or
> something like that. It does seem like this is a pretty common operation.
> But it would be cleaner to leave the "filename" slot for the file name if
> it exists, and push the directory components into the directory slot.
>
> Jim
>
>
> > On Aug 26, 2014, at 11:31 AM, Zachary Turner <zturner at google.com> wrote:
> >
> > In other words, it seems to me like m_directory and m_filename are
> implementation details whose only purpose is to reduce memory usage. i.e.
> since they use ConstString, it uses the fact that a directory can have many
> children (i.e. a one to many relationship) and gets more cache hits on the
> string pool by removing the last component of the path. Internally,
> however, these fields are not necessarily treated as strict filenames and
> directory names, but external users of the class, due to the confusing
> names, do treat them as strict filenames and directory names. If we could
> remove this ambiguity, and just provide GetPath() and GetParent(), and
> remove the ability to manually assign to the implementation details of the
> class, I think it would make things less confusing and more robust.
> >
> >
> > On Tue, Aug 26, 2014 at 11:20 AM, Zachary Turner <zturner at google.com>
> wrote:
> > I think what I'm getting at is that the interface to the class is just
> confusing. For example, AppendPathComponent(x) will append together
> m_directory, m_filename, and x, then put everything after the last slash
> into m_filename and everything else into m_directory. So this function's
> understanding of the way the class is represented is that "filename" is not
> actually necessarily a filename, but rather just everything after the last
> slash. Different parts of the class seem to have a different understanding
> of what things mean. I think a better design would be to just delete the
> methods GetFilename() and GetDirectory(), and leave GetPath() (which
> returns the full string) and additional method GetParent() which returns
> m_filename. If you really want to know what type of object it is then stat
> it. Incidentally, there's an enumeration in there that is supposed to
> represent what type of FileSystem object it is (filesystem, directory,
> socket, etc) and which is never used for anything.
> >
> >
> > On Tue, Aug 26, 2014 at 11:07 AM, <jingham at apple.com> wrote:
> >
> > > On Aug 25, 2014, at 7:05 PM, Zachary Turner <zturner at google.com>
> wrote:
> > >
> > > It's possible I'm misunderstanding how this class works, or is
> supposed to work. Either way, its interface is confusing as written, so
> perhaps it needs to be re-thought.
> > >
> > > But, if you call FileSpec::SetFile(foo), then my understanding is that
> foo represents an arbitrary path. It might be a directory, it might be a
> file, doesn't matter. FileSpec internally stores everything except the
> last component as the "directory", and the last component as the "file",
> even if directory+file actually points to a directory.
> > >
> > > Is this not how it's supposed to work? I think it's confusing and
> inconsistent if that's not how it works. In other words, a multi-component
> "directory" field and a null path should be an invalid FileSpec in my
> mind. If you want to get back the entire thing, don't use
> file_spec.GetDirectory(), just use file_spec.GetPath(), that's what it's
> for.
> > >
> > > Does this make sense? Thoughts?
> >
> > But in this reading, FileSpec::GetFilename() returns that last directory
> component, doesn't it? That seems odd to me. I would expect GetFilename
> of a directory to return an empty string, since it isn't a file.
> >
> > Jim
> >
> >
> > >
> > >
> > > On Mon, Aug 25, 2014 at 11:21 AM, Greg Clayton <gclayton at apple.com>
> wrote:
> > > Author: gclayton
> > > Date: Mon Aug 25 13:21:06 2014
> > > New Revision: 216398
> > >
> > > URL: http://llvm.org/viewvc/llvm-project?rev=216398&view=rev
> > > Log:
> > > Change back all paths returns for lldb::PathType enumerations from
> HostInfo::GetLLDBPath() to return the directories in the
> FileSpec.m_directory field to match previous implementations. This change
> previously broke some path stuff in upstream branches.
> > >
> > >
> > > Modified:
> > > lldb/trunk/source/Host/common/HostInfoBase.cpp
> > > lldb/trunk/source/Host/linux/HostInfoLinux.cpp
> > > lldb/trunk/source/Host/macosx/HostInfoMacOSX.mm
> > > lldb/trunk/source/Host/posix/HostInfoPosix.cpp
> > > lldb/trunk/source/Host/windows/HostInfoWindows.cpp
> > >
> > > Modified: lldb/trunk/source/Host/common/HostInfoBase.cpp
> > > URL:
> http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Host/common/HostInfoBase.cpp?rev=216398&r1=216397&r2=216398&view=diff
> > >
> ==============================================================================
> > > --- lldb/trunk/source/Host/common/HostInfoBase.cpp (original)
> > > +++ lldb/trunk/source/Host/common/HostInfoBase.cpp Mon Aug 25 13:21:06
> 2014
> > > @@ -228,7 +228,7 @@ HostInfoBase::ComputeSharedLibraryDirect
> > > Host::GetModuleFileSpecForHostAddress(reinterpret_cast<void
> *>(reinterpret_cast<intptr_t>(HostInfoBase::GetLLDBPath))));
> > >
> > > // Remove the filename so that this FileSpec only represents the
> directory.
> > > - file_spec.SetFile(lldb_file_spec.GetDirectory().AsCString(),
> true);
> > > + file_spec.GetDirectory() = lldb_file_spec.GetDirectory();
> > >
> > > return (bool)file_spec.GetDirectory();
> > > }
> > > @@ -264,7 +264,7 @@ HostInfoBase::ComputeTempFileDirectory(F
> > > // Make an atexit handler to clean up the process specify LLDB
> temp dir
> > > // and all of its contents.
> > > ::atexit(CleanupProcessSpecificLLDBTempDir);
> > > - file_spec.SetFile(pid_tmpdir.GetString().c_str(), false);
> > > +
> file_spec.GetDirectory().SetCStringWithLength(pid_tmpdir.GetString().c_str(),
> pid_tmpdir.GetString().size());
> > > return true;
> > > }
> > >
> > >
> > > Modified: lldb/trunk/source/Host/linux/HostInfoLinux.cpp
> > > URL:
> http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Host/linux/HostInfoLinux.cpp?rev=216398&r1=216397&r2=216398&view=diff
> > >
> ==============================================================================
> > > --- lldb/trunk/source/Host/linux/HostInfoLinux.cpp (original)
> > > +++ lldb/trunk/source/Host/linux/HostInfoLinux.cpp Mon Aug 25 13:21:06
> 2014
> > > @@ -194,7 +194,8 @@ HostInfoLinux::GetProgramFileSpec()
> > > bool
> > > HostInfoLinux::ComputeSystemPluginsDirectory(FileSpec &file_spec)
> > > {
> > > - file_spec.SetFile("/usr/lib/lldb", true);
> > > + FileSpec temp_file("/usr/lib/lldb", true);
> > > + file_spec.GetDirectory().SetCString(temp_file.GetPath().c_str());
> > > return true;
> > > }
> > >
> > > @@ -204,17 +205,15 @@ HostInfoLinux::ComputeUserPluginsDirecto
> > > // XDG Base Directory Specification
> > > //
> http://standards.freedesktop.org/basedir-spec/basedir-spec-latest.html
> > > // If XDG_DATA_HOME exists, use that, otherwise use
> ~/.local/share/lldb.
> > > - FileSpec lldb_file_spec;
> > > const char *xdg_data_home = getenv("XDG_DATA_HOME");
> > > if (xdg_data_home && xdg_data_home[0])
> > > {
> > > std::string user_plugin_dir(xdg_data_home);
> > > user_plugin_dir += "/lldb";
> > > - lldb_file_spec.SetFile(user_plugin_dir.c_str(), true);
> > > + file_spec.GetDirectory().SetCString(user_plugin_dir.c_str());
> > > }
> > > else
> > > - lldb_file_spec.SetFile("~/.local/share/lldb", true);
> > > -
> > > + file_spec.GetDirectory().SetCString("~/.local/share/lldb");
> > > return true;
> > > }
> > >
> > >
> > > Modified: lldb/trunk/source/Host/macosx/HostInfoMacOSX.mm
> > > URL:
> http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Host/macosx/HostInfoMacOSX.mm?rev=216398&r1=216397&r2=216398&view=diff
> > >
> ==============================================================================
> > > --- lldb/trunk/source/Host/macosx/HostInfoMacOSX.mm (original)
> > > +++ lldb/trunk/source/Host/macosx/HostInfoMacOSX.mm Mon Aug 25
> 13:21:06 2014
> > > @@ -149,7 +149,7 @@ HostInfoMacOSX::ComputeSupportExeDirecto
> > > ::strncpy(framework_pos, "/Resources", PATH_MAX -
> (framework_pos - raw_path));
> > > #endif
> > > }
> > > - file_spec.SetFile(raw_path, true);
> > > + file_spec.GetDirectory().SetCString(raw_path);
> > > return (bool)file_spec.GetDirectory();
> > > }
> > >
> > > @@ -169,7 +169,7 @@ HostInfoMacOSX::ComputeHeaderDirectory(F
> > > framework_pos += strlen("LLDB.framework");
> > > ::strncpy(framework_pos, "/Headers", PATH_MAX -
> (framework_pos - raw_path));
> > > }
> > > - file_spec.SetFile(raw_path, true);
> > > + file_spec.GetDirectory().SetCString(raw_path);
> > > return true;
> > > }
> > >
> > > @@ -199,7 +199,7 @@ HostInfoMacOSX::ComputePythonDirectory(F
> > > // We may get our string truncated. Should we protect this
> with an assert?
> > > ::strncat(raw_path, python_version_dir.c_str(),
> sizeof(raw_path) - strlen(raw_path) - 1);
> > > }
> > > - file_spec.SetFile(raw_path, true);
> > > + file_spec.GetDirectory().SetCString(raw_path);
> > > return true;
> > > }
> > >
> > > @@ -218,14 +218,15 @@ HostInfoMacOSX::ComputeSystemPluginsDire
> > >
> > > framework_pos += strlen("LLDB.framework");
> > > ::strncpy(framework_pos, "/Resources/PlugIns", PATH_MAX -
> (framework_pos - raw_path));
> > > - file_spec.SetFile(raw_path, true);
> > > + file_spec.GetDirectory().SetCString(raw_path);
> > > return true;
> > > }
> > >
> > > bool
> > > HostInfoMacOSX::ComputeUserPluginsDirectory(FileSpec &file_spec)
> > > {
> > > - file_spec.SetFile("~/Library/Application Support/LLDB/PlugIns",
> true);
> > > + FileSpec temp_file("~/Library/Application Support/LLDB/PlugIns",
> true);
> > > + file_spec.GetDirectory().SetCString(temp_file.GetPath().c_str());
> > > return true;
> > > }
> > >
> > >
> > > Modified: lldb/trunk/source/Host/posix/HostInfoPosix.cpp
> > > URL:
> http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Host/posix/HostInfoPosix.cpp?rev=216398&r1=216397&r2=216398&view=diff
> > >
> ==============================================================================
> > > --- lldb/trunk/source/Host/posix/HostInfoPosix.cpp (original)
> > > +++ lldb/trunk/source/Host/posix/HostInfoPosix.cpp Mon Aug 25 13:21:06
> 2014
> > > @@ -158,14 +158,15 @@ HostInfoPosix::ComputeSupportExeDirector
> > > log->Printf("Host::%s() failed to find /lib/liblldb
> within the shared lib path, bailing on bin path construction",
> > > __FUNCTION__);
> > > }
> > > - file_spec.SetFile(raw_path, true);
> > > + file_spec.GetDirectory().SetCString(raw_path);
> > > return (bool)file_spec.GetDirectory();
> > > }
> > >
> > > bool
> > > HostInfoPosix::ComputeHeaderDirectory(FileSpec &file_spec)
> > > {
> > > - file_spec.SetFile("/opt/local/include/lldb", false);
> > > + FileSpec temp_file("/opt/local/include/lldb", false);
> > > + file_spec.GetDirectory().SetCString(temp_file.GetPath().c_str());
> > > return true;
> > > }
> > >
> > > @@ -187,6 +188,6 @@ HostInfoPosix::ComputePythonDirectory(Fi
> > > // We may get our string truncated. Should we protect this with
> an assert?
> > > ::strncat(raw_path, python_version_dir.c_str(), sizeof(raw_path)
> - strlen(raw_path) - 1);
> > >
> > > - file_spec.SetFile(raw_path, true);
> > > + file_spec.GetDirectory().SetCString(raw_path);
> > > return true;
> > > }
> > >
> > > Modified: lldb/trunk/source/Host/windows/HostInfoWindows.cpp
> > > URL:
> http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Host/windows/HostInfoWindows.cpp?rev=216398&r1=216397&r2=216398&view=diff
> > >
> ==============================================================================
> > > --- lldb/trunk/source/Host/windows/HostInfoWindows.cpp (original)
> > > +++ lldb/trunk/source/Host/windows/HostInfoWindows.cpp Mon Aug 25
> 13:21:06 2014
> > > @@ -107,6 +107,6 @@ HostInfoWindows::ComputePythonDirectory(
> > > lldb_file_spec.AppendPathComponent("../lib/site-packages");
> > > lldb_file_spec.GetPath(raw_path, sizeof(raw_path));
> > >
> > > - file_spec.SetFile(raw_path, true);
> > > + file_spec.GetDirectory().SetCString(raw_path);
> > > return true;
> > > }
> > >
> > >
> > > _______________________________________________
> > > lldb-commits mailing list
> > > lldb-commits at cs.uiuc.edu
> > > http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
> > >
> > > _______________________________________________
> > > lldb-commits mailing list
> > > lldb-commits at cs.uiuc.edu
> > > http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
> >
> >
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20140826/6b2d1a21/attachment.html>
More information about the lldb-commits
mailing list