We need a test for this to ensure we don't regress.

Looks fine. Sorry for the delay.

Glad this worked.

Might need to retitle again as well

So I looked at the instances of STT_NOTYPE in a few shared libraries on my computer and they do seem to have valid addresses in them.

It would be a good idea to see what this $debug_ranges627 symbol actually is. It seems like a stray linker symbol that wasn't stripped? It would be a good idea to figure out what this symbol is so we can determine how to deal with it correctly.

I think a better solution is to not give the symbol's address a valid section in the first place. This would be done in ObjectFileELF.cpp when . It doesn't seem like the section is valid because $debug_ranges627 shouldn't have an address that matches something in .text. It just doesn't make sense.

I was out on vacation, sorry for the delay. Looks good.

I was out on vacation, sorry for the delay. Looks good.

I was out on a week long vacation, sorry for the delay. The main questions is: should symbols with NOTYPE actually be included in any address lookups. I would vote for no as how can 1 address resolve to more than one section. That doesn't make sense to me. One address should resolve to one and only one section. Sounds like the NOTYPE symbols should just say they don't have a section. Thoughts?

I was out on vacation, sorry for the delay. Looks good.

Remove the outdated comment and we are good to go.

It would be nice to auto detect the names correctly?

maybe lldb::SymbolType::eSymbolTypeCompiler?

So the issue is with the ObjectFileELF when it makes its symbol table. It is taking this symbols:

I'm ok as long as Pavel is ok. Please wait for the OK from him as well.

So where did the other changes go where we use "--fd" for non Apple builds? Did those changes get lost? They will be needed.

Looking at an ELF file with DWARF, I see:

