Page MenuHomePhabricator

[lldb/lua] Supplement Lua bindings for lldb module
ClosedPublic

Authored by siger-young on Aug 15 2021, 9:44 AM.

Details

Summary

Add necessary typemaps for Lua bindings, together with some other files.

Signed-off-by: Siger Yang <sigeryeung@gmail.com>

Diff Detail

Event Timeline

siger-young created this revision.Aug 15 2021, 9:44 AM
siger-young requested review of this revision.Aug 15 2021, 9:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 15 2021, 9:44 AM

Using lua_newuserdata instead of lua_newuserdatauv to support Lua 5.3

siger-young retitled this revision from [lldb/lua] Supplement typemaps for Lua bindings to [lldb/lua] Supplement Lua bindings for lldb module.Aug 15 2021, 12:59 PM
siger-young edited the summary of this revision. (Show Details)
siger-young edited the summary of this revision. (Show Details)Aug 15 2021, 3:12 PM
tammela added inline comments.Aug 16 2021, 4:44 AM
lldb/bindings/lua/lua-typemaps.swig
317

Please fix this

402

This seems it could leak.
Are you sure it doesn't? If so add a comment here explaining where it's freed.

425
426
433
453

The raw variants here are overkill, you can the use normal ones

460

Leaking?

470

else Lua error?

tammela requested changes to this revision.Aug 16 2021, 4:50 AM

Missing test cases!
Either a script that test it all, individual unit tests or a combination of both.

This revision now requires changes to proceed.Aug 16 2021, 4:50 AM

This update adds some tests for Lua LLDB module.

tammela added a comment.EditedSep 4 2021, 2:28 PM

Trying to run the tests in my system failed with the following:

FAIL: lldb-api :: lua_api/TestLuaAPI.py (1793 of 2389)
******************** TEST 'lldb-api :: lua_api/TestLuaAPI.py' FAILED ********************
Script:
--
/usr/bin/python3.9 /home/pedro/vm/llvm-HEAD/lldb/test/API/dotest.py -u CXXFLAGS -u CFLAGS --env ARCHIVER=/usr/bin/ar --env OBJCOPY=/usr/bin/objcopy --env LLVM_LIBS_DIR=/home/pedro/vm/llvm-HEAD/build/./lib --arch x86_64 --build-dir /home/pedro/vm/llvm-HEAD/build/lldb-test-build.noindex --lldb-module-cache-dir /home/pedro/vm/llvm-HEAD/build/lldb-test-build.noindex/module-cache-lldb/lldb-api --clang-module-cache-dir /home/pedro/vm/llvm-HEAD/build/lldb-test-build.noindex/module-cache-clang/lldb-api --executable /home/pedro/vm/llvm-HEAD/build/./bin/lldb --compiler /home/pedro/vm/llvm-HEAD/build/./bin/clang --dsymutil /home/pedro/vm/llvm-HEAD/build/./bin/dsymutil --llvm-tools-dir /home/pedro/vm/llvm-HEAD/build/./bin --lldb-libs-dir /home/pedro/vm/llvm-HEAD/build/./lib /home/pedro/vm/llvm-HEAD/lldb/test/API/lua_api -p TestLuaAPI.py --env LUA_EXECUTABLE=/usr/bin/lua5.3
--
Exit Code: 1

