- User Since
- Sep 23 2014, 10:11 AM (260 w, 5 d)
Wed, Sep 18
Tue, Sep 17
So it seems that "constexpr" not needing to be captured is not consistent across all builds. The attempt to fix with 372144 then caused some buildbots that have warnings as errors to fail since they would warn that you shouldn't capture a constexpr and that became an error. Windows buildbots require the capture to be there or they fail.
$ svn commit
Transmitting file data ...done
Committed revision 372135.
Any more issues to resolve or does this look good now?
Mon, Sep 16
- Change "enum InfoType" to "enum InfoType : uint32_t"
Fri, Sep 13
- Fix two operator bool calls to use "bool(<object>)" instead of "<object>.operator bool()"
Thu, Sep 12
- Fix comment from /< to /
- change "sizeof(uint32_t)" to "4"
- Updates license on FunctionInfo.cpp
Added Jason Molenda who owns the unwind stuff so he might be able to comment and help us with this patch.
My opinion is we need to be able to ask both the SymbolFile and ObjectFile for unwind plans in the API, but we can always just ask the SymbolFile for the unwind plan and the default SymbolFile virtual function can just defer to the ObjectFile. We also need to say "I need an async unwind plan" or "I need an sync unwind plan". Since we always have a symbol file, we fall back to SymbolFileSymtab if we don't have DWARF or PDB.
Wed, Sep 11
$ svn commit
Transmitting file data ......done
Committed revision 371657.
Switch LineTable and InlineInfo inside of FunctionInfo to be Optional<T>.
Tue, Sep 10
- Fixed license on LineTable.h/.cpp
- Fixed camel case on encode_special
- Remote LTOC_ prefix from LineTableOpCode enumerators
Mon, Sep 9
$ git llvm push
Pushing 1 monorepo commit:
f54a49a8133 Fix ELF core file memory reading for PT_LOAD program headers with no p_filesz
Transmitting file data ..done
Committed revision 371457.
Committed f54a49a8133 to svn.
LGTM. Test seems about as good as we can do. Pavel, you ok with this now?
Does anyone have a chance to look at this? Full documentation on the line table encoding is in the LineTable.h header file in the comments.
At the object file level I would love to see much less of this specific unwind info making it into the ObjectFile interface. Why can't we just ask the ObjectFile to provide an UnwindPlan for a given address. There is so much complexity within the UnwindPlan where it is managing all these specific unwind plans. IMHO we should just ask the ObjectFile for an UnwindPlan and let the ObjectFile decide which one is best. Right now UnwindPlan manages a ton of different kinds of unwind alternatives and UnwindPlan has all sorts of knowledge about each specific unwind plan type (ARM compact unwind, Apple compact unwind, EH frame, .debug_frame, arch default, and more). We seem to be adding to this complexity here by making all ObjectFile instances having to know how to create each different type when UnwindPlan already has the all the abstraction we need in UnwindPlan::Row.
Fri, Sep 6
This looks like a good approach to me. Pavel?
So made a "sbt" command in a Python module as a way to get a backtrace when our backtracing fails because Android almost never makes it back to the first frame for any stacks. This code just grabs the current SP, looks up the memory region using the SP, and then it uses this as the address range (current SP back to the base of the stack) to scan for any pointers with an address aligned address that falls into executable sections. What it doesn't do, is to try and backup one instruction from the potential RA values to see if there is a branch to the current function. This avoids having to know anything about stack parameter sizes and just scan the stack for all executable memory region pointers, and then ask each architecture to backup one instruction from each proposed RA and see if there is a function call to the current function. For ARM this is easy: backup by 4 for ARM and 2 or 4 for Thumb and see if there is a branch and link. For x64, it would be trickier, but it might be as easy as backup up by 1 + address size and looking for a callq. If we can successfully do this, we could really improve stack backtracing for all architectures and OSs. Thoughts?
Thu, Sep 5
Add full error handling to encoding and decoding in the same way that I did it in https://reviews.llvm.org/D66600.
Wed, Sep 4
$ svn commit
Transmitting file data .....done
Committed revision 370936.
Tue, Sep 3
Fri, Aug 30
Seems like revision 370054 sets the screen size quite wide. Just checking that this change will be used for this test?
It is great to have tests. The main issue I would see is some of the text might not render correctly if the screen size is too small. Can we control the size (char width and height) of the screen in pexpect tests?
Added error handling to InlineInfo::encode and InlineInfo::decode with full testing.
Just a few nits around DEFINE_GPR64 and how the "alt" names are specified. See inline comments.
Just a few string reflows and fix a typo
Mon, Aug 26
Added Pavel since he previously worked on Android LLDB at Google. Pavel, please add any additional reviewers to this patch that you think might be interested.
Should we do this via a "settings set" setting instead of an environment variable? This would make it easier to test running a test suite kind of situation.
Also is this the server that runs on the host machine where LLDB resides, or remotely on the device? If this is something that would be done at "platform connect" time, each platform can have its own unique options and arguments to "platform connect". Might be better to just add them as options to the "platform connect" command. Something like:
- use llvm_unreachable when encoding InlineInfo objects. We don't want them emitted into the GSYM file and taking up space if they aren't valid.
- improve comment about terminating child InlineInfo object siblings.
What about having the first one that matched being the one that is used and avoid errors all together? We do this with the regex alias stuff already. With regular expressions people are sure to make ones that conflict with each other.
Aug 22 2019
Don't clear the inline info when decodeAll returns false so that we can see what was actually decoded. Not sure how great of an error we can return other than "not enough data". I am open to suggestions?
Now that obj2yaml is a library, we might be able to make some real DWARF to exercise this? I can custom make any DWARF you want as I have a DWARF generator.
Sounds good about LanguageSet being cheap to pass by value. Are there any paths that will call this over and over where we still will be calculating the LangaugeSet over and over in a type system? We might benefit from using llvm::once and a static LanguageSet in the static functions that return LanguageSets if that is the case?
- document arguments to "static bool decodeAll(...)" in InlineInfo.cpp
- If decodeAll() returns false, clear the InlineInfo
Looks good overall. Just a question of it we want to return "const LanguageSet &" to avoid copies. Also switch static functions that currently return "LanguageSet" over to use static variables and llvm::once to init them and then return "const LanguageSet &".
Aug 21 2019
Aug 19 2019
Let's ask the underlying question: Which of the above use-cases does a GSYM writer need, and why isn't the StringRef case sufficient?
Pavel, any comments on the testing code? LGTM.
- remove function name from doxygen comments
- change @param to \param
- fix typos
- added full FileWriter doxygen comments
- renamed writeCStr to writeNullTerminated
- update all DataExtractor offsets to be uint64_t to match recent DataExtractor changes
Can anyone take a look please? Another week, no comments.
The other thing to think about is the SB layer must be thread safe. StringList is not thread safe right now, but it probably should be. We might need to keep the internal version of StringList and make sure it is thread safe. That will ensure that our buildbots always run clean.
Aug 16 2019
Needs a test, but looks good.
Why are we not just using ObjectFileJIT? I am guessing these breakpoint expressions create one of these by compiling the breakpoint expression and JIT'ing it just like any other expression. If this is the case, then why do we need to create a ObjectFileTrampoline? Seems like we could add .cfi directives to the assembly we use for the breakpoint condition function so that we can unwind out of it?
Aug 15 2019
Better. Might be good to put a comment inside CalculatePublicStopInfo() regarding why the "SetStopInfo(GetStopInfo());" is needed. Seems like you could just remove the code from the sight of it. I will leave that up to you though.
Aug 14 2019
Looks fine to me. A few nits.
Aug 13 2019
I agree with Adrian, this isn't something that needs to hold up this patch, but I do think it is worth doing soon in LLVM. The line table parsing code just needs to take a std::function (or llvm equivalent) that gets called like a lambda when a row is to be pushed. The current LLVM parsing code just has this callback create its own line tables as is currently done. We would just create our own line tables using our callback. So it won't take much work at all in llvm to make this happen and then be able to adopt it in LLDB.
Thanks for running the perf tests, that will be great to keep handy somewhere. Might be good to check this test in somewhere. I still think that making changes to the llvm line parser might be a good idea where we can get a callback when a row is to be pushed with the current line table state. This would allow symbolication clients that need just one address, like in atos kind of cases, to not have to store an array of these things and then search them. It would also allow us to just populate our line table as we see fit without the need to have complete LLVM tables and then convert them. Shouldn't be very hard to modify the code in llvm. Let me know what you think.