Page MenuHomePhabricator

Delete LLDB_DEFAULT_SHELL, and determine the default shell at runtime
ClosedPublic

Authored by zturner on Oct 15 2014, 1:06 PM.

Details

Summary

This patch removes the #define for LLDB_DEFAULT_SHELL, instead relying on a runtime call to determine the default shell.

Since we return the default shell as a FileSpec instead of as a const char*, this patch opened the door for a further improvement of propagating this FileSpec usage to other places that had previously stored the shell as a char*, so we do that as well.

Diff Detail

Repository
rL LLVM

Event Timeline

zturner updated this revision to Diff 14959.Oct 15 2014, 1:06 PM
zturner retitled this revision from to Delete LLDB_DEFAULT_SHELL, and determine the default shell at runtime.
zturner updated this object.
zturner edited the test plan for this revision. (Show Details)
zturner added a reviewer: clayborg.
zturner added a subscriber: Unknown Object (MLST).
clayborg requested changes to this revision.Oct 15 2014, 1:23 PM
clayborg edited edge metadata.

You can't add members to any lldb::SB class, you will need to remove m_cached_shell and either put it into lldb_private::ProcessLaunchInfo. Why? Because we expect the lldb::SB API to be stable for people to link against and adding a new ivar changes the layout of the class. All lldb::SB classes must have a single member that is either a pointer, unique_ptr or shared_ptr and these can't change. Other rules for lldb::SB API classes include: no virtual functions and no inheritance.

This revision now requires changes to proceed.Oct 15 2014, 1:23 PM
In D5805#4, @clayborg wrote:

You can't add members to any lldb::SB class, you will need to remove m_cached_shell and either put it into lldb_private::ProcessLaunchInfo. Why? Because we expect the lldb::SB API to be stable for people to link against and adding a new ivar changes the layout of the class. All lldb::SB classes must have a single member that is either a pointer, unique_ptr or shared_ptr and these can't change. Other rules for lldb::SB API classes include: no virtual functions and no inheritance.

Can I make it a function local static? The issue is that the function returns a raw char*, so after my change it can't just return the result of m_opaque_sp->GetShell(). And I need to make sure its life extends beyond the scope of the function.

zturner updated this revision to Diff 15098.Oct 17 2014, 2:25 PM
zturner edited edge metadata.

Moves the member variable of SBTarget to a function local static, to avoid changing the ABI of the class.

The differential between patch 1 and patch 2 doesn't show up correctly since I used a different value for git diff -U, and I forgot the original value. The change in this patchset is only deleting the member variable from SBTarget and moving it to a function local static though. The full diff should still show correctly.

clayborg requested changes to this revision.Oct 17 2014, 3:04 PM
clayborg edited edge metadata.

In SBLaunchInfo::GetShell() just make a local ConstString from the shell and return GetCString(). Once a string has been const-ified, it will remain in the string pool forever. This will keep this function thread safe.

This revision now requires changes to proceed.Oct 17 2014, 3:04 PM
zturner updated this revision to Diff 15100.Oct 17 2014, 4:12 PM
zturner edited edge metadata.

Make a ConstString out of the path before returning it.

clayborg accepted this revision.Oct 17 2014, 5:23 PM
clayborg edited edge metadata.

Looks good.

This revision is now accepted and ready to land.Oct 17 2014, 5:23 PM
zturner closed this revision.Oct 20 2014, 10:57 AM
zturner updated this revision to Diff 15143.

Closed by commit rL220217 (authored by @zturner).