Page MenuHomePhabricator

[lldb/Lua] Add Boilerplate for a Lua Script Interpreter
ClosedPublic

Authored by JDevlieghere on Dec 9 2019, 4:45 PM.

Details

Reviewers
labath
Group Reviewers
Restricted Project
Commits
rG67de896229c0: [lldb/Lua] Add Boilerplate for a Lua Script Interpreter
Summary

This adds the boilerplate necessary to support the Lua script interpreter. The interpreter itself is non-functional and just says it's not yet implemented.

Discussion on the mailing list:http://lists.llvm.org/pipermail/lldb-dev/2019-December/015812.html

Diff Detail

Event Timeline

JDevlieghere created this revision.Dec 9 2019, 4:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 9 2019, 4:45 PM
Herald added a subscriber: mgorny. · View Herald Transcript

Return the right name from GetPluginNameStatic.

xiaobai added a subscriber: xiaobai.Dec 9 2019, 5:03 PM

I'm really excited to see where this goes! :D

lldb/source/Plugins/ScriptInterpreter/Lua/ScriptInterpreterLua.cpp
64

Why is this Null and not Lua? :P

labath added a subscriber: labath.Dec 10 2019, 12:31 AM
labath added inline comments.
lldb/cmake/modules/LLDBConfig.cmake
28 ↗(On Diff #232963)

I think this will tick off some bots (and people) because it means that the default configuration will not build unless one has (compatible?) lua installed. Though I don't really like that, the usual way to handle external dependencies in llvm is to detect their presence and automatically disable the relevant functionality.

Now, that's not how things work in lldb right now, so it _may_ make sense to do the same for lua (though it also may make sense to port everything to the llvm style). However, the current lldb behavior has been a source of friction in the past and I suspect a fresh build error might reignite some of that.

Anyway, you have been warned...

332 ↗(On Diff #232963)

Can we replace this (and maybe python too, while at it) with a Host/Config.h entry? A global definition means that one has to recompile everything when these change in any way, whereas in practice only a handful of files need this..

mgorny added inline comments.Dec 10 2019, 3:18 AM
lldb/cmake/modules/LLDBConfig.cmake
28 ↗(On Diff #232963)

Fixing this is one of the things at the far end of my todo. If you could look into replacing the disable logic with something better, a lot of people would really be grateful.

JDevlieghere marked 5 inline comments as done.
JDevlieghere added inline comments.
lldb/cmake/modules/LLDBConfig.cmake
28 ↗(On Diff #232963)

I've changed the default for now and put up an "RFC" here: https://reviews.llvm.org/D71306

Reuse IOHandlerEditline for the Lua interpreter.

^ updated the wrong patch

emaste added a subscriber: emaste.Dec 16 2019, 12:51 PM
labath accepted this revision.Dec 19 2019, 1:47 AM

Given the positive response to the RFC, I think we can start landing this stuff.

lldb/cmake/modules/LLDBConfig.cmake
28 ↗(On Diff #232963)

That's cool. I think this can stay as off until we deal with the deal with the dependency problem

This revision is now accepted and ready to land.Dec 19 2019, 1:47 AM
This revision was automatically updated to reflect the committed changes.
labath added inline comments.Dec 20 2019, 7:54 AM
lldb/cmake/modules/LLDBConfig.cmake
62 ↗(On Diff #234745)

Umm... I guess this wanted to be LLDB_ENABLE_LUA ?