Ok, thanks for explaining. Your reasoning makes sense.

Might be good to have a test where we have a relative DW_AT_name and not DW_AT_comp_dir? Just in case?

Looks good to me

So LLDB treats compile units special depending on the inline strategy. Because of this, I outlined some examples of how and why we should create a compile unit per "FUNC" token. Let me know if anything above was unclear

This is fine to start.

All the change to the symbol vendor make sense, but it seems like all of the call sites should be:

DW_AT_comp_dir isn't enough. See inline suggestions.

But if everyone else thinks differently, I'll go with the majority. Just my initial thoughts.

So there really aren't that many things:

• module only filled out, parse all types
• compile unit only filled out, parse all type from a compile unit
• function filled out, parse all types in a function
• block filled out, parse all types in block, or we can skip this one

So I like the ability to specify any symbol context to parse all types for. If you pass in a symbol context with only a module filled in, we parse all types. If we pass in a symbol context with only the compile unit filled in (no function), we parse all types in the compile unit. If we pass in a symbol context with a function or block, we parse all types in that function or block. Currently it is only being used for compile units, but I don't think we need to change the API.

In D56425#1350253, @JDevlieghere wrote:

In D56425#1350236, @JDevlieghere wrote:

In D56425#1350234, @clayborg wrote:

If we keep the list sorted we might be able to improve finding breakpoints by ID, but that can be done if we need to. BreakpointList::Add would need to insert it sorted, then we can get better than O(n) performance on FindBreakpointByID and Remove (anything that was using find_if when it is searching for a breakpoint by ID).

I'll do this as a follow-up.

On second thought, maybe it's not such a good idea after all. Internal breakpoints are negative so if we keep the vector sorted we'd always insert them at the front of the vector. We could change strategies based on whether the list is internal or not, but that seems overkill, especially given the code is not that hot as Vedant pointed out.

If we keep the list sorted we might be able to improve finding breakpoints by ID, but that can be done if we need to. BreakpointList::Add would need to insert it sorted, then we can get better than O(n) performance on FindBreakpointByID and Remove (anything that was using find_if when it is searching for a breakpoint by ID).

LGTM

Second vote from me to switch to a std::vector since we have a function get breakpoint at index. Other than that, looks good.

The UUID that is used in ELF and Mach-o is designed to be something that is stable in a binary after it has been linked and should be the same before and after any kind of post production (stripping symbols, stripping section content to make a stand alone symbol file, etc). When someone types "target symbols add /path/to/symbol/file/a.out" we will grab its UUID and try to match it up with an existing object file.

You have convinced me! Sorry I had paged out the original intent you conveyed from before the break. Thanks for the details.

I have a minidump generator if you need me to make any specific minidump files for you.

So it seems like DynamicLoaderWindowsDYLD is not doings its job correctly and it needs to be fixed. DynamicLoaderWindowsDYLD is responsible for setting all of the section load addresses correctly using an abstract lldb_private::Process plug-in. No special calls should be added if that is possible. We have someone working on lldb-server.exe and then we will have two clients for DynamicLoaderWindowsDYLD: ProcessWindows and ProcessGDBRemote. DynamicLoaderWindowsDYLD should work for any lldb_private::Process subclass without the need to call back into a specific lldb_private::Process subclass method.

Seems like DynamicLoaderWindowsDYLD isn't doing its job correctly here and DynamicLoaderWindowsDYLD should be fixed. We shouldn't have to go to the process at all in order to set the section load addresses correctly. Why? As you might have seen lldb-server.exe is being worked on now and then we will have two clients for DynamicLoaderWindowsDYLD: the native ProcessWindows plug-in and ProcessGDBRemote. The

excited to see this starting as well!

Just read the original description again and now code makes sense. Main questions for me: is there a benefit to creating multiple sections? Can we just create one section and name it ".breakpad"? Should we not try to find a section that contains the address from the FUNC, line entry or PUBLIC and then avoid creating a section? That way we only create breakpad sections of there are not backing sections from a real object or symbol file?

So is this done as one section per function? Or one section for contiguous functions? What about if there are only symbols? I tried to read the code but wasn't able to decipher everything clearly in my head.

LGTM.

LGTM

The changes from "constexpr" to "const" might introduce global constructors in the shared library which is what we were trying to avoid. The less work that the LLDB shared library does on load the better. We might need to use a macro that expands to "constexpr" for non windows and to "const" for windows in a private LLDB header (PropertyDefinition.h?)

Looks good!

Just remove the "regions-linux-map.dmp" as I already checked one in with rLLDB349658 and this is good to go

In D55841#1336519, @clayborg wrote:

In D55841#1336515, @tatyana-krasnukha wrote:

Add a test

