[Lldb-commits] Fix for double command completion text
Enrico Granata
egranata at apple.com
Tue Jun 18 10:22:47 PDT 2013
As far as I could tell, TestAliases.py worked with this patch
I think the main difference in logic between your original patch and this one is that your original attempt skipped the inexact-commands-only step. What the attached patch does is perform all the steps and at each step bail out if a unique match is found, with just that one match in the StringList.
Hopefully that does the magic without causing regressions :)
Let me know what you find!
Enrico Granata
📩 egranata@.com
☎️ 27683
On Jun 17, 2013, at 9:44 PM, Matthew Sorrels <sorrels.m at gmail.com> wrote:
> This is what I had attempted to do the first time, but it ran into problems with user created aliases that were super-sets of the base commands. It's why the TestAliases.py was failing for my patch. I do believe it's possible to get that path to work, but at that point I didn't trust that it would return the same results from GetCommandSP as before. Which is why I switched to just fixing the output part of the problem. I did try to do a "unique insert solution", but there is a lot of code that assumed that the number of matches was equal to the size of the StringList. It seemed more risky for how small a bug it is.
>
> I'll take a look at this tomorrow when I'm back in the office.
>
>
> On Mon, Jun 17, 2013 at 5:23 PM, Enrico Granata <egranata at apple.com> wrote:
>
>
>
> Please find my attempt at a patch for this bug included
> It essentially follows the above logic in GetCommandObject()
> I have tested completions and they work as expected:
> ./lldb
> (lldb) pl<TAB>
> Available completions:
> platform
> plugin
> (lldb) pl
>
> I have not yet done a full test suite run on this, but running a select subset of tests that specifically exercise the lldb command line has not uncovered any regression.
>
> Enrico Granata
> 📩 egranata@.com
> ☎️ 27683
>
> On Jun 17, 2013, at 4:45 PM, Enrico Granata <egranata at apple.com> wrote:
>
>> I have been looking at the same issue (duplicate completions), and it looks like the root issue is that CommandInterpreter::GetCommandSP() is acting both as an exact matcher (returning a CommandObjectSP) and as an inexact matcher (filling up a StringList), and is hardly effective at this dual task (its return value only says whether the exact match worked, not whether the inexact match also worked)
>> I am thinking that a good solution might be:
>> - try to get an exact match and do not fill the list
>> - if you get an exact match, then add that command to the list and bail out
>> - if you did not get an exact match, only then try to get inexact matches into the StringList
>>
>> Another alternative would, of course, be to have a InsertUnique() in StringList, even though either the RemoveDups() and the InsertUnique() seem more hacks around the issue than anything, at least IMHO.
>>
>> Enrico Granata
>> 📩 egranata@.com
>> ☎️ 27683
>>
>> On Jun 17, 2013, at 4:32 PM, Matthew Sorrels <sorrels.m at gmail.com> wrote:
>>
>>> Thanks for fixing that. I thought I was losing my mind there for a second. There are still issues with running the tests on Linux though (you need to use clang and you must add a -lstdc++ to the test/make/Makefile.rules to just get the number of failures down to only 4). And so many tests are skipped on Linux it's hard to say if any regressions are being added by new code.
>>>
>>> Here's a second attempt to fix this double completion bug. It works a bit differently since the whole command structure seems determined to keep adding on matches to the match list. This just removes the dups at the very end by adding a new function to StringsList to sort the list and remove the dups. Let me know what you think. If it looks reasonable please commit it. Thanks,
>>>
>>> Index: include/lldb/Core/StringList.h
>>> ===================================================================
>>> --- include/lldb/Core/StringList.h (revision 184143)
>>> +++ include/lldb/Core/StringList.h (working copy)
>>> @@ -72,6 +72,9 @@
>>> void
>>> RemoveBlankLines ();
>>>
>>> + void
>>> + RemoveDupsAndSort ();
>>> +
>>> size_t
>>> SplitIntoLines (const char *lines, size_t len);
>>>
>>> Index: source/Core/StringList.cpp
>>> ===================================================================
>>> --- source/Core/StringList.cpp (revision 184143)
>>> +++ source/Core/StringList.cpp (working copy)
>>> @@ -230,6 +230,18 @@
>>> }
>>> }
>>>
>>> +// Remove any duplicates from the list of strings, side effect it will sort the list
>>> +void
>>> +StringList::RemoveDupsAndSort ()
>>> +{
>>> + if (GetSize() < 2)
>>> + return;
>>> +
>>> + // sort the list, remove the dups
>>> + std::sort(m_strings.begin(), m_strings.end());
>>> + m_strings.erase(std::unique(m_strings.begin(), m_strings.end()), m_strings.end());
>>> +}
>>> +
>>> std::string
>>> StringList::CopyList(const char* item_preamble,
>>> const char* items_sep)
>>> Index: source/Interpreter/CommandInterpreter.cpp
>>> ===================================================================
>>> --- source/Interpreter/CommandInterpreter.cpp (revision 184143)
>>> +++ source/Interpreter/CommandInterpreter.cpp (working copy)
>>> @@ -1889,6 +1889,10 @@
>>> {
>>> // The cursor is in the first argument, so just do a lookup in the dictionary.
>>> CommandObject *cmd_obj = GetCommandObject (parsed_line.GetArgumentAtIndex(0), &matches);
>>> +
>>> + // Remove the dups
>>> + matches.RemoveDupsAndSort();
>>> +
>>> num_command_matches = matches.GetSize();
>>>
>>> if (num_command_matches == 1
>>>
>>>
>>>
>>> On Mon, Jun 17, 2013 at 11:03 AM, Kopec, Matt <matt.kopec at intel.com> wrote:
>>> My mistake, this was fixed in r183932 and I wasn't updated. However, the tests weren't updated to reflect this fix. I'll commit a fix for that.
>>>
>>> Matt
>>>
>>> On 2013-06-14, at 5:20 PM, Matthew Sorrels <sorrels.m at gmail.com> wrote:
>>>
>>>> If I fix the test so it has the space, I can reproduce your test failure with my patch. There were a lot of ways to fix this double printing bug, I just need to use one that doesn't also have a few side effects.
>>>>
>>>> I synced and rebuilt lldb clean and I'm definitely getting a space on that line doing the TestAliases.py test. I wonder if it's some issue of the toolchain (version of Phython, version of clang, etc) I'm using. Here's the -t output on that section:
>>>>
>>>> runCmd: bpi
>>>> output: Current breakpoints:
>>>> 1: name = 'foo', locations = 1
>>>> 1.1: where = a.out`foo(int, int) + 14 at main.cpp:43, address = 0x000000000040070e, unresolved, hit count = 0
>>>>
>>>> 2: name = 'sum', locations = 1
>>>> 2.1: where = a.out`sum(int, int) + 8 at main.cpp:25, address = 0x0000000000400698, unresolved, hit count = 0
>>>>
>>>> 3: file = 'main.cpp', line = 32, locations = 1
>>>> 3.1: where = a.out`strange_max(int, int) + 8 at main.cpp:32, address = 0x00000000004006b8, unresolved, hit count = 0
>>>>
>>>>
>>>> Which fails the test as it's written since there is a space between the = and 'main.cpp' are you saying your lldb output doesn't have a space between the equal sign and 'main.cpp'?
>>>>
>>>>
>>>>
>>>> On Fri, Jun 14, 2013 at 1:58 PM, Kopec, Matt <matt.kopec at intel.com> wrote:
>>>> With my version of lldb, there is no space as you describe. I even checked the output manually in the command line. Strangely, I'm on a very recent lldb too. That will warrant separate investigation. Otherwise, you are running the test correctly. Can you reproduce?
>>>>
>>>> Matt
>>>>
>>>> On 2013-06-14, at 4:33 PM, Matthew Sorrels <sorrels.m at gmail.com>
>>>> wrote:
>>>>
>>>>> I get a failure in TestAliases.py without my change as well. But the error I get is different than what you list (occurs sooner at line 99). It appears to be a bug in the test script itself, it's missing a space. on line 99 between the = and 'main.cpp' Or am I running these tests wrong somehow? If I add the space the test runs (without my diff).
>>>>>
>>>>> I can see why my patch is causing the regression your reporting. I think I'm going to have make the matches list remove dups (rather than just raw append).
>>>>>
>>>>> matthews at matthews-linux:~/work/llvm/llvm/tools/lldb/test$ python dotest.py --executable ~/work/llvm/llvm/build/bin/lldb -q -p TestAliases.py
>>>>> UNSUPPORTED: LLDB (clang-x86_64) :: test_with_dsym (TestAliases.AliasTestCase) (requires Darwin)
>>>>> FAIL: LLDB (clang-x86_64) :: test_with_dwarf (TestAliases.AliasTestCase)
>>>>> ======================================================================
>>>>> FAIL: test_with_dwarf (TestAliases.AliasTestCase)
>>>>> ----------------------------------------------------------------------
>>>>> Traceback (most recent call last):
>>>>> File "/data/work/llvm/llvm/tools/lldb.svn/test/lldbtest.py", line 368, in wrapper
>>>>> return func(self, *args, **kwargs)
>>>>> File "/data/work/llvm/llvm/tools/lldb.svn/test/functionalities/alias/TestAliases.py", line 24, in test_with_dwarf
>>>>> self.alias_tests ()
>>>>> File "/data/work/llvm/llvm/tools/lldb.svn/test/functionalities/alias/TestAliases.py", line 99, in alias_tests
>>>>> "3: file ='main.cpp', line = 32, locations = 1" ])
>>>>> File "/data/work/llvm/llvm/tools/lldb.svn/test/lldbtest.py", line 1720, in expect
>>>>> msg if msg else EXP_MSG(str, exe))
>>>>> AssertionError: False is not True : '3: file ='main.cpp', line = 32, locations = 1' returns expected result
>>>>> Config=x86_64-clang
>>>>> ----------------------------------------------------------------------
>>>>> Ran 2 tests in 0.443s
>>>>>
>>>>> FAILED (failures=1, skipped=1)
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> On Fri, Jun 14, 2013 at 12:15 PM, Kopec, Matt <matt.kopec at intel.com> wrote:
>>>>> Hi Matthew,
>>>>>
>>>>> I ran this patch against the lldb test suite and a regression came up. Here is how to quickly run the failing test and the corresponding output:
>>>>>
>>>>> mkopec1 at mkopec1-linux:~/dev/llvm/tools/lldb/test$ python dotest.py --executable /home/mkopec1/dev/llvm/build/Debug+Asserts/bin/lldb -q -p TestAliases.py
>>>>> UNSUPPORTED: LLDB (clang-x86_64) :: test_with_dsym (TestAliases.AliasTestCase) (requires Darwin)
>>>>> FAIL: LLDB (clang-x86_64) :: test_with_dwarf (TestAliases.AliasTestCase)
>>>>> ======================================================================
>>>>> FAIL: test_with_dwarf (TestAliases.AliasTestCase)
>>>>> ----------------------------------------------------------------------
>>>>> Traceback (most recent call last):
>>>>> File "/home/mkopec1/dev/llvm/tools/lldb/test/lldbtest.py", line 368, in wrapper
>>>>> return func(self, *args, **kwargs)
>>>>> File "/home/mkopec1/dev/llvm/tools/lldb/test/functionalities/alias/TestAliases.py", line 24, in test_with_dwarf
>>>>> self.alias_tests ()
>>>>> File "/home/mkopec1/dev/llvm/tools/lldb/test/functionalities/alias/TestAliases.py", line 139, in alias_tests
>>>>> self.runCmd ("alias exprf2 expr --raw -f %1 --")
>>>>> File "/home/mkopec1/dev/llvm/tools/lldb/test/lldbtest.py", line 1582, in runCmd
>>>>> msg if msg else CMD_MSG(cmd))
>>>>> AssertionError: False is not True : Command 'alias exprf2 expr --raw -f %1 --' returns successfully
>>>>> Config=x86_64-clang
>>>>> ----------------------------------------------------------------------
>>>>> Ran 2 tests in 0.633s
>>>>>
>>>>> FAILED (failures=1, skipped=1)
>>>>>
>>>>> Matt
>>>>>
>>>>> On 2013-06-07, at 5:24 PM, Matthew Sorrels <sorrels.m at gmail.com> wrote:
>>>>>
>>>>> > When doing command completion with multiple command matches you will see the commands twice. The best example of this bug is typing pl<tab>
>>>>> >
>>>>> > (lldb) pl
>>>>> > Available completions:
>>>>> > platform
>>>>> > plugin
>>>>> > platform
>>>>> > plugin
>>>>> >
>>>>> >
>>>>> > with this patch you get the expected result:
>>>>> >
>>>>> > (lldb) pl
>>>>> > Available completions:
>>>>> > platform
>>>>> > plugin
>>>>> >
>>>>> >
>>>>> > The fix works because once an exact match for a command and a command+aliases fails it just asks for any inexact matches from both commands and aliases rather than asking for any inexact commands (which returns a list of matches) and then any inexact commands and aliases (which adds the commands to the list again plus any aliases).
>>>>> >
>>>>> > If this seems reasonable, please commit it for me. Thanks.
>>>>> >
>>>>> >
>>>>> > Index: source/Interpreter/CommandInterpreter.cpp
>>>>> > ===================================================================
>>>>> > --- source/Interpreter/CommandInterpreter.cpp (revision 183564)
>>>>> > +++ source/Interpreter/CommandInterpreter.cpp (working copy)
>>>>> > @@ -848,13 +848,7 @@
>>>>> > command_obj = GetCommandSP (cmd_cstr, true, true, matches).get();
>>>>> > }
>>>>> >
>>>>> > - // If there wasn't an exact match among the aliases, look for an inexact match
>>>>> > - // in just the commands.
>>>>> > -
>>>>> > - if (command_obj == NULL)
>>>>> > - command_obj = GetCommandSP(cmd_cstr, false, false, matches).get();
>>>>> > -
>>>>> > - // Finally, if there wasn't an inexact match among the commands, look for an inexact
>>>>> > + // If there wasn't an exact match among the commands and aliases, look for an inexact
>>>>> > // match in both the commands and aliases.
>>>>> > if (command_obj == NULL)
>>>>> > command_obj = GetCommandSP(cmd_cstr, true, false, matches).get();
>>>>> >
>>>>> > _______________________________________________
>>>>> > 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
>>
>> _______________________________________________
>> 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/20130618/9a655a63/attachment.html>
More information about the lldb-commits
mailing list