Fix abbreviation test on Windows

[amccarth]

Summary

1. Demangler was including function return types, which foiled the pattern matching tests.

2. Demangler was not putting spaces after commas (e.g., in parameter lists), which also foiled the pattern matching.

3. Checking to see if dis -f does the same thing as disassemble -f was overly complicated and architecture specific (and didn't account for arch = i686).

4. Environment variables aren't always inherited all the way down to always making it down the make commands to build the targets, causing OS to resolve to "" which then defaulted to "windows32" which didn't match the tests for "Windows_NT". While not specific to the abbreviations test, this was important for debugging the Python through Visual Studio.

[zturner]

source/Core/Mangled.cpp

5283

I kind of want to block this. I know it's the most straightforward solution, but I don't think it's safe to make assumptions about the format of a mangled name. Even the __cxa_demangle function that is used on other platforms that have it, there is no guarantee that a particualr implementation of __cxa_demangle will separate arguments with spaces. Plus, this would mean changing an actual implementation to satisfy the peculiarities of a particular test.

I think we should consider any test that uses a regex to match a demangled name to be broken. A test should never regex match a demangled name. If it's trying to extract the function name or argument list, we should use SBFunction or something like that. We can probably extend the API wherever necessary to introduce the things we need. Or alternatively, it's possible the test is regex matching more than it actually needs to verify that the test was successful. You might see if there's a way to reduce (or even better, eliminate) the regex and change the code to use other SB apis to verify that the actual state of the thing is as expected.

test/functionalities/abbreviation/TestAbbreviations.py

151–158

I'm starting to think this *entire function* could just be re-written. If the point of this test is to test abbreviations, then all it should be doing is checking that running the abbreviation does the same thing as running the full command. For example, do we actually care that "dis -f" begins with the word "disassembly"? Or do we just care that "dis -f" does the same thing as "disassemble -f"? We (should) already have tests that verify that disassemble -f does the correct thing, so as long as dis -f does whatever disassemble -f does, we should be fine.

The same goes for all the tests in this file. I think maybe every one of these checks should be converted to the following pattern:

1. Run a sequence of non aliased command
2. save the output
3. Run the same sequence of aliased commands
4. Verify that the output matches that from step 2.

That's it. If you want to check that the command behaves *correctly*, leave that for a test that understands the command better.

test/make/Makefile.rules

40–45

Can you investigate this a little more? Can you dump your environment variables from inside of a Visual Studio instance (using the embedded PowerShell Console) to see if the OS environment variable is set?

[zturner]

+Jim and Greg for their thoughts, especially on the first two points (regex matching a demangled name being fundamentally broken, and re-writing TestAbbreviations to just compare output of aliased and non-aliased commands.

[amccarth]

source/Core/Mangled.cpp

5283

I mostly agree with you, but I convinced myself it was a good idea to make the demangled names on Windows be as consistent as possible with demangled names on any other platform, regardless of how we test it.

test/functionalities/abbreviation/TestAbbreviations.py

151–158

I considered that approach, but it's a tad more complicated with most of the other commands because they change the state of the target process. You'd have to do a pass of all the commands without abbreviating them and then a pass with abbreviations. This has the disadvantage of moving the reference for any particular command far away from the actual test, and you'd have a bunch of housekeeping to keep track of all the outputs. Either that or test each abbreviation as a separate run, which would be a lot slower.

test/make/Makefile.rules

40–45

I already verified that the Python test saw the variable, but make did not.

[zturner]

source/Core/Mangled.cpp

5283

I actually wonder if they should be as different as possible just so that we can find bugs like this more easily. :) It seems kind of similar to saying "we should make our undefined behavior act like their undefined behavior". I definitely see the advantages of doing that, but since it's undefined (or implementation-defined, more specifically), we have no guarantee that someone else won't come along again later with a perfectly valid implementation of __cxa_demangle (or a hand-written demangler) and this will all break again.

test/functionalities/abbreviation/TestAbbreviations.py

151–158

Yes, that's certainly a problem.

Jim / Greg, can we add a flag to "command alias" like "command alias -s <alias>" that returns the thing which it is aliased to? For example:

(lldb) command alias -s sc
script
(lldb)

Then this whole file just becomes a test that "command alias -s <alias>" returns the correct unaliased command. We have other tests for verifying that functionality actually works.

If we want to verify that you can run a command through an alias, then we can get by with a one liner that only tests a single trivial command that doesn't modify any debugger state. Like sc print "Hello" and just verify that you see Hello in the command output. And all the rest of the tests can simply verify that the alias is set up correctly.

I think this should be sufficient test coverage and less flaky than the current scenario.

[clayborg]

We could skip adding the "command alias -s" and just use "help <alias>":

(lldb) help run
....
'run' is an abbreviation for 'process launch -X true --'

We could just check the output of "help" instead?

[amccarth]

I'm happy to rewrite the test using the help trick.

I'm also happy to back off on the spaces-after-commas solution, though it would help in resolving other test failures on Windows.

[zturner]

In D9033#156691, @clayborg wrote:

We could skip adding the "command alias -s" and just use "help <alias>":

(lldb) help run
....
'run' is an abbreviation for 'process launch -X true --'

We could just check the output of "help" instead?

I just noticed we also have test/functionalities/alias/TestAliases.py.

Now I'm wondering if we can simply delete this whole file as it's not needed and can just be covered by TestAliases.

Regarding the help output, that would work (and be better than what we're currently doing). However we had a lengthy discussion earlier, and I think at least on this side we all feel like things would be better off if tests were matching strings less frequently. It's kind of unfortunate being in a situation where you don't ever want to change the way a command prints its output because it will mean fixing a bunch of tests. If this were a GUI application, it would be like testing by simulating mouse clicks vs. just invoking the functions behind the mouse clicks.

So with that in mind, wherever possible it would be nice if we could stop using regexes in the test suite and instead using the SB APIs to check the actual state of the thing we care about. One way to do this is to add a function to SBCommandInterpreter called ResolveAlias. We already have similar methods there, like AliasExists, CommandExists, but nothing to actually do this conversion. If we had this, all of these tests just become something like self.expect("script" == SBCommandInterpreter.ResolveAlias("sc"))

If you don't like this though, checking the output of help would be the next best thing.

[jingham]

Seems to me getting a "resolves to" method would be a fine addition to the SBCommandInterpreter. You could use this to do tooltips for the current command, for instance. At present the command alias mechanism is a little picky and requires you to spell out command and option identifiers fully when making the alias. So a straight-up compare would work as Zachary suggests. If we ever relax this the alias mechanism should canonicalize the alias when it makes it to avoid collisions with commands added later on. But so long as the tests keep spelling out the names in full, this shouldn't be a problem.

[zturner]

In D9033#156737, @jingham wrote:

Seems to me getting a "resolves to" method would be a fine addition to the SBCommandInterpreter. You could use this to do tooltips for the current command, for instance. At present the command alias mechanism is a little picky and requires you to spell out command and option identifiers fully when making the alias. So a straight-up compare would work as Zachary suggests. If we ever relax this the alias mechanism should canonicalize the alias when it makes it to avoid collisions with commands added later on. But so long as the tests keep spelling out the names in full, this shouldn't be a problem.

Regarding TestAliases vs TestAbbreviations. Do you see a reason why we need both of these or can we delete one? One seems to be testing that aliases resolve to the correct thing (which it seems like we agree can be better done by adding a ResolveAlias() to SBCommandInterpreter), and the other seems to be testing that running alias commands produces the same output as running unaliased commands. So can we just change the former to use ResolveAlias() everywhere, and outright delete the latter? (or change the latter to test only 1 trivial command like sc print "Hello"?

[clayborg]

I like the SBCommandInterpreter::ResolveAlias(...) approach. The question is should it expand the current command you passed in and return the expanded form? Or just find the alias and return the template. I would say it should resolve all arguments and place them where you want them to go and return the actual command that would be executed.

I agree we should be adding SB API for anything that might be depending on regex and string scraping...

[zturner]

Cool, sounds like we're all in agreement!

Adrian, maybe you can break this up and commit just the make / $(OS) portion of this change now, and then work on the ResolveAlias change separately? I'd rather not commit the demangling change at all. It's probably more work, but I think everyone will benefit if we reduce the amount of regex scraping the test suite uses and add methods where necessary to the public API if it isn't currently sufficient to find out what we need.

[amccarth]

Sounds good to me.

[jingham]

In D9033#156738, @zturner wrote:

Regarding TestAliases vs TestAbbreviations. Do you see a reason why we need both of these or can we delete one? One seems to be testing that aliases resolve to the correct thing (which it seems like we agree can be better done by adding a ResolveAlias() to SBCommandInterpreter), and the other seems to be testing that running alias commands produces the same output as running unaliased commands. So can we just change the former to use ResolveAlias() everywhere, and outright delete the latter? (or change the latter to test only 1 trivial command like sc print "Hello"?

Testing the aliases just tests that the string with substitutions got properly registered and reported. But that doesn't tell you whether the %1, %2, etc substitution mechanism is still working properly when the alias is run. So we should keep some tests that test that. You want to make sure that the % substitutions work for option values and arguments, that they work when the options are supplied out of order, and a few other things like that.

[amccarth]

Per our earlier conversation ...

1. I factored the command resolution phase out of CommandInterpreter::HandleCommand and exposed it through SBCommandInterpreter.
2. I changed the abbreviations test to use that API to directly test resolution of both abbreviations and aliases.

If we proceed with this, I would then:

1. Follow up with a patch to eliminate the old alias test, which would become obsolete.
2. Remove the test app from lldb/test/functionalities/abbrevation. (With my change, it's no longer built, but lldbtest.py seems to still want to issue make clean commands.)
3. Find a new home for testing the dynamic prompt (which was being tested as part of abbreviations).

I need guidance to finish this patch up.

1. I'm not fond of using the result type to stash the resolved command text, but I didn't see another way to return a dynamically generated string, nor could I find any precedent for doing so.
2. There may be other things I'm doing that go against the grain of lldb design that I'm not yet aware of.

On the plus side, I believe this eliminates most--if not all--of the platform issues: one test for Darwin and Dwarf, no architecture problems on Windows.

[zturner]

I like the way the tests are written now. It's much more obvious that the conditions being verified correspond to making sure the aliases resolve correctly.

include/lldb/API/SBCommandInterpreter.h

233 ↗

(On Diff #24258)

No space between function name and argument list (existing code that does this can be fixed over time as it is touched)

include/lldb/Interpreter/CommandInterpreter.h

632–633 ↗

(On Diff #24258)

Any reason for the line break here? Also no space between function name and argument list again. (I won't mention this again, even though it occurs in other places as well).

source/Interpreter/CommandInterpreter.cpp

1274 ↗

(On Diff #24258)

If you make an llvm::StringRef out of this, you can use StringRef::ltrim which actually finds even more types of whitespace characters. Seems better than duplicating this whitespace-finding logic.

[clayborg]

Fix the things that Zachary pointed out and we are good to go.

[amccarth]

include/lldb/API/SBCommandInterpreter.h

233 ↗

(On Diff #24258)

Done. I was just trying to be consistent with the code in this file. I'll nix the extra spaces throughout my change.

include/lldb/Interpreter/CommandInterpreter.h

632–633 ↗

(On Diff #24258)

Line break was to keep the line below 80 columns, which, as it turns out, is a hard habit to break. Fixed.

source/Interpreter/CommandInterpreter.cpp

1274 ↗

(On Diff #24258)

But it's not actually trimming the string; it's finding the range of the first whitespace-delimited token. Look below where it find the end of the token.

This isn't really part of this refactor. The only reason I even touched this code was to reduce the repetition of yet another white_space string, given that this translation unit already had one. (My change originally needed more white space scanning, but it doesn't currently.)

[amccarth]

Addresses earlier comments.

[zturner]

Looks good as long as Greg and Jim are ok with it.

[clayborg]

Looks fine.

[amccarth]

http://reviews.llvm.org/rL235633

source/Core/Mangled.cpp

 //===-- Mangled.cpp ---------------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 
+#include <algorithm>
 // FreeBSD9-STABLE requires this to know about size_t in cxxabi.h
 #include <cstddef>
 #if defined(_MSC_VER)
 #include "lldb/Host/windows/windows.h"
 #include <Dbghelp.h>
 #elif defined (__FreeBSD__)
 #define LLDB_USE_BUILTIN_DEMANGLER
 #else

 #include "lldb/Target/CPPLanguageRuntime.h"
 #include <ctype.h>
 #include <string.h>
 #include <stdlib.h>
 
 
 using namespace lldb_private;
 
+#if defined(_MSC_VER)
+static void
+insert_spaces_after_commas (char *s, std::size_t max)
+{
+    assert(max > 0);
+    const std::size_t length = strlen(s);
+    const int comma_count = std::count(s, s + length, ',');
+    if (comma_count > 0 && length + comma_count < max - 1)
+    {
+        // Start at the end and work backwards until the target location catches
+        // up to the source location.  (We already know that everything from the
+        // beginning of the string to that point is identical.)
+        const char *source = s + length;
+        char *target = s + length + comma_count;
+        while (target > source)
+        {
+            if (*source == ',')
+            {
+                *target-- = ' ';
+            }
+            *target-- = *source--;
+        }
+    }
+}
+#endif
+
 static inline bool
 cstring_is_mangled (const char *s)
 {
     if (s)
 #if defined(_MSC_VER)
         return (s[0] == '?');
 #else
         return (s[0] == '_' && s[1] == 'Z');
#ifdef LLDB_USE_BUILTIN_DEMANGLER
                 // Try to use the fast-path demangler first for the
                 // performance win, falling back to the full demangler only
                 // when necessary
                 char *demangled_name = FastDemangle (mangled_cstr,
                                                      m_mangled.GetLength());
                 if (!demangled_name)
                     demangled_name = __cxa_demangle (mangled_cstr, NULL, NULL, NULL);
 #elif defined(_MSC_VER)
-                char *demangled_name = (char *)::malloc(1024);
-                ::ZeroMemory(demangled_name, 1024);
-                DWORD result = ::UnDecorateSymbolName(mangled_cstr, demangled_name, 1023,
+                const std::size_t demangled_buffer_size = 1024;
+                char *demangled_name = (char *)::malloc(demangled_buffer_size);
+                ::ZeroMemory(demangled_name, demangled_buffer_size);
+                DWORD result = ::UnDecorateSymbolName(mangled_cstr, demangled_name,
+                                                      demangled_buffer_size - 1,
                                                       UNDNAME_NO_ACCESS_SPECIFIERS |       // Strip public, private, protected keywords
                                                           UNDNAME_NO_ALLOCATION_LANGUAGE | // Strip __thiscall, __stdcall, etc keywords
+                                                          UNDNAME_NO_FUNCTION_RETURNS |    // Strip function return types
                                                           UNDNAME_NO_THROW_SIGNATURES |    // Strip throw() specifications
                                                           UNDNAME_NO_MEMBER_TYPE |         // Strip virtual, static, etc specifiers
                                                           UNDNAME_NO_MS_KEYWORDS           // Strip all MS extension keywords
                                                       );
                 if (result == 0)
                 {
                     free (demangled_name);
                     demangled_name = nullptr;
                 }
+                else
+                {
+                    // UnDecorateSymbolName doesn't put spaces after commas, but
+                    // the built-in demangler does, so add them for consistency.
+                    // Besides, our tests expect them.
+                    insert_spaces_after_commas(demangled_name, demangled_buffer_size);
+                }
+
 #else
                 char *demangled_name = abi::__cxa_demangle (mangled_cstr, NULL, NULL, NULL);
 #endif
 
                 if (demangled_name)
                 {
                     m_demangled.SetCStringWithMangledCounterpart(demangled_name, m_mangled);
                     free (demangled_name);

test/functionalities/abbreviation/TestAbbreviations.py

def running_abbreviations (self):
         self.expect("pro st",
                     patterns = [ "Process .* stopped",
                                  "thread #1:",
                                  "a.out",
                                  "sum\(a=1238, b=78392\)",
                                  "at main.cpp\:25",
                                  "stop reason = breakpoint 2.1" ])
 
-        # ARCH, if not specified, defaults to x86_64.
-        self.runCmd("dis -f")
+        # Recognizing output as disassembly in an architecture-independent way
+        # is tricky.  Since we're just trying to verify that `dis -f` does the
+        # same things as `disassemble -f`, we'll run both commands and compare
+        # the output.  This doesn't validate anything about the disassembly
+        # itself; there are other tests for that.
+        self.runCmd("disassemble -f")
         disassembly = self.res.GetOutput()
-        if self.getArchitecture() in ["", 'x86_64', 'i386']:
-            # hey! we shouldn't have a software breakpoint in here
-            self.assertFalse("int3" in disassembly)
-            self.expect(disassembly, exe=False,
-                        startstr = "a.out`sum(int, int)",
-                        substrs = [' mov',
-                                   ' addl ',
-                                   'ret'])
-        else:
-            self.fail('unimplemented for arch = "{arch}"'.format(arch=self.getArchitecture()))
+        self.expect("dis -f", startstr = disassembly)
 
         self.expect("i d l main.cpp",
                     patterns = ["Line table for .*main.cpp in `a.out"])
 
         self.expect("i d se",
                     patterns = ["Dumping sections for [0-9]+ modules."])
 
         self.expect("i d symf",

test/make/Makefile.rules

 
 # Uncomment line below for debugging shell commands
 # SHELL = /bin/sh -x
 
 THIS_FILE_DIR := $(shell dirname $(realpath $(lastword $(MAKEFILE_LIST))))/
 LLDB_BASE_DIR := $(THIS_FILE_DIR)../../
 
 #----------------------------------------------------------------------
-# If ARCH is not defined, default to x86_64.
 # If OS is not defined, use 'uname -s' to determine the OS name.
+#
+# uname on Windows gives "windows32", but most environments standardize
+# on "Windows_NT", so we'll make it consistent here.  When running
+# tests from Visual Studio, the environment variable isn't inherited
+# all the way down to the process spawned for make.
+#----------------------------------------------------------------------
+ifeq "$(OS)" ""
+  OS = $(shell uname -s)
+endif
+ifeq "$(OS)" "windows32"
+  OS = Windows_NT
+endif
+
+#----------------------------------------------------------------------
+# If ARCH is not defined, default to x86_64.
 #----------------------------------------------------------------------
 ifeq "$(ARCH)" ""
 ifeq "$(OS)" "Windows_NT"
   ARCH = x86
 else
   ARCH = x86_64
 endif
 endif
 
-ifeq "$(OS)" ""
-  OS = $(shell uname -s)
-endif
-
 #----------------------------------------------------------------------
 # CC defaults to clang.
 #
 # If you change the defaults of CC, be sure to also change it in the file
 # test/plugins/builder_base.py, which provides a Python way to return the
 # value of the make variable CC -- getCompiler().
 #
 # See also these functions: