-
Notifications
You must be signed in to change notification settings - Fork 151
WIP: LuaJIT unwind support #69
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Hi, thank you for your PR! We will look at it in this state to understand how to proceed and to save your time. |
|
Nice |
perforator/lib/elf/elf.h
Outdated
|
|
||
| TStringBuf symbolName{name.data(), name.size()}; | ||
| for (const auto& targetSymbol : targetSymbols) { | ||
| if (symbolName.find(targetSymbol) == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we only match prefix here and avoid using find ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed to StartsWith (hope it's right).
that's some sort of custom STL, and sources appears to be generated from some "pxd" file. it doesn't make development easier, as I struggled to find documentation for these custom TString classes :)
perforator/lib/lua/lua.cpp
Outdated
| template <typename ELFT> | ||
| TMaybe<TParsedLuaVersion> ParseVersion( | ||
| const llvm::object::ObjectFile& file, | ||
| const TLuaAnalyzer::TSymbols&symbols |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
space between type name and argument name
perforator/lib/lua/lua.h
Outdated
| } | ||
| }; | ||
|
|
||
| enum class EUnicodeType { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be removed, seems like copy-paste from python
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed, removed
perforator/lib/lua/lua.cpp
Outdated
|
|
||
| template <typename ELFT> | ||
| TMaybe<TParsedLuaVersion> ParseVersion( | ||
| const llvm::object::ObjectFile& file, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to pass object file here if ParseVersion just returns version parsed from a symbol name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, removed. that was a copy-paste from python.
| return false | ||
| } | ||
| return true | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should add newline between functions
|
|
||
| namespace NPerforator::NBinaryProcessing::NLua { | ||
|
|
||
| NPerforator::NBinaryProcessing::NLua::LuaConfig BuildLuaConfig(llvm::object::ObjectFile* objectFile) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets be consistent with other functions and return TMaybe<NPerforator::NBinaryProcessing::NLua::LuaConfig
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I've used a python as a base (skeleton), which did:
NPerforator::NBinaryProcessing::NPython::PythonConfig BuildPythonConfig(llvm::object::ObjectFile* objectFile) {
while PHP does:
TMaybe<NPerforator::NBinaryProcessing::NPhp::PhpConfig> BuildPhpConfig(llvm::object::ObjectFile* objectFile) {
I've changed Lua to return TMaybe
| *result.MutablePhpConfig() = std::move(phpConfig.GetRef()); | ||
| } | ||
|
|
||
| *result.MutableLuaConfig() = std::move(luaConfig); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle null config to be consistent with if statements above
| log.String("buildid", buildID), | ||
| log.Any("version", analysis.PythonConfig.Version), | ||
| ) | ||
| if analysis.PythonConfig != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to change it here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was testing something at initial state to see how python initialized. reverted back.
| } | ||
|
|
||
| // Create Lua symbolizer | ||
| if enabled := p.conf.BPF.TraceLua; enabled == nil || *enabled { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets consider that if TraceLua is nil it is defaulted to false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, I've changed
question: python and php/jvm are handled quite a different, are there reasons for that? e.g. php/jvm have their own FeatureFlagsConfig entries, while python doesn't.
until now, I followed more python model while doing LuaJIT code.
|
I'll need to rebase code later and clean up the code. since the time I've opened PR we made some progress on these items:
|
040c23c to
a90e39c
Compare
works only on my version/build of interprerter, offsets are hard-coded!
--debug doesn't load too large program if both Lua and Python are active
it makes easier to identify Lua frames amongst C remove some no longer needed prints
they do not expose LuaJIT_version_* symbol if they happen to expose luaopen_jit, just assume it's 2.1 for now
Formatting, refactoring, new colors, new metrics. Remove debug prints as they no longer useful
…che invalidation. Add more metrics.
…changing BPF_TRACE to BPF_PRINTK for LUA_TRACE
Remove unused enums
…code for these changes.
closes: #66
this is heavily work in progress, more like proof of concept, code is totally dirty and incomplete (there are many debug prints, commented code blocks for experiments and so on...).
works only on my version/build of interpreter, offsets are hard-coded (f9140a62)!
it means it obviously won't work on arbitrary version of LuaJIT - if version/flags changed, offsets would likely to change.
mostly inspired by:
heavily based on existing Python and PHP unwind implementations in perforator
what is implemented:
what is not implemented yet, to be implemented:
debug_framepcreturnsNO_BCPOS, checkisluafuncfails for some reason, to be debugged and fixed