Page MenuHomePhabricator

Fix argdumper build in cmake (OS X) after r228636
ClosedPublic

Authored by ki.stfu on Feb 10 2015, 10:23 PM.

Details

Summary

This patch fixes the following tests on OS X:

FAIL: test_with_dsym (TestLaunchWithGlob.LaunchWithGlobTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/testuser/build/workspace/LLDB_master_release_OSX/llvm_master/tools/lldb/test/lldbtest.py", line 456, in wrapper
    return func(self, *args, **kwargs)
  File "/Users/testuser/build/workspace/LLDB_master_release_OSX/llvm_master/tools/lldb/test/functionalities/launch_with_glob/TestLaunchWithGlob.py", line 21, in test_with_dsym
    self.do_test ()
  File "/Users/testuser/build/workspace/LLDB_master_release_OSX/llvm_master/tools/lldb/test/functionalities/launch_with_glob/TestLaunchWithGlob.py", line 42, in do_test
    self.runCmd("process launch -G true -w %s -- fi*.tx?" % (os.getcwd()))
  File "/Users/testuser/build/workspace/LLDB_master_release_OSX/llvm_master/tools/lldb/test/lldbtest.py", line 1953, in runCmd
    msg if msg else CMD_MSG(cmd))
AssertionError: False is not True : Command 'process launch -G true -w /Users/testuser/build/workspace/LLDB_master_release_OSX/llvm_master/tools/lldb/test/functionalities/launch_with_glob -- fi*.tx?' returns successfully
Config=x86_64-clang
======================================================================
FAIL: test_with_dwarf (TestLaunchWithGlob.LaunchWithGlobTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/testuser/build/workspace/LLDB_master_release_OSX/llvm_master/tools/lldb/test/lldbtest.py", line 473, in wrapper
    return func(self, *args, **kwargs)
  File "/Users/testuser/build/workspace/LLDB_master_release_OSX/llvm_master/tools/lldb/test/functionalities/launch_with_glob/TestLaunchWithGlob.py", line 28, in test_with_dwarf
    self.do_test ()
  File "/Users/testuser/build/workspace/LLDB_master_release_OSX/llvm_master/tools/lldb/test/functionalities/launch_with_glob/TestLaunchWithGlob.py", line 42, in do_test
    self.runCmd("process launch -G true -w %s -- fi*.tx?" % (os.getcwd()))
  File "/Users/testuser/build/workspace/LLDB_master_release_OSX/llvm_master/tools/lldb/test/lldbtest.py", line 1953, in runCmd
    msg if msg else CMD_MSG(cmd))
AssertionError: False is not True : Command 'process launch -G true -w /Users/testuser/build/workspace/LLDB_master_release_OSX/llvm_master/tools/lldb/test/functionalities/launch_with_glob -- fi*.tx?' returns successfully

Diff Detail

Event Timeline

ki.stfu updated this revision to Diff 19729.Feb 10 2015, 10:23 PM
ki.stfu retitled this revision from to Fix argdumper build in cmake (OS X) after r228636.
ki.stfu updated this object.
ki.stfu edited the test plan for this revision. (Show Details)
ki.stfu added reviewers: clayborg, zturner, epertoso, emaste.
ki.stfu added subscribers: clayborg, zturner, epertoso and 2 others.
ki.stfu updated this object.Feb 10 2015, 10:25 PM
zturner edited edge metadata.Feb 10 2015, 10:27 PM

Sorry for the ignorance, but what is argdumper and why is it Mac only?

+enrico

But why is it only for mac? We shouldn't be adding features that are only
available on specific platforms without good reason.

Tests were marked as "Darwin only" (see test/functionalities/launch_with_glob/TestLaunchWithGlob.py), therefore I have fixed it only for os x.

Should I port it on Windows? or non-windows platforms only?

Hello Enrico,

RE: I assume that the lack of this build logic was causing the test case to fail on your machine. Correct?

Correct. But tests had failed on my side because argdumper not found. I use cmake to build lldb.

Log:

1: test_with_dsym (TestLaunchWithGlob.LaunchWithGlobTestCase) ...
os command: make clean ; make MAKE_DSYM=YES ARCH=x86_64 CC="clang"
with pid: 94168
stdout: rm -f "a.out"  main.o main.d main.d.tmp
rm -f -r "a.out.dSYM"
clang++ -std=c++11 -g -O0 -arch x86_64   -I/Users/IliaK/p/llvm/tools/lldb/test/make/../../include   -c -o main.o main.cpp
clang++  main.o -g -O0 -arch x86_64   -I/Users/IliaK/p/llvm/tools/lldb/test/make/../../include   -o "a.out"
"/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/dsymutil"  -o "a.out.dSYM" "a.out"