Command Output (stdout):
--
lldb version 14.0.0 (https://github.com/llvm/llvm-project.git revision 3c5616bad32ec20af3d160dee79b6da6d5978857)
  clang revision 3c5616bad32ec20af3d160dee79b6da6d5978857
  llvm revision 3c5616bad32ec20af3d160dee79b6da6d5978857
Skipping the following test categories: ['libc++', 'dsym', 'gmodules', 'debugserver', 'objc']

--
Command Output (stderr):
--
Traceback (most recent call last):
  File "/home/pedro/vm/llvm-HEAD/lldb/test/API/dotest.py", line 7, in <module>
    lldbsuite.test.run_suite()
  File "/home/pedro/vm/llvm-HEAD/lldb/packages/Python/lldbsuite/test/dotest.py", line 974, in run_suite
    visit('Test', dirpath, filenames)
  File "/home/pedro/vm/llvm-HEAD/lldb/packages/Python/lldbsuite/test/dotest.py", line 681, in visit
    visit_file(dir, name)
  File "/home/pedro/vm/llvm-HEAD/lldb/packages/Python/lldbsuite/test/dotest.py", line 622, in visit_file
    module = __import__(base)
  File "/home/pedro/vm/llvm-HEAD/lldb/test/API/lua_api/TestLuaAPI.py", line 8, in <module>
    import lit.util
ModuleNotFoundError: No module named 'lit'

My guess is that you are not using LLVM's lit but instead using the system's

lldb/CMakeLists.txt
60–68

This is broken if the user specifies another INSTALL_PREFIX, as it forces it to /usr/local/lib/lua/5.3.
I think a safe option is to always set to lib/lua/5.3, so install prefixes can be concatenated freely.

105

I think we are missing the Framework build here.
LLDB_BUILD_FRAMEWORK is a Darwin specific variable, so you will most likely need one to test this.
I can help if you don't have access to a Darwin device.

lldb/bindings/lua/lua-typemaps.swig
195–213

Add comment here saying that the raw calls will restrict the table to be a sequence of strings.

Please add it in other functions as well.

219–221

This is not being generated by SWIG for some reason.

I think it's more readable to just have an else clause that raises an error on line 212.

275

Else clause here for raising an error for types that are not supported

278

This bug was not caught by the tests, perhaps we need to expand it more?
This would certainly generate a NULL pointer de-reference as the argument would always be NULL, since it's replacing the above code with just free().

siger-young added inline comments.Sep 9 2021, 10:58 AM
lldb/bindings/lua/lua-typemaps.swig
219–221

I tried commenting and uncommenting these lines then diff, found that the typecheck is actually generated on those overloaded functions. SBTarget::Launch no longer works after commenting in my cases:

lua5.3: test.lua:27: Wrong arguments for overloaded function 'SBTarget_Launch'
  Possible C/C++ prototypes are:
    lldb::SBTarget::Launch(lldb::SBListener &,char const **,char const **,char const *,char const *,char const *,char const *,uint32_t,bool,lldb::SBError &)
    lldb::SBTarget::Launch(lldb::SBLaunchInfo &,lldb::SBError &)

It seems that SWIG uses "SWIG_isptrtype" to decide arg matches "char**", so this typecheck is necessary.

For those functions with just one definition (i.e. not overloaded), this typecheck does nothing on them, so it will be typemap(in)'s responsibility to check arg types.

Hi Siger,

We are almost there.
I encourage you to continue working on this patch.

I have been busy the past weeks, but I will try to review ASAP once you address my comments.

lldb/bindings/lua/lua-typemaps.swig
219–221

I see, Thanks.
That was indeed a good way to see what SWIG is generating :).

JDevlieghere requested changes to this revision.Sep 20 2021, 1:11 PM
JDevlieghere added inline comments.
lldb/CMakeLists.txt
55–60

FindLuaAndSwig.cmake is responsible for finding Lua. If LLDB_ENABLE_LUA is set, then you can assume Lua is available.

This revision now requires changes to proceed.Sep 20 2021, 1:11 PM

This update mainly fixed problematic typemaps and adding necessary comments.

Together, it forced Lua installation path as "PREFIX/lib/lua/5.3" and removed "lit.util" in tests.

Fix typo in SBData test.

Rebase commits.

Just one last thing and I think we are done!

Thanks for your work.

lldb/test/API/lua_api/lua_lldb_test.lua
4

Could we not use an external dependency?
For instance in my setup it fails because it couldn't find this library.

siger-young added inline comments.Sep 27 2021, 1:15 AM
lldb/test/API/lua_api/lua_lldb_test.lua
4

Does it make sense to directly copy "luaunit.lua" to the Lua test dir?

tammela added inline comments.Sep 27 2021, 5:18 AM
lldb/test/API/lua_api/lua_lldb_test.lua
4

You don't seem to have a hard dependency on it.
Couldn't you just replicate what you are interested? Instead of bringing in a full blown unit testing framework...

Add dependency "luaunit" as an internal one.

siger-young added inline comments.Sep 27 2021, 5:31 AM
lldb/test/API/lua_api/lua_lldb_test.lua
4

Oh sorry, I didn't notice the comments here.

I will remove the dependency of "luaunit" soon.

Add assertion functions and error status detection to remove "luaunit"

tammela accepted this revision.Oct 3 2021, 12:20 PM

LGTM.

You will have to rebase to main.

Thanks!

Pull and merge conflicts, will soon be merged into main.

This revision was not accepted when it landed; it landed in state Needs Review.Oct 12 2021, 7:10 AM
This revision was automatically updated to reflect the committed changes.