[Lldb-commits] Fix for double command completion text
Matthew Sorrels
sorrels.m at gmail.com
Mon Jun 17 21:44:04 PDT 2013
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/20130617/17680e94/attachment.html>
More information about the lldb-commits
mailing list