Page MenuHomePhabricator

Refactor Args a different way

Authored by zturner on Sep 30 2016, 9:19 AM.



In D24952 I tried to refactor the Args class to get rid of m_argv. The original motivation for this is that I wanted a way to implement GetArgumentAtIndex() so as to return a StringRef. I could have just returned a StringRef from the m_argv array, but it seems wasteful to calculate the length every time when we were already storing a std::string. Unfortunately the std::string was in a std::list (required so that it would not be invalidated when inserting items into the arg list) so there was no random access. So I wanted to see if I could do better, and that was the motivation for the original patch.

However, after fixing up all the places in the code to use the "new" Args, I wasn't completely satisfied. I like that the old code could just give you an argv pointer that you can pass to a system API. So I started over and ended up with this. Quick overview of changes:

  1. Instead of using a std::list<std::string> to own the memory, we use a std::vector<std::unique_ptr<char[]>>. This provides random access, which is something list couldn't provide, and it also provides non-const access to the underlying buffer, which is something that std::string couldn't provide, leading to a lot of const_cast which are no longer necessary.
  2. Each entry's quote char, backing memory (i.e. std::unique_ptr<char[]>), and StringRef with precomputed length are stored together in a single entry. This guarantees by design that the various arrays' lengths do not need to stay in sync with each other, because there is only one array.
  3. Some longstanding undefined behavior is documented and left for someone else to fix.
  4. Asserts are added to the code to document the pre-conditions. I know we are allergic to asserts, but I want to emphasize that none of these asserts have anything to do with the input parameters. The asserts are ALWAYS on internal class state.

With this change it should finally be easy to change GetArgumentAtIndex() to return a StringRef, because it can just return m_entries[idx].ref.

Diff Detail


Event Timeline

zturner updated this revision to Diff 73064.Sep 30 2016, 9:19 AM
zturner retitled this revision from to Refactor Args a different way.
zturner updated this object.
zturner added a reviewer: clayborg.
zturner added subscribers: lldb-commits, Restricted Project.

Just a drive by.

449 ↗(On Diff #73064)

This comment is no longer true given the change from protected to private just above.

96 ↗(On Diff #73064)

Minor formatting glitch with the \\ in the comment.

192 ↗(On Diff #73064)

This says "copy constructor" but it seems to be documenting the copy assignment operator.

195 ↗(On Diff #73064)

You got right of m_args, so maybe this whole comment needs a rewrite.

282 ↗(On Diff #73064)

m_args is gone.

zturner updated this revision to Diff 73095.Sep 30 2016, 10:52 AM
zturner updated this revision to Diff 73099.Sep 30 2016, 11:09 AM

Renamed EntryData to ArgEntry and made it public. This will be useful later for providing iterator support over the entries.

labath added a subscriber: labath.Oct 2 2016, 8:51 AM

random drive-by. Otherwise, I like it.

207 ↗(On Diff #73099)

I think we don't need to call Clear() here, as all memory owned by the object will be correctly destroyed anyway.

zturner added inline comments.Oct 2 2016, 8:55 AM
207 ↗(On Diff #73099)

Thanks for pointing that out. Originally I wasn't using unique_ptr but rather new[] and delete[]. After I changed to unique_ptr<char[]> this became unnecessary and I forgot to remove it. Thanks for pointing it out.

Hi Greg, might you have a chance to look at this today? I've got a huge backlog of CLs to get in. The rest probably won't require reviews, but this one is a precursor to everything else.

tfiala added a subscriber: tfiala.Oct 3 2016, 10:25 AM

@zturner , Greg is out this week (and was last Friday as well).

I'll get somebody over here to review.

I will test this on macOS. I will have the results this afternoon.

tfiala requested changes to this revision.Oct 3 2016, 1:30 PM
tfiala added a reviewer: tfiala.

I'm getting one test crash (segfault) in logging/

Exception Type:        EXC_BAD_ACCESS (SIGSEGV)
Exception Codes:       KERN_INVALID_ADDRESS at 0x0000000000000000
Exception Note:        EXC_CORPSE_NOTIFY

Termination Signal:    Segmentation fault: 11
Termination Reason:    Namespace SIGNAL, Code 0xb
Terminating Process:   exc handler [0]

VM Regions Near 0:
    __TEXT                 0000000102994000-0000000102995000 [    4K] r-x/rwx SM=COW  /System/Library/Frameworks/Python.framework/Versions/2.7/Resources/

Application Specific Information:
HandleCommand(command = "log disable lldb")

Thread 0 Crashed:: Dispatch queue:
0                      	0x00000001062ca132 lldb_private::DisableLog(char const**, lldb_private::Stream*) + 50 (Logging.cpp:96)
1                      	0x0000000105d90d44 CommandObjectLogDisable::DoExecute(lldb_private::Args&, lldb_private::CommandReturnObject&) + 1028 (CommandObjectLog.cpp:255)
2                      	0x00000001062c5da8 lldb_private::CommandObjectParsed::Execute(char const*, lldb_private::CommandReturnObject&) + 1096 (CommandObject.cpp:1011)
3                      	0x000000010628a64e lldb_private::CommandInterpreter::HandleCommand(char const*, lldb_private::LazyBool, lldb_private::CommandReturnObject&, lldb_private::ExecutionContext*, bool, bool) + 10174 (CommandInterpreter.cpp:1691)
4                      	0x00000001033d282e lldb::SBCommandInterpreter::HandleCommand(char const*, lldb::SBExecutionContext&, lldb::SBCommandReturnObject&, bool) + 622 (SBCommandInterpreter.cpp:201)
5                      	0x00000001033d2581 lldb::SBCommandInterpreter::HandleCommand(char const*, lldb::SBCommandReturnObject&, bool) + 81 (SBCommandInterpreter.cpp:177)
6                      	0x000000010353826b _wrap_SBCommandInterpreter_HandleCommand__SWIG_0(_object*, _object*) + 779 (LLDBWrapPython.cpp:11848)
7                      	0x000000010347036d _wrap_SBCommandInterpreter_HandleCommand(_object*, _object*) + 733 (LLDBWrapPython.cpp:12089)

This is on public macOS Sierra and Xcode.

If that doesn't give you enough to track it down, I can take a look in a bit.

This revision now requires changes to proceed.Oct 3 2016, 1:30 PM
zturner updated this revision to Diff 73335.Oct 3 2016, 1:43 PM
zturner edited edge metadata.

I know what this is. It should be fixed in this patch, I guess I didn't have the newest patch uploaded.

tfiala added a comment.Oct 3 2016, 1:44 PM

I know what this is. It should be fixed in this patch, I guess I didn't have the newest patch uploaded.

Okay, I'll give that a shot now.

tfiala accepted this revision.Oct 3 2016, 2:10 PM
tfiala edited edge metadata.

That works fine.


This revision is now accepted and ready to land.Oct 3 2016, 2:10 PM
jingham added a subscriber: jingham.Oct 3 2016, 2:39 PM

You messed up the meaning of one comment (noted inline). Otherwise this looks fine to me too.

97–98 ↗(On Diff #73099)

The example needs to be "Hello ""World" or it doesn't make sense. "Hello " "World" with a space in between would be two arguments.

This revision was automatically updated to reflect the committed changes.