Page MenuHomePhabricator

[lldb] Fixing the bug that the "log timer" has no tab completion
ClosedPublic

Authored by gedatsu217 on Mar 27 2020, 12:32 AM.

Details

Summary

I fixed the bug that the "log timer" has no tab command.

Original code has the only CommandObjectLogTimer class, but it is not sufficient. Thus I divided the content of CommandObjectLog class into CommandObjectLogEnable class, CommandObjectLogDisable class, CommandObjectLogDump class, CommandObjectLogReset class, CommandObjectLogIncrement class.

Event Timeline

gedatsu217 created this revision.Mar 27 2020, 12:32 AM
teemperor requested changes to this revision.Mar 27 2020, 3:35 AM

I think that makes sense. However, it causes a test to fail:

FAIL: lldb-api :: commands/log/invalid-args/TestInvalidArgsLog.py (95 of 1985)
******************** TEST 'lldb-api :: commands/log/invalid-args/TestInvalidArgsLog.py' FAILED ********************
Script:
--
/usr/bin/python /home/teemperor/work/ci/llvm/lldb/test/API/dotest.py --arch x86_64 -s /home/teemperor/work/ci/build/lldb-test-traces -S nm -u CXXFLAGS -u CFLAGS --env ARCHIVER=/usr/bin/ar --env OBJCOPY=/usr/bin/objcopy --env LLVM_LIBS_DIR=/home/teemperor/work/ci/build/./lib --build-dir /home/teemperor/work/ci/build/lldb-test-build.noindex --lldb-module-cache-dir /home/teemperor/work/ci/build/lldb-test-build.noindex/module-cache-lldb/lldb-api --clang-module-cache-dir /home/teemperor/work/ci/build/lldb-test-build.noindex/module-cache-clang/lldb-api --executable /home/teemperor/work/ci/build/./bin/lldb --compiler /home/teemperor/work/ci/build/./bin/clang --dsymutil /home/teemperor/work/ci/build/./bin/dsymutil --filecheck /home/teemperor/work/ci/build/./bin/FileCheck --lldb-libs-dir /home/teemperor/work/ci/build/./lib /home/teemperor/work/ci/llvm/lldb/test/API/commands/log/invalid-args -p TestInvalidArgsLog.py
--
Exit Code: 1
[...]
FAIL: LLDB (/home/teemperor/work/ci/build/bin/clang-11-x86_64) :: test_timer_empty (TestInvalidArgsLog.InvalidArgsLogTestCase)
======================================================================
FAIL: test_timer_empty (TestInvalidArgsLog.InvalidArgsLogTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/teemperor/work/ci/llvm/lldb/packages/Python/lldbsuite/test/decorators.py", line 350, in wrapper
    return func(self, *args, **kwargs)
  File "/home/teemperor/work/ci/llvm/lldb/test/API/commands/log/invalid-args/TestInvalidArgsLog.py", line 21, in test_timer_empty
    self.expect("log timer", error=True,
  File "/home/teemperor/work/ci/llvm/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 2329, in expect
    self.assertFalse(self.res.Succeeded(),
AssertionError: True is not False : Command 'log timer' is expected to fail!
Config=x86_64-/home/teemperor/work/ci/build/bin/clang-11
----------------------------------------------------------------------

The test just checks that "log timer" (which will be interpreted as "log timers) fails, but it seems CommandObjectMultiword marks these invalid invocations as "valid" and then prints the help text. I think the proper fix is to change that CommandObjectMultiword should mark commands such as "log timers" as failed commands (as they aren't really intentional).

Some other minor points:

  • Every patch that changes functionality should have a test (see lldb/test/API/functionalities/completion/TestCompletion.py where your short test can go).
  • If you upload a diff, make sure you include all the context into the diff file (E.g., by doing -U9999 or something like that). Also your diff should be relative to the top directory (yours is relative to the lldb subdirectory).

Beside that this looks good to me, thanks!

This revision now requires changes to proceed.Mar 27 2020, 3:35 AM

Sorry, I do not understand what you said completely. In short, is "log timer" (not "log timers") expected to fail? In that case, for example, "log ena" operates the same as "log enable". Thus, I think that "log timer" should operate the same as "log timers". Or, should I fix CommandObjectMultiword to solve such problems? (In short, should I fix it so that "log ena" do not operate like "log enable", "log timer" do not operate like "log timers", "brea li" do not operate "breakpoint list" and so on?)

In addition to it, when you test, how commands do you execute?
Now, when I execute "bin/lldb-dotest ../lldb/test/API/functionalities/completion -p TestCompletion.py", an error that "No valid FileCheck executable; some tests may fail..." occurred. I do not know what I should do.

Sorry, I do not understand what you said completely. In short, is "log timer" (not "log timers") expected to fail? In that case, for example, "log ena" operates the same as "log enable". Thus, I think that "log timer" should operate the same as "log timers". Or, should I fix CommandObjectMultiword to solve such problems? (In short, should I fix it so that "log ena" do not operate like "log enable", "log timer" do not operate like "log timers", "brea li" do not operate "breakpoint list" and so on?)

In addition to it, when you test, how commands do you execute?
Now, when I execute "bin/lldb-dotest ../lldb/test/API/functionalities/completion -p TestCompletion.py", an error that "No valid FileCheck executable; some tests may fail..." occurred. I do not know what I should do.

The "log timer" thing is just a typo, that's not relevant to the test failing. LLDB automatically extends commands if they are unique prefixes to known commands and both should behave the same. So that's fine. I just fixed the typo upstream, so now it's "log timers" also on master.

Anyway, my point was that the test is failing with this patch. The old test called "log timers" and expected the old custom error message. With this patch, "log timers" is not a successful command that prints the help text. (With successful command I mean that the CommandReturnObject is now marked as success when "log timers" is executed).

So this test needs to be fixed. This can be done by either changing the test so that it expects "log timers" to succeed and update the error message it expects, or by changing that calling a CommandObjectMultiword without any following arguments will cause that the command will be marked as not successful.

Thanks. I revised it. Then, I want to have a test. How do I have a test?

I forgot to answer the test question. To run the test you set the LIT_FILTER environment variable to TestCompletion.py and just run make check-lldb (or ninja check-lldb if you used ninja as your CMake backend). check-lldb will run all tests and LIT_FILTER is the string that all test names have to match to be run.

Thanks. I revised it. Then, I want to have a test. How do I have a test?

There is a TestCompletion.py file that already has several completion tests. You can just take one of the test cases there and copy-paste->adapt it there. The new test can also just be in TestCompletion.py as another function.

gedatsu217 added a project: Restricted Project.

I revised the test case. If "log timers" is executed, an error will not occur.

When I submitted this patch last time, I probably included all the context into the diff file (I executed "git diff -U999999 > CommandObjectLog.patch"). I submit a patch made of this command this time, so would you confirm if it is appropriate. (And I do not know about directory you said, so would you confirm if it is good?)

Also, I tried to execute "ninja check-lldb" many times, but an error that is strange to me occurred. I am dealing with this error now.
I could not execute "ninja check-lldb", but I could run a specific test. Thus I fixed the code so that the previous error does not occur by changing the test case. Would you confirm if it goes well?

teemperor accepted this revision.Apr 8 2020, 11:01 PM

Sorry for the delay.

This LGTM me now. The diff seems also be correct, even though the TestInvalidArgsLog.py part somehow doesn't apply for me, but I can do that manually.

If check-lldb doesn't work for you then that's bad. What's the error message you get?

This revision is now accepted and ready to land.Apr 8 2020, 11:01 PM
This revision was automatically updated to reflect the committed changes.
gedatsu217 added a comment.EditedApr 9 2020, 11:10 AM
[39/575] Linking CXX shared library lib/libc++abi.1.0.dylib
FAILED: lib/libc++abi.1.0.dylib 
: && /Library/Developer/CommandLineTools/usr/bin/c++ -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color  -g  -dynamiclib -Wl,-headerpad_max_install_names -nodefaultlibs  -compatibility_version 1.0.0 -current_version 1.0.0 -o lib/libc++abi.1.0.dylib -install_name @rpath/libc++abi.1.dylib projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/cxa_aux_runtime.cpp.o projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/cxa_default_handlers.cpp.o projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/cxa_demangle.cpp.o projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/cxa_exception_storage.cpp.o projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/cxa_guard.cpp.o projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/cxa_handlers.cpp.o projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/cxa_unexpected.cpp.o projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/cxa_vector.cpp.o projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/cxa_virtual.cpp.o projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/stdlib_exception.cpp.o projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/stdlib_stdexcept.cpp.o projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/stdlib_typeinfo.cpp.o projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/abort_message.cpp.o projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/fallback_malloc.cpp.o projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/private_typeinfo.cpp.o projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/stdlib_new_delete.cpp.o projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/cxa_exception.cpp.o projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/cxa_personality.cpp.o  -Wl,-rpath,@loader_path/../lib  -lSystem  -Wl,-exported_symbols_list,/Users/shu/Documents/llvm-project/libcxxabi/src/../lib/itanium-base.exp  -Wl,-exported_symbols_list,/Users/shu/Documents/llvm-project/libcxxabi/src/../lib/new-delete.exp  -Wl,-exported_symbols_list,/Users/shu/Documents/llvm-project/libcxxabi/src/../lib/personality-v0.exp && :
Undefined symbols for architecture x86_64:
  "__ZTIDu", referenced from:
     -exported_symbol[s_list] command line option
  "__ZTIPDu", referenced from:
     -exported_symbol[s_list] command line option
  "__ZTIPKDu", referenced from:
     -exported_symbol[s_list] command line option
  "__ZTSDu", referenced from:
     -exported_symbol[s_list] command line option
  "__ZTSPDu", referenced from:
     -exported_symbol[s_list] command line option
  "__ZTSPKDu", referenced from:
     -exported_symbol[s_list] command line option
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

This is an error message. I know it is caused around libc++, but I don't know where parts should I fix.
I did below to solve it.

  • delete build directory and make build directory again (cmake -G Ninja -DLLVM_ENABLE_PROJECTS="clang;lldb;libcxx;libcxxabi" ../llvm)
  • build each project(ninja lldb, ninja clang, ninja cxx, ninja cxxabi)
    • but when I executed "ninja cxx" and "ninja cxxabi", the same error occured.

I sometimes tried to solve this problem this week, but I could not solve it.
(I could not go to my laboratory because of coronavirus spreading, and I was busy dealing with that, so I could not take much time.)

[39/575] Linking CXX shared library lib/libc++abi.1.0.dylib
FAILED: lib/libc++abi.1.0.dylib 
: && /Library/Developer/CommandLineTools/usr/bin/c++ -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color  -g  -dynamiclib -Wl,-headerpad_max_install_names -nodefaultlibs  -compatibility_version 1.0.0 -current_version 1.0.0 -o lib/libc++abi.1.0.dylib -install_name @rpath/libc++abi.1.dylib projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/cxa_aux_runtime.cpp.o projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/cxa_default_handlers.cpp.o projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/cxa_demangle.cpp.o projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/cxa_exception_storage.cpp.o projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/cxa_guard.cpp.o projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/cxa_handlers.cpp.o projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/cxa_unexpected.cpp.o projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/cxa_vector.cpp.o projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/cxa_virtual.cpp.o projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/stdlib_exception.cpp.o projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/stdlib_stdexcept.cpp.o projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/stdlib_typeinfo.cpp.o projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/abort_message.cpp.o projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/fallback_malloc.cpp.o projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/private_typeinfo.cpp.o projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/stdlib_new_delete.cpp.o projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/cxa_exception.cpp.o projects/libcxxabi/src/CMakeFiles/cxxabi_shared.dir/cxa_personality.cpp.o  -Wl,-rpath,@loader_path/../lib  -lSystem  -Wl,-exported_symbols_list,/Users/shu/Documents/llvm-project/libcxxabi/src/../lib/itanium-base.exp  -Wl,-exported_symbols_list,/Users/shu/Documents/llvm-project/libcxxabi/src/../lib/new-delete.exp  -Wl,-exported_symbols_list,/Users/shu/Documents/llvm-project/libcxxabi/src/../lib/personality-v0.exp && :
Undefined symbols for architecture x86_64:
  "__ZTIDu", referenced from:
     -exported_symbol[s_list] command line option
  "__ZTIPDu", referenced from:
     -exported_symbol[s_list] command line option
  "__ZTIPKDu", referenced from:
     -exported_symbol[s_list] command line option
  "__ZTSDu", referenced from:
     -exported_symbol[s_list] command line option
  "__ZTSPDu", referenced from:
     -exported_symbol[s_list] command line option
  "__ZTSPKDu", referenced from:
     -exported_symbol[s_list] command line option
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

This is an error message. I know it is caused around libc++, but I don't know where parts should I fix.
I did below to solve it.

  • delete build directory and make build directory again (cmake -G Ninja -DLLVM_ENABLE_PROJECTS="clang;lldb;libcxx;libcxxabi" ../llvm)
  • build each project(ninja lldb, ninja clang, ninja cxx, ninja cxxabi)
    • but when I executed "ninja cxx" and "ninja cxxabi", the same error occured.

I sometimes tried to solve this problem this week, but I could not solve it.
(I could not go to my laboratory because of coronavirus spreading, and I was busy dealing with that, so I could not take much time.)

I don't think I've seen this error before. Are you on the latest Xcode?

In any case, you should be able to just have "clang;lldb" in the project list which should get your check-lldb working. You won't get libc++ tests but at least the testsuit itself should work this way.

I performed a factory reset for OS upgrade and rebuild LLDB, and solved this problem. Thanks.