There was one added. See code

In D55841#1336515, @tatyana-krasnukha wrote:

Add a test

In D55718#1336045, @tatyana-krasnukha wrote:

ARCflags are used by ABISysV_arc (related patch D55724). I would be glad to move it to architecture plugin, but I ought to add SetFlags/GetFlags to Architecture interface in this case. Then we'll have the same members in ArchSpec and in Architecture, that may look confusing.

One quick change required to avoid STL in public headers.

2 month ping

See inline comments and let me know what you think.

Change looks good.

Fixed all requested changes. Added a complete test in lit.

nice! A lot of great commands in heap.py. Like see them fixed and improved

It would be nice to see context when you submit diffs. with SVN:

svn diff -x -U9999999 ...

Or git:

git diff -U9999999

Looks good.

Looks good

Looks good. See inline comment and fix if you agree.

In D55727#1331843, @zturner wrote:

This sounds like a good use-case for a lit / FileCheck test. Could you add some simple tests that run lldb with -c on a static minidump with known information inside that we don't change, then run these commands and check the output?

Fixed the test inputs CMakeLists.txt file:

Looks good.

Fixed errors, fully test, and make static function that are local to MinidumpParser.cpp that parse the region info from linux maps, memory info list, memory list and memory 64 list.

Just need a way to verify symbols are good. See my inline comment.

The order in the modules list is maintained by changing the llvm::StringMap to map from module name to "filtered_modules" index. This avoids having to iterate across the StringMap in the end and make the filtered_modules in a different ordering.

Looks good to me. Jim should ok as well.

In D55356#1327242, @labath wrote:

In D55356#1327224, @clayborg wrote:

In D55356#1327099, @labath wrote:

Actually, this now causes an lldb-mi test to fail, but it's not clear to me if the problem is in the test, or this patch. This issue happens when lldb-mi is printing the "library loaded" message after a module gets added to a not-yet-running target. It tries to print the load address by first getting the base address and then converting that to a load address.

Before this patch, that would always fail, because well.. ELF and PECOFF had this function unimplemented, and for MachO the base address was section-relative, and so it wasn't resolved to a load address without the section being loaded. However, with this patch, in the ELF (and presumably PECOFF) case, the load address is not section-relative and so the GetLoadAddress function happily returns the address.

Is this the expected behavior here? (i.e., object_file->GetLoadAddress().GetLoadAddress(target) returning a valid value even though the target is not running)

Not unless someone has manually set the section load address in the test?

The test is not setting the load address manually. This simply falls out of how Address::GetLoadAddress is implemented:

addr_t Address::GetLoadAddress(Target *target) const {
  SectionSP section_sp(GetSection());
  if (section_sp) {
    ...
  } else {
    // We don't have a section so the offset is the load address
    return m_offset;
  }
}

So, where's the bug here? It's not clear to me how to change Address::GetLoadAddress to do something different than what it is doing now.

In D55356#1327224, @clayborg wrote:

In D55356#1327099, @labath wrote:

Actually, this now causes an lldb-mi test to fail, but it's not clear to me if the problem is in the test, or this patch. This issue happens when lldb-mi is printing the "library loaded" message after a module gets added to a not-yet-running target. It tries to print the load address by first getting the base address and then converting that to a load address.

Before this patch, that would always fail, because well.. ELF and PECOFF had this function unimplemented, and for MachO the base address was section-relative, and so it wasn't resolved to a load address without the section being loaded. However, with this patch, in the ELF (and presumably PECOFF) case, the load address is not section-relative and so the GetLoadAddress function happily returns the address.

Is this the expected behavior here? (i.e., object_file->GetLoadAddress().GetLoadAddress(target) returning a valid value even though the target is not running)

Not unless someone has manually set the section load address in the test?

In D55522#1326955, @labath wrote:

It just occurred to me that we have another copy of /proc/<pid>/maps -> MemoryRegionInfo conversion code. It lives in NativeProcessLinux.cpp (ParseMemoryRegionInfoFromProcMapsLine). It would be nice to extract this parser into a some place (Plugins/Process/Utility ?), where it can be shared by the two process plugins. As a bonus, that should make it easy to write unit tests for that function.

In D55356#1327099, @labath wrote:

Actually, this now causes an lldb-mi test to fail, but it's not clear to me if the problem is in the test, or this patch. This issue happens when lldb-mi is printing the "library loaded" message after a module gets added to a not-yet-running target. It tries to print the load address by first getting the base address and then converting that to a load address.

Before this patch, that would always fail, because well.. ELF and PECOFF had this function unimplemented, and for MachO the base address was section-relative, and so it wasn't resolved to a load address without the section being loaded. However, with this patch, in the ELF (and presumably PECOFF) case, the load address is not section-relative and so the GetLoadAddress function happily returns the address.