stderr:
retcode: 0


runCmd: file /Users/IliaK/p/llvm/tools/lldb/test/functionalities/launch_with_glob/a.out
output: Current executable set to '/Users/IliaK/p/llvm/tools/lldb/test/functionalities/launch_with_glob/a.out' (x86_64).


/Users/IliaK/p/llvm/build_ninja/lib/python2.7/site-packages/lldb
runCmd: process launch -G true -w /Users/IliaK/p/llvm/tools/lldb/test/functionalities/launch_with_glob -- fi*.tx?
runCmd failed!
error: could not find argdumper tool


Command 'process launch -G true -w /Users/IliaK/p/llvm/tools/lldb/test/functionalities/launch_with_glob -- fi*.tx?' failed!

/Users/IliaK/p/llvm/build_ninja/lib/python2.7/site-packages/lldb
runCmd: process launch -G true -w /Users/IliaK/p/llvm/tools/lldb/test/functionalities/launch_with_glob -- fi*.tx?
runCmd failed!
error: could not find argdumper tool


Command 'process launch -G true -w /Users/IliaK/p/llvm/tools/lldb/test/functionalities/launch_with_glob -- fi*.tx?' failed!

/Users/IliaK/p/llvm/build_ninja/lib/python2.7/site-packages/lldb
runCmd: process launch -G true -w /Users/IliaK/p/llvm/tools/lldb/test/functionalities/launch_with_glob -- fi*.tx?
runCmd failed!
error: could not find argdumper tool


Command 'process launch -G true -w /Users/IliaK/p/llvm/tools/lldb/test/functionalities/launch_with_glob -- fi*.tx?' failed!

FAILURE
ki.stfu updated this revision to Diff 19730.Feb 10 2015, 10:57 PM
ki.stfu edited edge metadata.

Fix argdumper build in cmake on all platforms

This was tested only on OS X. Could somebody test it on others?

ki.stfu set the repository for this revision to rL LLVM.
ki.stfu added a subscriber: abidh.

I was objecting to both things actually:

  1. Disabled in the CMake build that Ilia ported
  2. Test disabled on non mac.

We should be enabling tests in all platforms by default, and if it breaks
someone should be fixing the tests.

Also, I know you guys use the Xcode build primarily, but i think for large
changes like this it would be helpful to at least try to fix the CMake
build. Otherwise there's a good chance nobody will notice the change go
through and the feature will not be added to other builds until someday
when it breaks a test.

ki.stfu updated this revision to Diff 19782.Feb 11 2015, 1:56 PM

Enable dwarf test on non-mac platforms

I was objecting to both things actually:

  1. Disabled in the CMake build that Ilia ported
  2. Test disabled on non mac.

    We should be enabling tests in all platforms by default, and if it breaks someone should be fixing the tests.

Done. Now it's ready for submitting?

zturner added inline comments.Feb 11 2015, 2:46 PM
scripts/Python/finishSwigPythonLLDB.py
461–463

I think this won't work on Windows as written? The previous code was using make_symlink_windows() and make_symlink_other_platforms. make_symlink_windows makes a hardlink because os.symlink is not supported on Windows for some reason. So I think make_symlink_argdumper() should do the same. It would be nice if we wrapped this into an lldb_make_symlink(source, target) or something, so we didn't have to worry about this in the future.

ki.stfu planned changes to this revision.Feb 12 2015, 12:11 AM
ki.stfu added inline comments.Feb 12 2015, 6:25 AM
scripts/Python/finishSwigPythonLLDB.py
300

Is it an error? It should be "../../../liblldb%s"?

ki.stfu updated this revision to Diff 19827.Feb 12 2015, 6:45 AM

Refactor scripts/Python/finishSwigPythonLLDB.py

Note: I assumed that it was an error on line #255 (see D7550#122560) therefore I fixed it in Diff 4.

I will test this patch out on Windows.

Actually the original was correct. On Windows our directory structure looks like this:

ninja
--bin
----liblldb.dll
--lib
----site-packages
------lldb
--------lldb_d.pyd

So the lldb_d.pyd needs to be a link to the liblldb.dll. I will test with the correct relative path back in place and let you know.

clayborg accepted this revision.Feb 12 2015, 10:02 AM
clayborg edited edge metadata.

I will stay out of this one since it mostly affects the CMake and Make builds.

This revision is now accepted and ready to land.Feb 12 2015, 10:02 AM

