Much better.

Never mind, re-reading your original comment made it clear.

Who is holding the lock and then fork'ing? Seems like we should fix that? I thought all log calls should be "lock, log, unlock". Why is this causing problems?

Feel free to add me as a reviewer. Zach as well.

I agree with all of Zach's suggestions.

Looks good

Looks good.

(sorry for the delay, I was out on vacation for a week)

Sorry for the delay, I was out on vacation for a week.

Looks good. Would be nice to add support for byte sizes of 3, 5 and 7 to the unchecked version as noted in inline comments, or remove the function if no one is using this function. Just a few quick fixes and this will be good to go.

Back from vacation, sorry for the delay.

Looks fine. One main questions for new linux archs in particular: is linux using the lldb-server to debug these days even when debugging locally? If so, then this patch only needs to implement both a native register content and not the lldb_private::RegisterContext subclass.

Verify you have commit priveleges, and then check in the sources with the following text in the SVN commit message:

Differential Revision: https://reviews.llvm.org/D38142

Looks good.

Thanks for the explanation of the private vs shared, we can leave it as you have it. Just rename to RelocateSection and this will be good to go.

Very nice section auto relocate code. Just a few things to clean up and this will be good to go.

So I found a lot more stuff. The inlined comments detail quite a few changes we should make. The ELF relocations can use a bit of cleanup. Let me know if anything is not clear.

Very close. See inlined comments.

We should have a test. The test would need to use pexpect or something similar. If anyone has any pointer on how to make a test for this, please chime in. I was thinking just a pexpect based test.

Looks fine.

ok. Then back to the "can we use StringRef"? We might be able to check if the second.data() is NULL? It might be NULL if the string doesn't end with a newline. It it does, it might be empty but contain a pointer to the '\0' byte?

I think Zach might be correct. If we already gathered the data, why not just output it all? Lets answer this first before returning to which implementation to use when parsing it up into lines and dumping it. Seems a shame to throw any data away.

I like the solution detailed above by tatyana-krasnukha

If we get a response of "l" for the "qfThreadInfo", it might be worth trying to send a "H1" packet (select thread ID 1 for subsequent continues and steps) to see if the server knows about thread 1 before proceeding? This patch also broke other platforms that respond with "OK" to the "qfThreadInfo". So we really need to do some extra verification that "l" was the only things received in response to the "qfThreadInfo".

The default return value of WillResume should be no error. Sorry for not catching this. The core file Process subclasses will need to override this manually.

This should be good to go. Watch the buildbots for failures.

As long as everyone agrees that no threads from qfThreadInfo means there is one thread whose thread ID is 1 then this is ok.

The multiple feature fix is fine.

Looks fine to me.

Why assert and not emit an error???

The top 32 bits is sometimes used to identify one of many DWARF files that are used to create a cohesive single view of the debug info such as when using DWARF in .o files on Mac, and other things like .dwo support and others. So the top 32 bits do mean something. I don't see any problem with this patch though. You will need to watch all of the buildbots closely after checking this in to make sure it doesn't affect other platforms that use these top 32 bits. Just because the test suite runs clean on your system, doesn't mean it will run smoothly on others. Let me know if you want to take this chance and be on the hook for fixes for other platforms in case things go south.

Just add a more descriptive comment that states this exact issue more clearly as I didn't understand the issue from the comment that is there now (the comment that starts on line 1644).

I tend to use the DWARF generator to create a super minimal DWARF file, then obj2yaml that. It tends to be really short and ok for adding as a test. The DWARF generator only exists in the gtest for the DWARF, but you can add a new gtest, generate the DWARF, then remove the gtest.

Yikes, I don't see how this was working at all, but it has been that way for a while? Good catch.

Thanks for the info. Does this seem like it is a compiler bug? Seems like there should be a DW_AT_comp_dir if the DW_AT_GNU_dwo_name is relative? Is it written in the DWO spec that if the DW_AT_comp_dir is empty then it is referring to itself? I would rather not hack LLDB if we can avoid it. If this is a long released compiler generating this, then we do need a fix in LLDB since it will be running into these binaries from here on out. If this is a new compiler, lets fix the compiler. Let me know which case this is.

Makes sense even though this can be quite expensive.

Can you explain the issue with an example?

This is sure to trigger things in the test suite. We will need to ensure a few things:

• test suite runs cleanly in debug mode after the lldbassert is added
• without changes to CommandObjectRegister.cpp that the lldbassert is triggered, and if not, add a test

Yes: lldbassert would be fine for that since those get compiled out during release. Patch looks fine. If we already have a test that would trigger the new "lldbassert" you will add, then no need for a special test for this, else we need a test that triggers this.

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: