Page MenuHomePhabricator

Fix abbreviation test on Windows
ClosedPublic

Authored by amccarth on Apr 15 2015, 11:24 AM.

Details

Summary
  1. Demangler was including function return types, which foiled the pattern matching tests.
  1. Demangler was not putting spaces after commas (e.g., in parameter lists), which also foiled the pattern matching.
  1. 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).
  1. 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.

Diff Detail

Event Timeline

amccarth updated this revision to Diff 23786.Apr 15 2015, 11:24 AM
amccarth retitled this revision from to Fix abbreviation test on Windows.
amccarth updated this object.
amccarth edited the test plan for this revision. (Show Details)
amccarth added a reviewer: zturner.
amccarth added a subscriber: Unknown Object (MLST).
zturner added inline comments.Apr 15 2015, 12:00 PM
source/Core/Mangled.cpp
5283 ↗(On Diff #23786)

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
53–54

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 ↗(On Diff #23786)

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?

+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 added inline comments.Apr 15 2015, 12:44 PM
source/Core/Mangled.cpp
5283 ↗(On Diff #23786)

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
53–54

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 ↗(On Diff #23786)

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

zturner added inline comments.Apr 15 2015, 1:10 PM
source/Core/Mangled.cpp
5283 ↗(On Diff #23786)

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
53–54

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 edited edge metadata.Apr 15 2015, 1:44 PM

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'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.

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 edited edge metadata.Apr 15 2015, 3:21 PM

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.

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"?

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...

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.

Sounds good to me.

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 updated this revision to Diff 24258.Apr 22 2015, 2:39 PM
amccarth edited edge metadata.

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.

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

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

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
1273

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 requested changes to this revision.Apr 22 2015, 3:27 PM
clayborg edited edge metadata.

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

This revision now requires changes to proceed.Apr 22 2015, 3:27 PM
amccarth added inline comments.Apr 23 2015, 9:53 AM
include/lldb/API/SBCommandInterpreter.h
233

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

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
1273

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 updated this revision to Diff 24309.Apr 23 2015, 9:54 AM
amccarth edited edge metadata.

Addresses earlier comments.

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

clayborg accepted this revision.Apr 23 2015, 10:30 AM
clayborg edited edge metadata.

Looks fine.

This revision is now accepted and ready to land.Apr 23 2015, 10:30 AM