Is this the expected behavior here? (i.e., object_file->GetLoadAddress().GetLoadAddress(target) returning a valid value even though the target is not running)

FYI: my approach to getting symbols to load was a bit different. I always let a simple PlaceholderModule be created, but I played some tricks in the GetObjectFile() method if someone had setting symbols for the module with "target symbols add ..". I will attach my PlaceholderModule so you can see what I had done. Since these modules always must be associated with a target, I keep a weak pointer to the target in the constructor. Then later, if someone does "target symbols add ..." the module will have Module:: m_symfile_spec filled in, so I set the m_platform_file to be m_file, and then move m_symfile_spec into m_file and then call Module::GetObjectFile(). This is nice and clean because you don't have to make up fake symbols to fake sections. When you create the placeholder module it needs the target:

auto placeholder_module =
    std::make_shared<PlaceholderModule>(module_spec, GetTarget());

Add reserve and shrink to fit when parsing memory regions.

In D55522#1326035, @tatyana-krasnukha wrote:

Don't you mind to take overridden GetMemoryRegions in this patch so that I can remove all minidump-related changes from D55472?

In D55522#1325839, @labath wrote:

This patch certainly provides a more comprehensive treatment of memory region information in minidump. However, there might be some value in removing the excessive shared_ptrs in the other patch. I think that should be evaluated separately.

Fixed Zach's issues.

Removed Xcode scheme changes.

BTW: check my changes in: https://reviews.llvm.org/D55522
It will be interesting to you since it parses the linux maps info if it is available in breakpad generated minidump files. This will give us enough info to create correct sections for object files when we have no ELF file or no symbol file (ELF/DWARF or breakpad).

In D55434#1325739, @labath wrote:

In D55434#1323912, @clayborg wrote:

If you plan on not making the breakpad file ever stand alone, then you will need to take any addresses and look them up in the module section list and use the other sections. I don't see why the breakpad file can't be stand alone though. It won't be as accurate, but it sure would be nice to be able to load a bunch of them into LLDB without needing to find the original executable and just symbolicate no?

I could try to make it stand-alone, but that seems to me like a duplication of effort. And since the sections I could conjure up from the breakpad info would never match the original elf file, I would have to support both cases anyway, one with using the sections from the object file, and one with the own, made-up sections.

I do intend to support both cases, but in a slightly different way. The way, I see it we have actually four cases to consider:

1. we have an stripped elf file, and a breakpad symbol file (the case of an unstripped elf file is uninteresting, as it will have much better debug info than breakpad can possibly provide)
2. we don't have an elf file, but we have a breakpad file
3. we don't have an elf file nor a breakpad file
   
   Because of case 3, we have to do some section conjuring independently of any breakpad file. We already do that to some extent, and @lemo is getting ready to extend that in D55142. Once we have that, and we find a breakpad file for this module (case 2), the breakpad file should be able to just latch onto the sections created for the placeholder object file. And I believe ProcessMinidump is in a better position to create the "placeholder" sections, as it has both access to the load addresses and sizes of the modules. From breakpad info, it would be hard to determine the size of the module if the highest address is occupied by a "PUBLIC" symbol, as they don't have any sizes associated with them.
   
   Going back to case 1, if we have a stripped elf file, the breakpad file should latch onto the sections of that one without ever knowing the difference. So in the end, I hope this will produce a clearer code because the concerns will be separated. Breakpad code will always deal with externally-provided sections, regardless of whether they come from a "real" object file, or a made-up one. And the "making-up" code can work independently of there being a breakpad file.

Add sorting functions needed for MemoryRegionInfo

See my patch: https://reviews.llvm.org/D55522

I am going to stop making comments as I have been working on a similar patch that does more than this one. I will post it today and we can decide which approach we want to use.

If you plan on not making the breakpad file ever stand alone, then you will need to take any addresses and look them up in the module section list and use the other sections. I don't see why the breakpad file can't be stand alone though. It won't be as accurate, but it sure would be nice to be able to load a bunch of them into LLDB without needing to find the original executable and just symbolicate no?

So we do something like you describe with the DYSM files. The object file is "a.out" and it has a dSYM file "a.out.dSYM/Context/Resources/DWARF/a.out" and the dSYM file will share sections with the "a.out" object file. So if you plan on loading the breakpad file as a symbol file that just needs some sections that it can give to the debug info it will eventually create, these sections must be the same between the binary and the breakpad object file. So I would still recommend trying to make sections that make sense like a real object file. It is also nice to be able to live off of the breakpad file itself. In this case the ObjectFile for breakpad when it is stand alone should do as good of a job as possible.