Another fix for buildbots. Don't use llvm::ArrayRef to refer to data, store in std::vector<>. In release builds this will fail.

Fixed a buildbot issue for linux with:

More documentation fixed for DataEncoder.

Update the headerdoc and fixed some of the comments that were out of date or left over from the inital copy from DataExtractor.

Updated to make DataEncoder always copy any data and own the buffer.

In D115073#3174376, @labath wrote:

I don't think we actually disagree here. I'm aware of your use case (let's call it "dynamic mode") for appending to a buffer. I agree it's useful and I don't want to change that. What I want to remove is the other mode (non-dynamic). Presently, this is the only mode, but we're not actually making a good use of it. All of the existing use cases can be implemented with a "dynamic" buffer. And the code would be much simpler since there is only one mode to support -- one in which the data encoder owns the buffer it is writing to and can resize it at will.

In D113650#3173914, @nealsid wrote:

In D113650#3168622, @clayborg wrote:

I can't build on macOS now. I checked out the source code and tried to do:

cmake -G Ninja -DCMAKE_BUILD_TYPE:STRING=Debug -DLLVM_ENABLE_ASSERTIONS:BOOL=TRUE -DLLVM_ENABLE_PROJECTS='clang;libcxx;libcxxabi;lldb' -DLLDB_BUILD_FRAMEWORK:BOOL=TRUE -DLLDB_USE_SYSTEM_DEBUGSERVER=ON -DLLDB_EDITLINE_USE_WCHAR=0 -DLLDB_ENABLE_LIBEDIT:BOOL=TRUE -DLLDB_ENABLE_CURSES:BOOL=TRUE -DLLDB_ENABLE_PYTHON:BOOL=TRUE -DLLDB_ENABLE_LIBXML2:BOOL=TRUE -DLLDB_ENABLE_LUA:BOOL=FALSE ../llvm-project/llvm

Random drive-by comment - any particular reason you compile without WCHAR support in libedit? I have a patch that removes the conditional support from our Editline class (actually it was committed and had to be reverted :P). But I was under the impression that nobody was using the non-WCHAR support anymore. Thanks.

In D115073#3173041, @labath wrote:

It seems to me that the DataEncoder class is a lot more complicated than what is necessary for our existing use cases. All of them involve creating a new buffer (either empty, or with some pre-existing content), modifying it, and passing it on. A lot of the DataEncoder complexity (IsDynamic, UpdateMemberVariables, ...) stems from the fact that it wants to support writing into unowned buffers, functionality that we don't even use.

Fixed "DataEncoder::Append*" methods so they return false correctly when used on caller owned buffer objects.

Update headerdoc comments and enable clang-format.

Thanks for doing that! I was able to successfully run the test on linux, and then I added extra context to the stop. It seems that the two breakpoints are set correctly, but the second breakpoint is not hit and the program exits. So all is good from the symbol table changes as far as I have been able to tell since both breakpoints were set. Below is the extra output that shows the first breakpoint was hit, and then we continue and don't hit the second breakpoint, but we do exit with the correct exit status of 12, so the program isn't crashing. In the lines below we now can see after the "continue" command, that is just exits with the expect statu of 12 (which is argc == 1 and it multiplies it by 3 and then by 4). I am happy to help look at this. If you can send a copy of the arm "minidebuginfo-set-and-hit-breakpoint.test.tmp.binary" binary I might be able to see what is going on.

In D113650#3168812, @lawrence_danna wrote:

@clayborg

Does this fix it for you?

https://reviews.llvm.org/D114973

Yep, fixes it for me! Thanks!!

I am not cross compiling in this case.

if I try to manually set the python3 executable with:

-DPYTHON_EXECUTABLE=/Applications/Xcode.app/Contents/Developer/usr/bin/python3

I get the following error:

[cmake] CMake Error at /Users/gclayton/Documents/src/lldb/clean/llvm-project/lldb/CMakeLists.txt:53 (message):
[cmake]   Crosscompiling LLDB with Python requires manually setting
[cmake]   LLDB_PYTHON_RELATIVE_PATH.

I can't build on macOS now. I checked out the source code and tried to do:

In D114288#3165212, @clayborg wrote:

In D114288#3163808, @omjavaid wrote:

Hi @clayborg
This breaks LLDB Arm/Linux buildbot. https://lab.llvm.org/buildbot/#/builders/17/builds/14035

I will check this out on linux. Any reason why I did not get a message to my email that this was failing?

In D114288#3163808, @omjavaid wrote:

Hi @clayborg
This breaks LLDB Arm/Linux buildbot. https://lab.llvm.org/buildbot/#/builders/17/builds/14035

Let the call_once protect the ObjectFile::m_symtab_up as suggested.

In D114288#3149188, @labath wrote:

I don't believe this solution is correct.

How did this work before? Is it because ObjectFileELF::GetSymtab contained the same (incorrect) unique_ptr pattern?

LGTM!

I resubmitted with a fix in https://reviews.llvm.org/D114288

Never mind, it seems to be fixed. Although the problem I mentioned is still true where some test cases might set "self.target = debugger.CreateTarget(...)" and if those test cases try to use the check_breakpoint function. See suggested code edit for a potential fix.

In D111899#3140422, @aprantl wrote:

FYI, this broke all the TestObjCNewSyntax* tests. I *think* I've fixed them.

The right fix IMHO is to rename the function in lldbtest.py from "def target(self):" to "def selected_target(self):" and then find all call sites and fix them.

This change causes many test suite errors due to inline comment. Please fix ASAP

I had to revert this. There is a deadlock in:

Suggested different replacement strings for the comments.

FYI: I am not familiar enough with the expression parser and IR interpreter logic to be able to be able to give the ok here. I hope other expression parser experts will continue to give good feedback on this patch!

In D82367#3135210, @dblaikie wrote:

In D82367#3133152, @clayborg wrote:

In D82367#3133132, @dblaikie wrote:

In D82367#3133081, @clayborg wrote:

Is ".debug_gnu_pubtypes" different and/or better than the old ".debug_pubtypes" in terms of contents? AFAIK all debuggers completely ignore these sections as they don't contain all types (only public types). I did a quick web search for ".debug_gnu_pubtypes" and didn't find any docs or anything documenting this format.

My general understanding is that GCC/GDB folks implemented .debug_gnu_pubnames(/types) because of the lack of guarantees in debug_pubnames(/types) - GCC does consume/rely on these sections (& gold can produce .gdb_index from these gnu_pub* sections - which is /necessary/ for correctness of gdb when using Split DWARF, in fact).

I'm guessing like lots of stuff, it's probably basically defined by the agreement between GCC and GDB - and Clang/LLVM just tries to implement gnu_pub* to be as compatible as possible with that.

Was there any documentation on this where the contents of this new format are detailed?

Not that I know of.

Sounds promising and I would love to have LLDB to not have to manually index DWARF on all linux/android targets if at all possible.

Presumably LLDB should probably be looking at the newer .debug_names format that's been standardized/documented, and derived from the apple names accelerator tables.

In D113810#3133655, @jingham wrote:

In D113810#3132418, @clayborg wrote:

In D113810#3128741, @jingham wrote:

The stop id includes all the user expression stops. So the same "drive through code" could produce very different stop counts if you ran a bunch of p-s in one session but not the other. You can't do better than that at present since we only track the natural/vrs. expression stop id for the last stop. But if it mattered you could keep a separate "natural stop" counter and report that instead. You can't quite get back to the natural stops by subtracting the number of expressions, because not all expressions run the target, and some do twice (if the one-thread run times out).

I actually don't care about the number of natural user stops as the user will be aware of these stops. The info I am interested in is when a debug session has thousands of stops that auto resumed a debug session. This can happen if unix signals are used for program flow control or working around things. Android uses SIGSEGV for catching java NULL derefs and many times we will auto resume from these depending on signal settings. If a debug session is taking a long time it is nice to know. Some users might also have some python module that gets loaded and might end up setting a breakpoint with a callback that can auto resume.

IMO, the real solution for this issue is for lldb to invent (if there isn't already) a "auto-continue signals" packet, so we could tell the stub to restart from those signals w/o even telling lldb about it. Oh, but I guess you can't do that for SIGSEGV...

Some perf numbers that involve loading a large iOS Application and all of its shared libraries without caching first and then with caching are found below. The "lldb-perf.py" command will start a timer, run any LLDB commands and then stop the timer and print out. Before the first run, I run "sudo purge" to purge the kernel file cache, and then I run one "cold" run with the file caches being empty. Then I run the same thing again with hot file caches. So the first run is always slower.

Thinking about this a bit more after Pavel's comment on if performance was improved due to mangling: we don't currently get any perf improvement from mangling/demangling. We might be able to get better performance out of this as well if we _do_ cache the "mangled/demangled" counterparts (save both strings when they are related instead of just saving the mangled string like I do in this patch) when we serialize the Mangled objects. When the objects are deserialized, they could read both strings and then no demangling would need to happen for that mangled name since it could be registers as the counterpart strings during deserialization...

In D113789#3130741, @labath wrote:

Maybe it's good for starting a discussion, but I am surprised that we would need to cache symtab information. This is a fairly simple format that needs to be read at application runtime as well, so I am surprised that reading it from the cache is substantially faster than doing it from the object file. What is the slow part there? Is it demangling by any chance?

In D82367#3133132, @dblaikie wrote:

In D82367#3133081, @clayborg wrote:

Is ".debug_gnu_pubtypes" different and/or better than the old ".debug_pubtypes" in terms of contents? AFAIK all debuggers completely ignore these sections as they don't contain all types (only public types). I did a quick web search for ".debug_gnu_pubtypes" and didn't find any docs or anything documenting this format.

My general understanding is that GCC/GDB folks implemented .debug_gnu_pubnames(/types) because of the lack of guarantees in debug_pubnames(/types) - GCC does consume/rely on these sections (& gold can produce .gdb_index from these gnu_pub* sections - which is /necessary/ for correctness of gdb when using Split DWARF, in fact).

I'm guessing like lots of stuff, it's probably basically defined by the agreement between GCC and GDB - and Clang/LLVM just tries to implement gnu_pub* to be as compatible as possible with that.

Is ".debug_gnu_pubtypes" different and/or better than the old ".debug_pubtypes" in terms of contents? AFAIK all debuggers completely ignore these sections as they don't contain all types (only public types). I did a quick web search for ".debug_gnu_pubtypes" and didn't find any docs or anything documenting this format.

In D113810#3128741, @jingham wrote:

The stop id includes all the user expression stops. So the same "drive through code" could produce very different stop counts if you ran a bunch of p-s in one session but not the other. You can't do better than that at present since we only track the natural/vrs. expression stop id for the last stop. But if it mattered you could keep a separate "natural stop" counter and report that instead. You can't quite get back to the natural stops by subtracting the number of expressions, because not all expressions run the target, and some do twice (if the one-thread run times out).

Added more comments to the encoding functions to detail how things are encoded.
Fixed error typos.
Added version to symbol table cache file so we can improve the format as time goes on and avoid loading out of data caches.

I'm good with this to get things started. Pavel?

In D113521#3122654, @jingham wrote:

In D113521#3120703, @labath wrote:

I have two high-level questions about this:

• what should be the relative priority of executable ModuleSP vs the launch info? IIUC, in the current implementation the module takes precedence, but it seems to me it would be more natural if the launch info came out on top. One of the reasons I'm thinking about this is because you currently cannot re-run executables that use exec(2) (I mean, you can, but it will rarely produce the desired results). This happens because we use the post-exec executable, but the rest of the launch info refers to the main one. If the executable module was not the primary source of truth here, then maybe the we could somehow use this mechanism to improve the exec story as well (by storing the original executable in the launch info or something).

As you point out indirectly above, there are really three entities in play here. There's the Target's ExecutableModule; there's the GetExecutableFile in a user-provided ProcessLaunchInfo passed to SBTarget::Launch, for instance; and there's the ProcessLaunchInfo that's stored in the TargetProperties held by the target.
So we need to decide what it means when these three entities differ.

Shouldn't the target;s launch info get a full copy of the user-provided ProcessLaunchInfo upon launch? If that is the case, then we always can refer to the target's launch info as the truth as far as ProcessLaunchInfo goes? Then the only question we have is what to do if the target has a module

This was submitted as a series of patches that updated "statistics dump" to emit JSON:

LGTM. I will ok soon if no one else chimes in!

nice!

Any progress on this? This causes serious debugging issues.

LGTM. This is in Jason's function so I will let him give the final ok.

This is how launch does it as well, so LGTM.

ok, lgtm

In D112691#3095010, @jingham wrote:
I can see wanting to dump statistics at various points in the running of a process, maybe triggered by breakpoints, for instance. In that case I wouldn't want to dump the settings data - if it is indeed redundant (see above) every time. Having the settings as a separate emission would make that possible. And just like we add gdb-remote as a convenience, it would be fine to have some low level commands that you can reassemble and then a portmanteau command that generates a "good for most purposes" report.

In D112691#3095010, @jingham wrote:

Do you care about the history of these settings? After all, the problem might arise because someone set a setting then unset it. Your statistics approach wouldn't catch that. If you are really trying to build an architecture where we can track this sort of problem down, then you might need more of a history approach, where the settings and certain other changes in the state of the debugger mark epochs, and you aggregate data into those epochs?

In D112691#3093889, @JDevlieghere wrote:

I understand the need for something like this to make some of the statistics more meaningful, but this is stretching the notion of statistics. Conceptually, this is approaching something like a dump of the debugger for issue/performance analysis. I think that idea is really exciting, and from that perspective there's a lot more useful information we could add to it. Long term, I can see this output be something that we ask users to include in every bug report.

In D112587#3092120, @jingham wrote:

Note, the breakpoint serialization for breakpoints only serializes the filter/resolver and the breakpoint specific options. It doesn't do anything with locations or their options. After all, you when you go to reset a breakpoint in another session (which is what this serialization is for) you have no way of knowing what locations you are going to find.

I am sure you have checked the Swift codebase as well for any uses. If so LGTM.

Looks good to me, but the expression parser isn't my main area of expertise. I would like to see someone with more knowledge in the expression parser to make the final ok. Raphael?

Changes in this version:

• If we fail to get "details" from the structured data, we put the error string into the "details" dictionary with the error string.
• Added "numLocations" (int64_t) and "numResolvedLocations" (int64_t) and "internal" (bool) into each breakpoint's stats.
• Include the internal breakpoints in the "breakpoints" so we are tracking all breakpoints that are being set.
• Removed the top level "names" field because it is already contained in the "details".

Added newlines to files that needed them at the end of the file.