Greg: At some point I would like to switch the Xcode build over to using
the SWIG python scripts. There's no reason to maintain two parallel sets
of scripts that do the same thing (and we can't assume an environment that
will be capable of running shell scripts on Windows). Might not matter for
the purposes of this review, but just a note.

There's no function maked make_symlink_lldb anywhere, so it's failing.

zturner commandeered this revision.Feb 12 2015, 10:45 AM
zturner edited reviewers, added: ki.stfu; removed: zturner.

Going to commandeer this revision because it's the only way I can attach a patch to it.

zturner updated this revision to Diff 19845.Feb 12 2015, 10:48 AM
zturner edited edge metadata.

In the future, please try to test the python changes more thoroughly. I know we can't expect everyone to test everything on every platform, but I'm not sure this was tested at all. There were lots of places not in Windows specific codepaths that had syntax errors. Like calling a function that didn't exist, returning a local variable of the wrong name, spaces instead of tabs, and more.

I'm glad that there's more people digging into this stuff, but please make sure to test sufficiently. I'm uploading a new version of this which makes it work insofar as I can test. Please feel free to download this revision and make sure it works on other platforms.

BTW, you can commandeer this revision back to re-take ownership of it. Use the combo box at the bottom of the page on the Phabricator web interface.

zturner updated this revision to Diff 19846.Feb 12 2015, 10:52 AM

I accidentally left in a stray debugging message in the CMakeLists.txt. This update removes that.

ki.stfu edited edge metadata.Feb 12 2015, 11:15 AM

In the future, please try to test the python changes more thoroughly. I know we can't expect everyone to test everything on every platform, but I'm not sure this was tested at all. There were lots of places not in Windows specific codepaths that had syntax errors. Like calling a function that didn't exist, returning a local variable of the wrong name, spaces instead of tabs, and more.

I'm glad that there's more people digging into this stuff, but please make sure to test sufficiently. I'm uploading a new version of this which makes it work insofar as I can test. Please feel free to download this revision and make sure it works on other platforms.

Sorry. I want to see what you had changed but when I selected Diff 4 in red column and Diff 6 in green column, it looked like you had removed all my changes in python scripts (see here). Is it a phabricator bug?

ki.stfu commandeered this revision.Feb 12 2015, 11:15 AM
ki.stfu edited reviewers, added: zturner; removed: ki.stfu.

Is it an error? It should be "../../../liblldb%s"?

Actually the original was correct. On Windows our directory structure looks like this:

ninja
--bin
----liblldb.dll
--lib
----site-packages
------lldb
--------lldb_d.pyd

So the lldb_d.pyd needs to be a link to the liblldb.dll. I will test with the correct relative path back in place and let you know.

You missed "python2.7" folder in "lib", which contains "site-packages" folder.

If so, then path should be "../../../../bin/liblldb%s".

zturner edited edge metadata.Feb 12 2015, 11:59 AM

Im seeing the same issue the diff. Try applying the patch and use git diff.
Does that work?

Hi Ilia, please see D7598 where I've attempted to rectify the messed up diff. Let me know if that patch works for on your platforms.

Im seeing the same issue the diff. Try applying the patch and use git diff.
Does that work?

Yes. It is shown correctly on my computer.

Hi Ilia, please see D7598 where I've attempted to rectify the messed up diff. Let me know if that patch works for on your platforms.

Thanks for created review. It was simpler to verify your changes. I have tested Diff 6 on OS X, it works. I think we can close D7598 and commit patch it from here. I'll do it today, if you don't mind.

ki.stfu updated this revision to Diff 19916.Feb 13 2015, 12:08 PM
ki.stfu edited edge metadata.

without any changes

oops. sorry guys. it's my local files.

ki.stfu updated this revision to Diff 19917.Feb 13 2015, 12:15 PM

restore Diff 6

ki.stfu updated this revision to Diff 19918.Feb 13 2015, 12:18 PM

Restore to Diff 6

ki.stfu updated this revision to Diff 19920.Feb 13 2015, 12:26 PM
ki.stfu updated this revision to Diff 19922.Feb 13 2015, 12:29 PM

restore tools/argdumper/CMakeLists.txt

ki.stfu closed this revision.Feb 13 2015, 12:30 PM
ovyalov added inline comments.
test/functionalities/launch_with_glob/TestLaunchWithGlob.py
24

It seems this test is failing on Linux - could we mark it "requires Darwin" for now?

Hi Oleksiy, I spent a few minutes looking into this test. Can you try
checking what CanDebugProcess() returns for your Platform implementation?
If it returns false, change it to true and see if this test still fails for
you.