In D67589#1673771, @jankratochvil wrote:

In D67589#1673756, @labath wrote:

We couldn't use PointerIntPair due to the our abi stability promise (http://lldb.llvm.org/resources/sbapi.html).

Thanks for this document! I did not know it. But isn't the document trying to keep ABI compatibility despite it is not explicitly stated there? Or otherwise why are there so many restrictions.

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
Sending include/llvm/DebugInfo/GSYM/FunctionInfo.h
Sending lib/DebugInfo/GSYM/FunctionInfo.cpp
Sending unittests/DebugInfo/GSYM/GSYMTest.cpp
Transmitting file data ...done
Committing transaction...
Committed revision 372135.

Any more issues to resolve or does this look good now?

In D67390#1672254, @labath wrote:

In D67390#1672210, @kwk wrote:

So my point of this whole question is: What makes a symbol unique in the sense that it shouldn't be added to the symtab if it is already there?

A symbol name is not unique because you can have multiple (static) functions with the same (mangled) name in one module. An address is not unique as well because you can have symbol aliases, which will have the same address (and we want to keep both names to resolve name breakpoints correctly for instance).

The name+address combination (my original suggestion) should be sufficiently unique for the purposes we care about. Theoretically, if you want, you could include some additional items in the uniqueness "key" like symbol type etc. (to rule out the perverse case of somebody setting a "file" symbol to conflict with some other function symbol), but I don't think that is really necessary.

In D67390#1672210, @kwk wrote:

@clayborg thank you for this explanation. My patch for minidebuginfo is already done in D66791 . But this one here I was asked to pull out as a separate patch. For good reasons as we see. I wonder how this extra parameter SymbolMapType of yours does help. In the end I have to identify duplicates. But if no symbol with the same name should be added then why do I care about where the symbol is coming from?

In D67390#1671463, @kwk wrote:

@clayborg what address is it exactly to store here std::map<lldb::addr_t, ContString> SymbolMapType;? As an example dc_symbol.GetAddress().GetFileAddress() is something that would work but as soon as we have minidebuginfo, then we might end having the same symbol coming from two different object files and so their address would still be different. Also do you want me to keep this map in ObjectFileELF?

In D67390#1671463, @kwk wrote:

@clayborg what address is it exactly to store here std::map<lldb::addr_t, ContString> SymbolMapType;? As an example dc_symbol.GetAddress().GetFileAddress() is something that would work but as soon as we have minidebuginfo, then we might end having the same symbol coming from two different object files and so their address would still be different. Also do you want me to keep this map in ObjectFileELF?

• Change "enum InfoType" to "enum InfoType : uint32_t"

In D67390#1671128, @kwk wrote:

In D67390#1671018, @labath wrote:

In D67390#1667270, @kwk wrote:

@labath how shall we go about this? We do have the situation that when you lookup a symbol you might find it twice if it is in .dynsym and in .symtab. Shall I adjust the test expectation to that or change my implementation?

That's a good question (and another reason why I wanted this to be a separate patch). Since only two tests broke it does not seem like having some symbols twice does much harm. OTOH, having an identical symbol twice does seem like asking for trouble down the line. One possibility would be to restrict this merging to the gnu_debugdata case only. Another option would be to make the merging code smarter and avoid adding the symbol a second time if it has the same name and address. That would have the advantage of having the symbol just once in the common case, while still preserving the full information (in case the symbol tables were munged independently of the gnu_debugdata thingy).

Overall, I guess I would prefer the last solution (inserting only different symbols) unless that turns out to be difficult. In that case, I think just restricting this to gnu_debugdata is fine.

The crucial line is the following line in ObjectFileELF::ParseSymbols():

symtab->AddSymbol(dc_symbol);

If I change that to:

symtab->FindSymbolsByNameAndAddress(dc_symbol.GetName(), dc_symbol.GetAddress(), indexVector)
if (indexVector.empty()) {
  symtab->AddSymbol(dc_symbol);
}

In D67390#1671018, @labath wrote:

In D67390#1667270, @kwk wrote:

@labath how shall we go about this? We do have the situation that when you lookup a symbol you might find it twice if it is in .dynsym and in .symtab. Shall I adjust the test expectation to that or change my implementation?

That's a good question (and another reason why I wanted this to be a separate patch). Since only two tests broke it does not seem like having some symbols twice does much harm.

• Fix two operator bool calls to use "bool(<object>)" instead of "<object>.operator bool()"

• Fix comment from /< to /
• change "sizeof(uint32_t)" to "4"

lgtm. Jim?

• 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.

$ svn commit
Sending include/llvm/DebugInfo/GSYM/FunctionInfo.h
Adding include/llvm/DebugInfo/GSYM/LineTable.h
Sending lib/DebugInfo/GSYM/CMakeLists.txt
Sending lib/DebugInfo/GSYM/FunctionInfo.cpp
Adding lib/DebugInfo/GSYM/LineTable.cpp
Sending unittests/DebugInfo/GSYM/GSYMTest.cpp
Transmitting file data ......done
Committing transaction...
Committed revision 371657.

Switch LineTable and InlineInfo inside of FunctionInfo to be Optional<T>.

• Fixed license on LineTable.h/.cpp
• Fixed camel case on encode_special
• Remote LTOC_ prefix from LineTableOpCode enumerators

$ git llvm push
Pushing 1 monorepo commit:

f54a49a8133 Fix ELF core file memory reading for PT_LOAD program headers with no p_filesz

Sending lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/elf-core/thread_crash/TestLinuxCoreThreads.py
Sending lldb/trunk/source/Plugins/Process/elf-core/ProcessElfCore.cpp
Transmitting file data ..done
Committing transaction...
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.