(in the above log output .text has a valid address, but .debug_aranges doesn't. We might need to cross correlate withe the program headers to tell if something gets loaded (PT_LOAD) for a given file address. What does the output of:

In your log we see:

Two issues we need to resolve:

• I don't think AddressClass should have Absolute as a value. Absolute values in symbol tables are just numbers, not necessarily addresses.
• This change won't work for ARM

Greg, are you with me checking this in?

In D35734#819193, @probinson wrote:

In D35734#818778, @tberghammer wrote:

This section have been already removed from Dwarf5 so I agree that we shouldn't spend too much time adding support for it.

Compilers wanting to use type units and DWARF 4 should be emitting them in the .debug_types section. DWARF 5 kept the concept but moved them back into the .debug_info section. Supporting type units in general seems like a good idea, and the only question is where they live and what the exact details of the header look like.

The enum might need to be scoped outside the function or in a header file...

I believe a crash looks like:

Please init fd

We have been hitting this at Facebook for server apps that statically link the entire world (libc, libc++, libstdc++, all other needed shared libraries) as the debug info is too large unless -fdebug-types-section is used.

Looks like there already is a test case that was failing as Jim mentioned. Accepting based on that.

Looks fine, just add an error that checks that when the --file options is used, that only one argument (symbol file) is specified.

Very nice.

Add a test for this and this will be good to go.

I am not sure how sensitive typemaps are to the names of the arguments? Maybe we can just have you use different names and then the two typemaps won't collide? You can also write some manual code to do the remapping without typemaps.

One other idea if we go with the UUID::Type solution: if we have UUIDs that are different, we could ask the ObjectFile (ObjectFileELF) to see if it thinks the files go together. Not sure if there is anything in the ELF file that would allow us to do that. In Mach-O we have all definitions for the .text and .data sections, just not the bytes. So it would be easy to compare the file address and file sizes of the sections that are in both to see if they match. Something like:

In D35607#815769, @labath wrote:

In D35607#814982, @clayborg wrote:

So we should just make sure this works:

(lldb) target symbols add --shlib a.out debug_binaries/a.out

The --uuid option is there to help you match up without having to specify the file path. If I am understanding correctly, "a.out" has no UUID or build ID, but "debug_binaries/a.out" does?

In this case, neither of the files has a build id/UUID. However, the tricky part here is that in this case we "invent" a UUID by doing a checksum of the whole file. This is done to support the .gnu_debuglink https://sourceware.org/gdb/onlinedocs/gdb/Separate-Debug-Files.html method of finding symbols. It works like this:

• if a file has a gnu_debuglink section, we set the UUID to the crc inside that section
• if a file does not have a gnu_debuglink section, we *assume* it will be used as a gnu_debuglink target, and also set the UUID to the crc we compute from the whole file contents. This works fine for the gnu_debuglink case, as then the rest of the machinery will match the files based on their UUIDs. However, it does not work for the general case without the debug link, as then the files will end up with two different "UUID"s (because they are based on the file content), and everything will consider them as different.
  
  The solution that would seem best to me here would be to leave the UUID of files which don't have a build-id as empty, as the gnu_debuglink is not really a UUID -- it's main function is to contain a file path, and the crc just acts as a sanity check. This should make the logic for matching two uuid-less files saner, but it would require us to implement the gnu_debuglink method in a different way (which I think is fine, as the current way seems pretty hacky, and causes us to often compute crcs of large files needlessly). I have no idea how much of our existing code would blow up if we started having files without a uuid though.
  
  What do you think about that?

I forgot the "return 0;" in the "if (!invalid_forms.empty()) {"

Good idea on passing back the unsupported form. Expand to all unsupported forms as suggested in inlined comments and this will be good to go.

So we should just make sure this works:

I am not sure how you expect to submit any patches then. Patches must be tested prior to submission.

We will need a warning to be emitted in SymbolFileDWARF::CalculateAbilities() stating we won't parse the DWARF due to "unsupported DW_FORM value of 0x1f20". Since this isn't done in a multi-threaded way, the warning should work without deadlocking.

Looks like we need to add support for:

"true.all" has custom DW_FORM_XXX values. There is no way to know what those are without adding parsing support to handle them. The right fix is to add support for this into LLDB. I will attach abbrev.txt which contains the .debub_abbrev dump.

Ah, that was hidden by "Older Comments"... Got the file. Will take a look

DWARF tags that we don't know should be just fine. If you can forward a DWARF file that exhibits this behavior I can take a look.

This will stop the crash, but I don't believe this is the right way to fix this. We should probably remove all DIEs from the CU and warn that there was a problem parsing debug info in the specific compile unit. We don't want LLDB to crash later one when it is trying to build stuff using the DWARF that is just abnormally truncated.

Before any changes are submitted, we assume you are getting a clean test suite run.

Any news on any speed changes? Please post the output of:

Let Pavel try this and verify and this is good to go.

In D31172#808105, @labath wrote:

In D31172#808038, @clayborg wrote:

Added a suggestion to make Target::GetArchitecturePlugin lazy and to ask if the arch is supported. Most of the time we will set this once and never need to change it, even when we attach, change arch, etc. The new suggestion will allow us to use the same arch plug-in without re-fetching it. Let me know what you think as the changes are thrown out there to see what people's opinions are.

Well... I don't feel strongly about it, but it's not my preferred solution (the IsSupported() call feels hacky). But since we're already throwing ideas out there, here are two of mine:

• confine all the changes to m_arch to the SetArchitecture method. Right now only one of the assignments to m_arch is done outside of this method, and that one can be easily converted to a call to SetArchitecture(). This should make sure the two values never get out of sync.

Added a suggestion to make Target::GetArchitecturePlugin lazy and to ask if the arch is supported. Most of the time we will set this once and never need to change it, even when we attach, change arch, etc. The new suggestion will allow us to use the same arch plug-in without re-fetching it. Let me know what you think as the changes are thrown out there to see what people's opinions are.

I think is saying we should just store the Architecture plug-in in the target instead of in the process. It will also need to be cleared when the target arch is changed.

Ah yes, I thought there was something missing...

Looks fine to me, lets make sure Pavel is OK with this. On MacOS socketpair is also much faster. Please run the following command while stopped at a breakpoint with and without your change:

Note that if you put "Differential Revision: " followed by the URL for this:

A few inline fixes, but this looks great. Very close

Wait for an OK from Pavel as well.

Looks good. Just one question on why we register a packet handler.

Much better on the API with Swig. Just a few inlined questions around GetRawBytes.

See inlined comments.

In D33035#799404, @abhishek.aggarwal wrote:

In D33035#799058, @clayborg wrote:

So std::vector shouldn't be used in a public API. You should make a class that wraps your objects. LLDB's public API has lldb::SBInstruction and lldb::SBInstructionList as examples. std::vector on other systems compiles differently for debug and release builds and things will crash.

Hi Greg .. Does it go for the tools built on top of LLDB (e.g. in my case where feature library is not a part of liblldb shared lib but built on top of it)? If yes, then I will proceed to remove std::vector from C++ public API of the tool and create a new class for it.

So std::vector shouldn't be used in a public API. You should make a class that wraps your objects. LLDB's public API has lldb::SBInstruction and lldb::SBInstructionList as examples. std::vector on other systems compiles differently for debug and release builds and things will crash. If you need this via swig for internal kind of stuff, then use a typemap where you map std::vector<T> to a list() with T instances in it in python.

I would think we would try to enable this using something like:

Looks good.

If the CIEs can change per CIE,l then this patch is fine.

Glad this is happening. Does this mean we won't see the "bad eh frame" warnings anymore on linux? See inlined comments.

I trust Pavel to review this since it is in the Linux native plugins mostly.

Looks like a good starting point. Thanks for the changes.

Zach made some comments via e-mail:

The other option for fixing the problem with showing the wrong variables in the backtraces would be to make up a new frame-format string that is used for uniqued stack frames and use that format when showing uniqued stack frames:

Clicked submit too early with last comments. After seeing how we previously needed to create a temp UniqueStack just to do the lookup, and also how UniqueStack make its thread index ID list mutable, a better solution is to use a std::map as shown in the inlined comments.

Much cleaner. After seeing how we must construct a UniqueStack just so we can search for it, an even cleaner solution would be to make unique_stacks into:

We should probably store the stacks as lldb::addr_t values that are load addresses for quicker comparisons and searches. Inlined code details things more clearly.

No need for the "StructuredDataType::" or "lldb::StructuredDataType::" prefixes. Since we aren't using an enum class (it was before) and since all enumerators start with "eStructuredDataType", it is now clear where this enum comes from without prefixing with the enum type name and/or namespace. I know you were replicating what was there before. There are two ways to fix this:
1 - define the lldb::StructuredDataType and "enum class StructuredDataType" and then shorten the names by removing eStructuredDataType from each enumerator
2 - leave lldb::StructuredDataType defined as enum StructuredDataType" and leave the names long and they can be used on their own.

If all R"( strings are under 80 characters, this is good to go. If not, just fix and submit, no extra review needed.

Looks like we are down to just running clang format on the code so the R"( strings don't go over 80 chars and this is good to go from me.

Very close and overall very nice, just a few implementation details to take care of.

Add a test and fix the minor things as suggested and this will be good to go.

I believe this patch is close enough for an initial checkin. We can iterate on it once it is in? We seem to be down to how to best iterate over ranges in ranges for intersect and for contains. Let me know your thoughts. Or feel free to commandeer this and check it in as you want it...

Close, some minor fixes.

Very nice.

Just a few makefile changes where CFLAGS is being modified and this will be good to go. Much better location for this. Sean, please keep an eye out for this kind of thing in the future, I believe there is a lot of type searching code and function searching code that could have the same thing done in the future.

Sorry I hadn't posted my reply comments, just did that. Will look at your new comments and answer as needed.

• Added an assert to catch if we don't remove overlapping or invalid ranges that catches the issue where the lambda was returning false instead of true and modified DieRangeInfo::insert() to assert as mentioned.
• fixed ::contains and ::intersects to use a more efficient traversal.
• fixed all other issues