Page MenuHomePhabricator

Refactor many file functions to use FileSpec over strings.
ClosedPublic

Authored by chaoren on May 12 2015, 5:09 PM.

Details

Summary

This should solve the issue of sending denormalized paths over gdb-remote
if we stick to GetPath(false) in GDBRemoteCommunicationClient, and let the
server handle any denormalization.

Diff Detail

Repository
rL LLVM

Event Timeline

chaoren retitled this revision from to Refactor many file functions to use FileSpec over strings..
chaoren updated this object.
chaoren edited the test plan for this revision. (Show Details)
chaoren added a subscriber: Unknown Object (MLST).

This change is not very complicated, but spans over many many files. This is sort of a rough draft before I spend more time testing/fixing it on all platforms.

clayborg requested changes to this revision.May 13 2015, 9:50 AM
clayborg edited edge metadata.

So I stopped looking, but the following fixes need to be made:

  • don't remove spaces between function names and the first paren
  • when using FileSpec::GetPath().c_str(), you must use that on a single line for a function call, or store the result in a local std::string. There are many places where you do:
const char *s = my_spec.GetPath().c_str(); // s contains a reference to freed memory

foo(s); // Can crash due to "s" containing questionable contents.
include/lldb/Target/Platform.h
251–264 ↗(On Diff #25650)

Revert these space removals?

source/API/SBLaunchInfo.cpp
178 ↗(On Diff #25650)

This is a crasher waiting to happen. You can't make a temp std::string and then call c_str() on it, the storage will go away. The only way to hand out "const char *" via the API is to make it a ConstString() so it gets uniqued and stored permanently in the string pool:

return ConstString(m_opaque_sp->GetWorkingDirectory().GetPath()).GetCString();

Once a string is in the constant string pool it never gets removed, so the above approach is the way to do it.

source/API/SBPlatform.cpp
321 ↗(On Diff #25650)

This will crash. Fix it like in SBLaunchInfo::GetWorkingDirectory().

543 ↗(On Diff #25650)

You need to store the path into a std::string so the storage doesn't go away otherwise the "GetPath()" temporary will clean itself up after this. You might have seen this pattern elsewhere as:

call_some_function(my_spec.GetPath().c_str(), ...);

This pattern is OK because the temporary will live until after the function call. But if you use two separate calls in separate expression statements, then you can't.

source/Host/common/Host.cpp
846 ↗(On Diff #25650)

Can crash. Please store in local std::string:

std::string working_dir = launch_info.GetWorkingDirectory().GetPath();

Then use as "working_dir.c_str()".

source/Host/macosx/Host.mm
416 ↗(On Diff #25650)

Note this is ok hello use working_dir.GetPath().c_str() as C++ will keep the temp around for the duration of the "Printf()" call.

This revision now requires changes to proceed.May 13 2015, 9:50 AM
zturner edited edge metadata.May 13 2015, 10:53 AM

So I stopped looking, but the following fixes need to be made:

  • don't remove spaces between function names and the first paren

I thought we removed the part from the coding standard about putting a space between the function name and the first parenthesis. Unless you just mean "Don't introduce unnecessary churn to remove spaces unless you're already editing the signature of the function anyway"

Zachary: yes that was my point. There were functions above and below that still had spaces and only a few were fixed. We should fix these kinds of things in separate patches and if we fix one, we should fix them all.

Ok, cool. Just making sure. Agree we shouldn't be changing things for
style issues alone.

chaoren edited edge metadata.

Address review comments.

chaoren edited edge metadata.

CreateProcessA should take NULL instead of empty string.

Just one place to fix the need to not have to make a string into a ConstString when passing it to ProcessMonitor constructor. See inlined comment.

source/Plugins/Process/POSIX/ProcessPOSIX.cpp
208 ↗(On Diff #25753)

You shouldn't need to constify this string. If ProcessMonitor takes "const char *" args, it should be making copies of the strings in std::string buffers if needed, and not assuming the storage for the strings will stay around.

clayborg requested changes to this revision.May 14 2015, 10:07 AM
clayborg edited edge metadata.

See previous comment for change request reasons.

This revision now requires changes to proceed.May 14 2015, 10:07 AM
chaoren added inline comments.May 14 2015, 12:24 PM
source/Plugins/Process/POSIX/ProcessPOSIX.cpp
208 ↗(On Diff #25753)

Unfortunately, ProcessMonitor simply passes working_dir to LaunchArgs, which then just assigns it directly to m_working_dir to be used later. I'm going to convert ProcessMonitor/LaunchArgs to use FileSpecs too.

chaoren edited edge metadata.
  • Convert ProcessMonitor, FileAction, etc to use FileSpecs as well.
chaoren added inline comments.May 14 2015, 3:10 PM
source/Host/common/Host.cpp
686 ↗(On Diff #25814)
&((*command_output_ptr)[0])

I don't think this has a well defined behavior.

chaoren edited edge metadata.
  • Remove undefined behavior of writing to std::string internals.
  • Convert NativeProcessLinux, SetSTD{IN, OUT, ERR}, etc to use FileSpecs.
  • Update ProcessLauncherAndroid to use FileSpecs.
  • Accidentally made LaunchArgs members references.
ovyalov added inline comments.May 17 2015, 1:20 PM
source/Host/common/Host.cpp
687 ↗(On Diff #25847)

Could you check for error before assigning?

source/Plugins/Process/Linux/NativeProcessLinux.cpp
1466 ↗(On Diff #25847)

Is it guaranteed that path that launch_info.GetWorkingDirectory()} returns is resolved or we need to call working_dir.ResolvePath() ?

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
735 ↗(On Diff #25847)

FileSpec{path, true} ?

774 ↗(On Diff #25847)

FileSpecMissing repository, expected "{src repository:path ...}" or "{src path repository=...}" in: {src, true}, FileSpecMissing repository, expected "{src repository:path ...}" or "{src path repository=...}" in: {src, false}?

786 ↗(On Diff #25847)

FileSpec{path, true}

source/Target/FileAction.cpp
45 ↗(On Diff #25847)

m_file_spec.GetPath().c_str() returns pointer to a temp std::string object - could you modify method's signature to return std::string? Or remove this method at all since there is GetFileSpec method.

source/Target/Platform.cpp
698 ↗(On Diff #25847)

Is it needed to return with error here if (!src_resolved.ResolvePath() || !src_resolved) ?

848 ↗(On Diff #25847)

s/error.Success()/src_resolved.ResolvePath() && src_resolved

1513 ↗(On Diff #25847)

s/NULL/empty FileSpec

vharron edited edge metadata.May 18 2015, 2:45 PM

Only got through ProcessPOSIX, will continue later

source/API/SBLaunchInfo.cpp
178 ↗(On Diff #25847)

If ConstString gets destructed after GetWorkingDirectory () returns, will the memory pointed to by working_dir.GetCString() be freed?

source/API/SBPlatform.cpp
323 ↗(On Diff #25847)

If ConstString gets destructed after GetWorkingDirectory () returns, will the memory pointed to by working_dir.GetCString() be freed?

If the memory does get freed, you could probably fix this by switching the GetWorkingDirectory() result to a FileSpec

source/Plugins/Process/Linux/NativeProcessLinux.cpp
1784 ↗(On Diff #25847)

Maybe a slight difference in behavior? If working_dir != 0 but working_dir[0]==0, the chdir is skipped?

chaoren added inline comments.May 18 2015, 2:51 PM
source/API/SBLaunchInfo.cpp
178 ↗(On Diff #25847)

Yes, IIUC from Greg, ConstString uses a global string pool.

source/Plugins/Process/Linux/NativeProcessLinux.cpp
1784 ↗(On Diff #25847)

No it's good. FileSpec::operator bool() returns true only when m_filename or m_directory are valid and nonempty.

chaoren edited edge metadata.
  • Address review comments. More FileSpecs.

Also added a bunch of FileSpec wrapper functions for convenience.

Note: This diff is only tested and expected to work for Linux. OS X & Windows
are probably broken by this. Will submit another diff tomorrow to address those.

source/Host/common/Host.cpp
687 ↗(On Diff #25847)

Done. Also using vector instead because it looks nicer.

source/Plugins/Process/Linux/NativeProcessLinux.cpp
1466 ↗(On Diff #25847)

I think it's a good idea to resolve regardless. Done.

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
735 ↗(On Diff #25847)

Done.

774 ↗(On Diff #25847)

Done and done.

786 ↗(On Diff #25847)

Done.

source/Target/FileAction.cpp
45 ↗(On Diff #25847)

I added FileSpec::GetCString to prevent this kind mistakes once and for all.

source/Target/Platform.cpp
698 ↗(On Diff #25847)

Apparently FileSpec::ResolvePath() doesn't actually resolve symlinks, just resolves to an absolute path. I updated FileSystem::Readlink to take FileSpecs.

848 ↗(On Diff #25847)

See previous comment.

1513 ↗(On Diff #25847)

Done.

  • Minor cleanup to use new FileSpec wrapper functions.
  • Fix windows/FileSystem.
  • Fix macosx/Host.

Ping? I've tested this on Linux, FreeBSD, OS X, and Windows with no obvious regression.

clayborg accepted this revision.May 26 2015, 3:08 PM
clayborg edited edge metadata.

Looks good.

This revision is now accepted and ready to land.May 26 2015, 3:08 PM
chaoren edited edge metadata.
  • resolve_path should be false for mkdir.
This revision was automatically updated to reflect the committed changes.
emaste added inline comments.Jun 1 2015, 8:11 AM
lldb/trunk/source/Plugins/Process/Linux/ProcessMonitor.cpp
1188–1191

For better or worse Linux/ProcessMonitor.cpp and FreeBSD/ProcessMonitor.cpp are largely identical code, and changes to one should have a corresponding change to the other. I'll commit the FreeBSD version of this change soon.