In D67022#1660945, @guiandrade wrote:

In D67022#1660901, @clayborg wrote:

This looks like a good approach to me. Pavel?

Even without tests? Just to clarify my last comment, I could not find a way to use the command "target modules dump as" to ensure we won't have a regression in the future. Apparently, with or without the extra calls, that command's output looks the same.

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?

Add full error handling to encoding and decoding in the same way that I did it in https://reviews.llvm.org/D66600.

In D67022#1656874, @labath wrote:

Thanks for the clarification Greg.

Functionally, this patch seems fine, but I am still wondering if we shouldn't make this more efficient. std::set is not the most memory-efficient structure, and so creating a std::set entry just to store one bit seems wasteful, particularly as there is already a container which uses the context as a key (m_decl_ctx_to_die). Could just shove this bit into the m_decl_ctx_to_die map (probably by changing it to something functionally equivalent to map<DeclContext, pair<bool, vector<DWARFDie>>>)? Given that the sole "raison d´être" of this map is to enable the ParseDeclsForContext functionality, I don't think that having that flag be stored there should be awkward. Quite the opposite, it would remove the awkward back-and-forth between the SymbolFileDWARF and the DWARFASTParsers (symbol file calls ForEachDIEInDeclContext, passing it a callback; then ast parser calls the callback; finally the callback immediately re-enters the parser via GetDeclForUIDFromDWARF) -- instead we could just have the SymbolFileDWARF do ast_parser->PleaseParseAllDiesInThisContextIfTheyHaventBeenParsedAlready(ctx) and have everything happen inside the parser.

So, overall, it seems to me that this way, we could improve the code readability while reducing the time, and avoiding a bigger memory increase. In fact, since we now will not be using the list of dies after they have been parsed once, we could even free up the vector<DWARFDie> after the parsing is complete, and thereby reduce the memory footprint as well. That would be a win-win-win scenario. :)

WDYT?

$ svn commit
Sending include/llvm/DebugInfo/GSYM/InlineInfo.h
Sending include/llvm/DebugInfo/GSYM/Range.h
Sending lib/DebugInfo/GSYM/InlineInfo.cpp
Sending lib/DebugInfo/GSYM/Range.cpp
Sending unittests/DebugInfo/GSYM/GSYMTest.cpp
Transmitting file data .....done
Committing transaction...
Committed revision 370936.

In D67022#1653248, @guiandrade wrote:

Hey guys,

This change is more for me to get to know what you think. I've noticed that the GetDeclForUIDFromDWARF() calls inside SymbolFileDWARF::ParseDeclsForContext (https://github.com/llvm/llvm-project/blob/ef82098a800178a1f973abb8af86eaa690a29734/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp#L1216) often end up having no side effect besides generating a llvm::DenseMap::find invocation (https://github.com/llvm/llvm-project/blob/f07b4aff06d83c6ad25d95f456fbc12b2d2a0a0c/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp#L3322), especially when we evaluate multiple expressions inside the same scope. So I was wondering if there's a way to improve that. Do you guys think we could do something like what this change proposes, or am I missing something important? Thanks!

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

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.

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.

In D65952#1624799, @labath wrote:

In D65952#1623297, @clayborg wrote:

So I am confused. Are we keeping SymbolVendor around for locating symbols files or are we getting rid of it entirely?

Well... right now my idea was to keep it around as an class with just some static methods for the purposes of locating symbol files, though I'm open to other ideas too. After this patch the SymbolVendor class will contain just one method (FindPlugin), but I think that in the future it could be used to host functionality that is common for different symbol vendors. Here, I'm mainly thinking of Symbols/LocateSymbolFile.cpp (formerly Host/Symbols.cpp), which contains a bunch of functionality for searching for symbol files (so it could fall under the symbol vendor mandate), and it is currently being called by other symbol vendors to do their job. However, it is also being called from placed other than symbol vendors, and so more refactoring would be needed for that to fit in neatly.

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 &".

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.

In D66357#1634175, @guiandrade wrote:

In D66357#1633461, @clayborg wrote:

Needs a test, but looks good.

Sure, I agree. Though, it's not total clear to me how we could do that.

I imagined something like the following.

TEST_F(DWARFASTParserClangTests,
       TestGetDIEForDeclContextReturnsOnlyMatchingEntries) {
  auto ast = ClangASTContext(...);
  auto ast_parser = DWARFASTParserClang(ast);
  CompilerDeclContext decl1(nullptr, (void *)1LL);
  CompilerDeclContext decl2(nullptr, (void *)2LL);
  CompilerDeclContext decl3(nullptr, (void *)2LL);
  CompilerDeclContext decl4(nullptr, (void *)3LL);
  ast_parser->LinkDeclContextToDIE(decl1, DWARFDIE());
  ast_parser->LinkDeclContextToDIE(decl2, DWARFDIE());
  ast_parser->LinkDeclContextToDIE(decl3, DWARFDIE());
  ast_parser->LinkDeclContextToDIE(decl4, DWARFDIE());

  auto decl_ctx_die_list = ast_parser->GetDIEForDeclContext(decl2);
  EXPECT_EQ(2u, decl_ctx_die_list.size());
  EXPECT_EQ(decl2.GetOpaqueDeclContext(),
            decl_ctx_die_list[0].GetDeclContext().GetOpaqueDeclContext());
  EXPECT_EQ(decl3.GetOpaqueDeclContext(),
            decl_ctx_die_list[1].GetDeclContext().GetOpaqueDeclContext());
}

But DWARFASTParserClang::LinkDeclContextToDIE is protected. Do you have any ideas to overcome that?

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?

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.

Looks fine to me. A few nits.

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.

lgtm