<div dir="ltr"><div>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.<br>
<br></div><div>I'll take a look at this tomorrow when I'm back in the office.<br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Jun 17, 2013 at 5:23 PM, Enrico Granata <span dir="ltr"><<a href="mailto:egranata@apple.com" target="_blank">egranata@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div>
<div style="text-indent:0px;letter-spacing:normal;text-align:start;text-transform:none;white-space:normal;word-wrap:break-word;word-spacing:0px"><div style="text-indent:0px;letter-spacing:normal;text-align:start;text-transform:none;white-space:normal;word-wrap:break-word;word-spacing:0px">
<div style="border-collapse:separate;border-spacing:0px"></div></div></div></div></div>
<br><div style="word-wrap:break-word"><div><div style="text-indent:0px;letter-spacing:normal;text-align:start;text-transform:none;white-space:normal;word-wrap:break-word;word-spacing:0px"><div style="text-indent:0px;letter-spacing:normal;text-align:start;text-transform:none;white-space:normal;word-wrap:break-word;word-spacing:0px">
<div style="border-collapse:separate;border-spacing:0px"></div><div style="border-collapse:separate;border-spacing:0px"><br></div><div style="border-collapse:separate;border-spacing:0px">Please find my attempt at a patch for this bug included</div>
<div style="border-collapse:separate;border-spacing:0px">It essentially follows the above logic in GetCommandObject()</div><div style="border-collapse:separate;border-spacing:0px">I have tested completions and they work as expected:</div>
<div style="border-collapse:separate;border-spacing:0px"><div style="margin:0px;font-family:'Andale Mono';color:rgb(41,249,20);background-color:rgb(0,0,0)">./lldb</div><div style="margin:0px;font-family:'Andale Mono';color:rgb(41,249,20);background-color:rgb(0,0,0)">
(lldb) pl<TAB></div><div style="margin:0px;font-family:'Andale Mono';color:rgb(41,249,20);background-color:rgb(0,0,0)">Available completions:</div><div style="margin:0px;font-family:'Andale Mono';color:rgb(41,249,20);background-color:rgb(0,0,0)">
<span style="white-space:pre-wrap"> </span>platform</div><div style="margin:0px;font-family:'Andale Mono';color:rgb(41,249,20);background-color:rgb(0,0,0)"><span style="white-space:pre-wrap"> </span>plugin</div><div style="margin:0px;font-family:'Andale Mono';color:rgb(41,249,20);background-color:rgb(0,0,0)">
(lldb) pl</div><div><br></div><div>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.</div><div><br></div>
<div><span>Enri</span><span>co Gran</span><span>ata</span></div></div><div style="border-collapse:separate;border-spacing:0px"><span style="font-size:12px">📩 egranata@</span><font style="font-size:12px" color="#ff2600"></font><span style="font-size:12px">.com</span><br style="font-size:12px">
<span style="font-size:12px">☎️ 27683</span></div></div></div>
</div>
<br><div><div>On Jun 17, 2013, at 4:45 PM, Enrico Granata <<a href="mailto:egranata@apple.com" target="_blank">egranata@apple.com</a>> wrote:</div><br><blockquote type="cite"><div style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;word-wrap:break-word">
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)<div>
I am thinking that a good solution might be:</div><div> - try to get an exact match and do not fill the list</div><div>- if you get an exact match, then add that command to the list and bail out</div><div>- if you did not get an exact match, only then try to get inexact matches into the StringList</div>
<div><br></div><div>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.<br>
<div><br><div><div style="letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;word-wrap:break-word"><div style="letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;word-wrap:break-word">
<div style="border-collapse:separate;border-spacing:0px"><span style="font-size:12px">Enrico Granata</span><br style="font-size:12px"><span style="font-size:12px">📩 egranata@</span><font style="font-size:12px" color="#ff2600"></font><span style="font-size:12px">.com</span><br style="font-size:12px">
<span style="font-size:12px">☎️ 27683</span></div></div></div></div><br><div><div>On Jun 17, 2013, at 4:32 PM, Matthew Sorrels <<a href="mailto:sorrels.m@gmail.com" target="_blank">sorrels.m@gmail.com</a>> wrote:</div>
<br><blockquote type="cite"><div style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
<div dir="ltr"><div>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.<br>
<br></div>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,<br>
<br>Index: include/lldb/Core/StringList.h<br>===================================================================<br>--- include/lldb/Core/StringList.h (revision 184143)<br>+++ include/lldb/Core/StringList.h (working copy)<br>
@@ -72,6 +72,9 @@<br> void<br> RemoveBlankLines ();<br> <br>+ void<br>+ RemoveDupsAndSort ();<br>+<br> size_t<br> SplitIntoLines (const char *lines, size_t len);<br> <span> </span><br>Index: source/Core/StringList.cpp<br>
===================================================================<br>--- source/Core/StringList.cpp (revision 184143)<br>+++ source/Core/StringList.cpp (working copy)<br>@@ -230,6 +230,18 @@<br> }<br> }<br> <br>
+// Remove any duplicates from the list of strings, side effect it will sort the list<br>+void<br>+StringList::RemoveDupsAndSort ()<br>+{<br>+ if (GetSize() < 2)<br>+ return;<br>+<br>+ // sort the list, remove the dups<br>
+ std::sort(m_strings.begin(), m_strings.end());<br>+ m_strings.erase(std::unique(m_strings.begin(), m_strings.end()), m_strings.end());<br>+}<br>+<br> std::string<br> StringList::CopyList(const char* item_preamble,<br>
const char* items_sep)<br>Index: source/Interpreter/CommandInterpreter.cpp<br>===================================================================<br>--- source/Interpreter/CommandInterpreter.cpp (revision 184143)<br>
+++ source/Interpreter/CommandInterpreter.cpp (working copy)<br>@@ -1889,6 +1889,10 @@<br> {<br> // The cursor is in the first argument, so just do a lookup in the dictionary.<br> CommandObject *cmd_obj = GetCommandObject (parsed_line.GetArgumentAtIndex(0), &matches);<br>
+<br>+ // Remove the dups<br>+ matches.RemoveDupsAndSort();<br>+ <span> </span><br> num_command_matches = matches.GetSize();<br> <br> if (num_command_matches == 1<br><br></div><div class="gmail_extra">
<br><br><div class="gmail_quote">On Mon, Jun 17, 2013 at 11:03 AM, Kopec, Matt<span> </span><span dir="ltr"><<a href="mailto:matt.kopec@intel.com" target="_blank">matt.kopec@intel.com</a>></span><span> </span>wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><div>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.</div>
<span><font color="#888888"><div><br></div><div>Matt</div></font></span><div><div><div><br></div><div><div><div>On 2013-06-14, at 5:20 PM, Matthew Sorrels <<a href="mailto:sorrels.m@gmail.com" target="_blank">sorrels.m@gmail.com</a>> wrote:</div>
<br><blockquote type="cite"><div dir="ltr"><div><div>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.<br>
<br></div>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:<br>
<br>runCmd: bpi<br>output: Current breakpoints:<br>1: name = 'foo', locations = 1<br> 1.1: where = a.out`foo(int, int) + 14 at main.cpp:43, address = 0x000000000040070e, unresolved, hit count = 0<span> </span><br>
<br>2: name = 'sum', locations = 1<br> 2.1: where = a.out`sum(int, int) + 8 at main.cpp:25, address = 0x0000000000400698, unresolved, hit count = 0<span> </span><br><br>3: file = 'main.cpp', line = 32, locations = 1<br>
3.1: where = a.out`strange_max(int, int) + 8 at main.cpp:32, address = 0x00000000004006b8, unresolved, hit count = 0<span> </span><br><br><br></div>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'?<br>
<br></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Jun 14, 2013 at 1:58 PM, Kopec, Matt<span> </span><span dir="ltr"><<a href="mailto:matt.kopec@intel.com" target="_blank">matt.kopec@intel.com</a>></span><span> </span>wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word">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?<div>
<br></div><div>Matt</div><div><br><div><div>On 2013-06-14, at 4:33 PM, Matthew Sorrels <<a href="mailto:sorrels.m@gmail.com" target="_blank">sorrels.m@gmail.com</a>></div><div><div> wrote:</div><br><blockquote type="cite">
<div dir="ltr"><div>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).<br>
<br></div>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).<br><div><br>matthews@matthews-linux:~/work/llvm/llvm/tools/lldb/test$ python dotest.py --executable ~/work/llvm/llvm/build/bin/lldb -q -p TestAliases.py<br>
UNSUPPORTED: LLDB (clang-x86_64) :: test_with_dsym (TestAliases.AliasTestCase) (requires Darwin)<span> </span><br>FAIL: LLDB (clang-x86_64) :: test_with_dwarf (TestAliases.AliasTestCase)<br>======================================================================<br>
FAIL: test_with_dwarf (TestAliases.AliasTestCase)<br>----------------------------------------------------------------------<br>Traceback (most recent call last):<br> File "/data/work/llvm/llvm/tools/lldb.svn/test/lldbtest.py", line 368, in wrapper<br>
return func(self, *args, **kwargs)<br> File "/data/work/llvm/llvm/tools/lldb.svn/test/functionalities/alias/TestAliases.py", line 24, in test_with_dwarf<br> self.alias_tests ()<br> File "/data/work/llvm/llvm/tools/lldb.svn/test/functionalities/alias/TestAliases.py", line 99, in alias_tests<br>
"3: file ='main.cpp', line = 32, locations = 1" ])<br> File "/data/work/llvm/llvm/tools/lldb.svn/test/lldbtest.py", line 1720, in expect<br> msg if msg else EXP_MSG(str, exe))<br>AssertionError: False is not True : '3: file ='main.cpp', line = 32, locations = 1' returns expected result<br>
Config=x86_64-clang<br>----------------------------------------------------------------------<br>Ran 2 tests in 0.443s<br><br>FAILED (failures=1, skipped=1)<br><br><br><div class="gmail_extra"><br><br><div class="gmail_quote">
On Fri, Jun 14, 2013 at 12:15 PM, Kopec, Matt<span> </span><span dir="ltr"><<a href="mailto:matt.kopec@intel.com" target="_blank">matt.kopec@intel.com</a>></span><span> </span>wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Hi Matthew,<br><br>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:<br><br>mkopec1@mkopec1-linux:~/dev/llvm/tools/lldb/test$ python dotest.py --executable /home/mkopec1/dev/llvm/build/Debug+Asserts/bin/lldb -q -p TestAliases.py<br>
UNSUPPORTED: LLDB (clang-x86_64) :: test_with_dsym (TestAliases.AliasTestCase) (requires Darwin)<br>FAIL: LLDB (clang-x86_64) :: test_with_dwarf (TestAliases.AliasTestCase)<br>======================================================================<br>
FAIL: test_with_dwarf (TestAliases.AliasTestCase)<br>----------------------------------------------------------------------<br>Traceback (most recent call last):<br> File "/home/mkopec1/dev/llvm/tools/lldb/test/lldbtest.py", line 368, in wrapper<br>
return func(self, *args, **kwargs)<br> File "/home/mkopec1/dev/llvm/tools/lldb/test/functionalities/alias/TestAliases.py", line 24, in test_with_dwarf<br> self.alias_tests ()<br> File "/home/mkopec1/dev/llvm/tools/lldb/test/functionalities/alias/TestAliases.py", line 139, in alias_tests<br>
self.runCmd ("alias exprf2 expr --raw -f %1 --")<br> File "/home/mkopec1/dev/llvm/tools/lldb/test/lldbtest.py", line 1582, in runCmd<br> msg if msg else CMD_MSG(cmd))<br>AssertionError: False is not True : Command 'alias exprf2 expr --raw -f %1 --' returns successfully<br>
Config=x86_64-clang<br>----------------------------------------------------------------------<br>Ran 2 tests in 0.633s<br><br>FAILED (failures=1, skipped=1)<br><br>Matt<br><div><br>On 2013-06-07, at 5:24 PM, Matthew Sorrels <<a href="mailto:sorrels.m@gmail.com" target="_blank">sorrels.m@gmail.com</a>> wrote:<br>
<br>> When doing command completion with multiple command matches you will see the commands twice. The best example of this bug is typing pl<tab><br>><br>> (lldb) pl<br>> Available completions:<br>> platform<br>
> plugin<br>> platform<br>> plugin<br>><br>><br>> with this patch you get the expected result:<br>><br>> (lldb) pl<br>> Available completions:<br>> platform<br>> plugin<br>
><br>><br>> 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).<br>
><br>> If this seems reasonable, please commit it for me. Thanks.<br>><br>><br>> Index: source/Interpreter/CommandInterpreter.cpp<br>> ===================================================================<br>
> --- source/Interpreter/CommandInterpreter.cpp (revision 183564)<br>> +++ source/Interpreter/CommandInterpreter.cpp (working copy)<br>> @@ -848,13 +848,7 @@<br>> command_obj = GetCommandSP (cmd_cstr, true, true, matches).get();<br>
> }<br>><br>> - // If there wasn't an exact match among the aliases, look for an inexact match<br>> - // in just the commands.<br>> -<br>> - if (command_obj == NULL)<br>> - command_obj = GetCommandSP(cmd_cstr, false, false, matches).get();<br>
> -<br>> - // Finally, if there wasn't an inexact match among the commands, look for an inexact<br>> + // If there wasn't an exact match among the commands and aliases, look for an inexact<br>> // match in both the commands and aliases.<br>
> if (command_obj == NULL)<br>> command_obj = GetCommandSP(cmd_cstr, true, false, matches).get();<br>><br></div>> _______________________________________________<br>> lldb-commits mailing list<br>
><span> </span><a href="mailto:lldb-commits@cs.uiuc.edu" target="_blank">lldb-commits@cs.uiuc.edu</a><br>><span> </span><a href="http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits</a><br>
<br></blockquote></div><br></div></div></div></blockquote></div></div><br></div></div></blockquote></div><br></div></blockquote></div><br></div></div></div></div></blockquote></div><br></div>_______________________________________________<br>
lldb-commits mailing list<br><a href="mailto:lldb-commits@cs.uiuc.edu" target="_blank">lldb-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits</a></div>
</blockquote></div><br></div></div>_______________________________________________<br>lldb-commits mailing list<br><a href="mailto:lldb-commits@cs.uiuc.edu" target="_blank">lldb-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits</a></div>
</blockquote></div><br></div><br></blockquote></div><br></div>