This is an archive of the discontinued LLVM Phabricator instance.

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

Event Timeline

chaoren updated this revision to Diff 25650.May 12 2015, 5:09 PM
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 reviewers: zturner, clayborg, vharron, ovyalov.
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
254–264

Revert these space removals?

source/API/SBLaunchInfo.cpp
178–179

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–324

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

546–547

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–847

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

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 updated this revision to Diff 25747.May 13 2015, 5:29 PM
chaoren edited edge metadata.

Address review comments.

chaoren updated this revision to Diff 25753.May 13 2015, 7:11 PM
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
203

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
203

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 updated this revision to Diff 25814.May 14 2015, 2:42 PM
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
685–687
&((*command_output_ptr)[0])

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

chaoren updated this revision to Diff 25818.May 14 2015, 3:23 PM
chaoren edited edge metadata.
  • Remove undefined behavior of writing to std::string internals.
chaoren updated this revision to Diff 25840.May 14 2015, 7:06 PM
  • Convert NativeProcessLinux, SetSTD{IN, OUT, ERR}, etc to use FileSpecs.
chaoren updated this revision to Diff 25841.May 14 2015, 7:27 PM
  • Update ProcessLauncherAndroid to use FileSpecs.
chaoren updated this revision to Diff 25847.May 14 2015, 10:59 PM
  • Accidentally made LaunchArgs members references.
ovyalov added inline comments.May 17 2015, 1:20 PM
source/Host/common/Host.cpp
687

Could you check for error before assigning?

source/Plugins/Process/Linux/NativeProcessLinux.cpp
1466–1467

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

FileSpec{path, true} ?

774

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

FileSpec{path, true}

source/Target/FileAction.cpp
45–51

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

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

848

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

1513

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–179

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

source/API/SBPlatform.cpp
323

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

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–179

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

source/Plugins/Process/Linux/NativeProcessLinux.cpp
1784

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

chaoren updated this revision to Diff 26033.May 18 2015, 8:19 PM
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

Done. Also using vector instead because it looks nicer.

source/Plugins/Process/Linux/NativeProcessLinux.cpp
1466–1467

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

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
735

Done.

774

Done and done.

786

Done.

source/Target/FileAction.cpp
45–51

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

source/Target/Platform.cpp
698

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

848

See previous comment.

1513

Done.

chaoren updated this revision to Diff 26094.May 19 2015, 2:23 PM
  • 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 updated this revision to Diff 26621.May 27 2015, 11:36 AM
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 ↗(On Diff #26814)

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.