Must be relocations causing the stuff to not match up to the llvm-dwarfdump output.

FYI: This might be because we are using a .o file and relocations are being applied internally!?

Fix the error message to check for the exact error message and this is good to go.

Is there no test case catching this?

LGTM

Looks good to me.

I would suggest checking the google stadia patch for the L1 and L2 caches:

One other optimization we can do is if we read from the process memory and it returns that is read zero bytes, right now we add the range we were trying to read into the m_invalid_ranges member variable. So lets say we were trying to read the range [0x1000-0x2000) on a mac. We will fail to read this due to __PAGEZERO, but I believe we currently add this range to the m_invalid_ranges. But we could ask about this memory region from the process and realize we can actually add [0x0-0x100000000) to the m_invalid_ranges. That might help avoid multiple bad reads from a large area that isn't mapped.

There was a fix that was never submitted for Google Stadia for the memory cache here:

Thanks for the changes! LGTM. Just one missed EXPECT_THAT_EXPECTED, but accepted.

Looks good. Only questions is if we can add a C++ unit test for this file and test the new Symbol::FromJSON() and test all error conditions.

This is a resubmission of https://reviews.llvm.org/D143793. I added fixes for the buildbots

Add fixes for buildbots where some compilers don't need a std::move(), but some do.

Failed to "arc diff" to update an older patch.

If these files can be used as the only source of information (without a stripped executable), we really should include a serialized SectionList in the JSON that can be loaded into ObjectFileJSON. This would be very useful for easily creating unit tests.

Just change HandleDestroryCallback to a member function and this is good to go.

I like this idea of this, but I would like to see this be a bit more complete. One idea is to remove the ObjectFileJSON::Symbol definition and just use lldb_private::Symbol objects and allow these objects to construct encode and decode from JSON. Then we have the ability to re-create any symbols we need in full fidelity. But that not be the goal in your case, but I don't think it would hurt to use the lldb_private::Symbol as the object we serialize and deserialize from JSON as ObjectFileJSON::Symbol just can't reproduce the full depth of the symbols we want.

Sorry for delay, I have been on vacation for the last two weeks. I will be back next Monday and get to this soon. Feel free to ping again next week if I don't get to it!

Address review comments:

• createSegment now can return an error if the segment size is too small
• added code to detect segment sizes that are too small to contain even a single function info
• added test code to test for small segment sizes

If anyone doesn't know the GSYM code that I added as reviewers, feel free to resign. I just did some blame lines on GSYM code to search for people that have made some fixes to the code since there aren't a lot of people that know the GSYM code!

In D142926#4116417, @JDevlieghere wrote:

In D142926#4115980, @clayborg wrote:

In D142926#4115875, @JDevlieghere wrote:

In D142926#4114215, @bulbazord wrote:

Not sure how useful it would be but I recorded the full list of methods get added with this change. Take a look and let me know if there are any that you think shouldn't be added.

Thanks for generating that list. Given that the reproducers are not functional, let's not add these to the Python API:

SBReproducer::Capture();
SBReproducer::Replay(const char *);
SBReproducer::Replay(const char *, bool);
SBReproducer::Finalize(const char *);
SBReproducer::GetPath();
SBReproducer::Generate();

Are they not functional from native code? or just not functional from Python?

The feature was removed but for ABI stability reasons we can't remove them from the SB API. Most/all of them return an error saying the feature has been removed.

In D142926#4115875, @JDevlieghere wrote:

In D142926#4114215, @bulbazord wrote:

Not sure how useful it would be but I recorded the full list of methods get added with this change. Take a look and let me know if there are any that you think shouldn't be added.

Thanks for generating that list. Given that the reproducers are not functional, let's not add these to the Python API:

SBReproducer::Capture();
SBReproducer::Replay(const char *);
SBReproducer::Replay(const char *, bool);
SBReproducer::Finalize(const char *);
SBReproducer::GetPath();
SBReproducer::Generate();

Looks good to me. Pavel?

In D142926#4111717, @bulbazord wrote:

Potentially interesting:

• SBData::GetDescription base_addr parameter has default value now
• SBInstructionList::GetInstructionsCount canSetBreakpoint has default value now
• SBMemoryRegionInfo::SBMemoryRegionInfo 3rd constructor parameter stack_memory has default value now

In D142926#4111717, @bulbazord wrote:

In addition to what I wrote above, I also fixed several documentation bugs (putting docstrings on the wrong method).

I manually audited the generated code before and after this change. Here are my notes:

Not too interesting:

• Some parameter names changed
• Fixed lots of documentation bugs
• Some documentation changed because of a re-ordering of function declarations

Potentially interesting:

• SBData::GetDescription base_addr parameter has default value now
• SBInstructionList::GetInstructionsCount canSetBreakpoint has default value now
• SBMemoryRegionInfo::SBMemoryRegionInfo 3rd constructor parameter stack_memory has default value now

Certainly interesting:

• SBListener::StopListeningForEventClass return type conflicts (ABI break?)

Very clean patch now, just a few nits about asserts!

If I am reading the code for this patch correctly, we need the BatonSP stuff because we have differing callback bytes for public vs private APIs. If we switch to using a common callback type of:

typedef void (*DebuggerDestroyCallback)(lldb::user_id_t &debugger_id, void *baton);

(we can define this both internally and leave the current "SBDebuggerDestroyCallback" in SBDefines.h alone), then we can avoid having to add any of the BatonSP stuff and just store a "void * m_destroy_callback_baton;" in Debugger.h.

Changes look fine to me. I would like someone that specializes in the expression parser to give the final ok though.

• My main source of frustration was that my concern is getting overlooked/ignored (not necessarily your fault -- I've been told I am not always sufficiently clear). I think that is something we could live with, if we thing the other cleanups in this patch are worth it (which could very well be the case) -- however, I would want us to be clear that's what we're doing.

In D137139#4091685, @dblaikie wrote:

In D137139#4091500, @clayborg wrote:

In D137139#4091496, @dblaikie wrote:

In D137139#4091399, @clayborg wrote:

In D137139#4091380, @dblaikie wrote:

In D137139#4090779, @ayermolo wrote:

To me it sounds it's easier to just fix it in llvm, no?

Though that doesn't address the concern @clayborg that lldb will still misbehave on existing compiled code?

Which is ok, this is to fix the compiler. I can make separate LLDB changes to address the reality of the situation.

What sort of change to LLDB are you thinking of?

using DW_AT_ranges instead of .debug_aranges if they are available.

So then LLDB wouldn't use this .debug_aranges data this patch fixes, right? So could we remove this .debug_aranges data instead?

So looks like this function needs to be fixed:

llvm::Optional<DIERef> DWARFBaseDIE::GetDIERef() const {
  if (!IsValid())
    return llvm::None;

In D138618#4086789, @labath wrote:

I'm sorry, but that patch does not fix the problem I am trying to point out. In fact, I think it makes things a lot worse.

We clearly have some kind of a communication problem, but I am running out of ideas of what can I do about it. Let me try rephrasing it one more time:

• this patch creates two path for converting a DIERef to a user_id_t -- a) ref.get_id(); and b) dwarf.GetUID(ref)
• of those two ways, one is clearly more intuitive
• of those two ways, one is always correct
• those two ways aren't the same -- (a) is simpler; (b) is correct
• you can't fix that by simply taking (b) away. All that does is make the API misuse even more likely. That patch essentially just deletes GetUID, and inlines it into all its callers.

Forget about the what the code does for a moment, and tell me which of these snippets looks better:
i)

if (IsValid())
  return GetDWARF()->GetUID(*this);

ii)

const std::optional<DIERef> &ref = this->GetDIERef();
if (ref)
  return DIERef(GetID(), ref->section(), ref->die_offset()).get_id();

iii)

if (IsValid())
  return GetDIERef()->get_id();

Now look up the implementation and tell me which one is correct.

Looks good to me. Pavel?

In D137139#4091496, @dblaikie wrote:

In D137139#4091399, @clayborg wrote:

In D137139#4091380, @dblaikie wrote:

In D137139#4090779, @ayermolo wrote:

To me it sounds it's easier to just fix it in llvm, no?

Though that doesn't address the concern @clayborg that lldb will still misbehave on existing compiled code?

Which is ok, this is to fix the compiler. I can make separate LLDB changes to address the reality of the situation.

What sort of change to LLDB are you thinking of?

In D137139#4091380, @dblaikie wrote:

In D137139#4090779, @ayermolo wrote:

To me it sounds it's easier to just fix it in llvm, no?

Though that doesn't address the concern @clayborg that lldb will still misbehave on existing compiled code?

In D137139#4084505, @dblaikie wrote:

In D137139#4084468, @clayborg wrote:

If the only time clang is not emitting .debug_aranges correctly is when we use -g1, then fixing that issue (by either fixing or removing .debug_arnages) would be fine by me. That being said, we have a clang that has been producing bad .debug_aranges, so unless we fix LLDB it won't fix the issue for binaries already out in the wild.

What sort of fix for LLDB are you picturing? One way to fix LLDB would be to teach it to ignore .debug_aranges - which would hurt perf on inputs with correct .debug_aranges but no CU ranges, but we don't seem to have anyone clamouring for support for that scenario.

Updated to latest sources and fixed issues from a recently added test for simple template types that showed some issues in type lookup. We previously would allow results from a lookup like "Foo" to match a type that was in the accelerator ables as "Foo", but for a type that was really "Foo<int>". This latest addition to the patch fixes these lookups from incorrectly succeeding.

Much better, thanks for making sure we don't end up indexing real skeleton units as they cause problems if they do get indexed.

If the only time clang is not emitting .debug_aranges correctly is when we use -g1, then fixing that issue (by either fixing or removing .debug_arnages) would be fine by me. That being said, we have a clang that has been producing bad .debug_aranges, so unless we fix LLDB it won't fix the issue for binaries already out in the wild.

As long as there are no regressions in the test suite this looks good to me

In D137139#3905859, @dblaikie wrote:

In D137139#3900161, @clayborg wrote:

In D137139#3900042, @ayermolo wrote:

In D137139#3899179, @dblaikie wrote:

This is the sort of thing I kind of wanted to avoid when we were talking about adding the aranges verifier - fixing bugs in aranges when instead we should be fixing bugs in consumers to not depend on aranges.

But anyway - if you really want/need aranges, perhaps a deeper fix would be good? We could emit aranges as CU ranges + data ranges instead of keeping an entirely separate list that can get out of sync? (& while we're at it, probably move the data ranges to the CU - rather than having to map them back from another independent data structure in DwarfDebug?)

I totally agree... Trying to see if we can migrate internal tool from using aranges. Although considering this is a bit of a "special case" with -g1, might be OK just let it be for that tool.

You have a good point. @clayborg can we just change lldb to ignore this section?

This is the first time I have been aware that .debug_aranges was so unreliable when we identified the root cause of this issue. I am totally fine and I will modify LLDB to ignore this section when it can. If we have valid indexes like .debug_names though, it would be a shame, if each compile unit doesn't have a DW_AT_ranges, to have to manually index the DWARF just to figure out the aranges for a compile unit, so I might let LLDB rely on this section if it is available, but only if the compile unit has no valid ranges.

I agree in the abstract, but are there any producers you know of that don't produce CU ranges (& also do produce aranges) that this would be intended to support?

We just need to create all DIERef objects using the GetID() from the symbol file as the file index, and we should be able to remove the SymbolFile::GetUID() function now. As long as file index zero is reserved for "vanilla DWARF that doesn't use DWO or OSO we will know the difference. We might want to not have SymbolFileDWARF inherit from UserID at all, and switch over to have SymbolFileDWARF add a virtual function:

uint32_t m_file_index = 0; // Zero means main DWARF file, 1...N identifies the Nth DWO file or OSO file
virtual uint32_t GetFileIndex() { return m_file_index; }

In D138618#4080706, @labath wrote:

In D138618#4077851, @ayermolo wrote:

In D138618#4073329, @labath wrote:

I think that the 1-based index thingy helps a lot here, but I haven't seen anything (in your reponse, or in the new patch) that would address my concernt DIERef<->user_id conversion ambiguity. I.e. how is one supposed to know what is the "right" way to convert a DIERef to a user_id:

• die_ref.get_id()
• or symbol_file.GetUID(die_ref) (which, funnily enough, will construct another DIERef, and *then* call get_id? (return DIERef(GetID(), ref.section(), ref.die_offset()).get_id();)

What's your position on that? That we should live with the ambiguity?

Searching for GetUID doesn't look like it's used all that often, maybe follow up patch is just to get rid of it, and replace with DIERef?

If you could make that work, that would be awesome, but I think that's going to be fairly hard. It's true that there aren't that many call sites of this functions, but the ones that are there are very crucial. The user_id_t type represents a symbol-file-neutral identifier (cookie, if you will) that different symbol file implementations use to identify parsed objects (types, mostly). SymbolFileDWARF uses it (via DIERef et al.) to identify the DIE belonging to that type. PDB symbol files use it differently, but the idea is the same. If we wanted to remove that, we'd have to come up with a whole new way to parse/link types -- and one that would work for non-dwarf symbol files as well.

Gotcha, makes sense now. Thanks for the extra info. We do need to make sure the m_symbol_file values match and return an empty TypeSP if they don't. Once that is done, this is good to go.

In D142052#4067177, @augusto2112 wrote:

@clayborg the intended usage here is to create a copy of the type in the same symbol file. I could add a sanity check that makes sure we're not creating a copy of something that isn't in the symbol file's type list in debug mode. Would that be enough?

We don't allow types to be copied into another SymbolFile. There is a strict rule in LLDB that SymbolFile objects only create types from their own data. There are many reasons we don't want this:

• Type objects have a m_symbol_file which points to the SymbolFile that created it. This pointer can and will go bad if the original symbol file is freed.
• Type objects have a symbol context member variable ( "SymbolContextScope *m_context;"), same issue as above
• Type objects might have a valid encoding type member variable ("Type *m_encoding_type;") that is non NULL that points to a type that is owned by the original symbol file
• Type objects might have a encoding User ID and encoding type that point to the original symbol file ( lldb::user_id_t m_encoding_uid = LLDB_INVALID_UID; EncodingDataType m_encoding_uid_type = eEncodingInvalid;) and the user_id_t only makes sense in the original encoding type.

Change looks good. It would be nice to test this if possible.

Update: There are some new failures after someone recently added new tests that involve simple template types. I need to fix my new lookup routine to do the right thing in light of these new tests. The main issue is currently if you have a "foo<int>", the accelerator tables and DWARF contain "foo" only. This causes the expression parser to be able to lookup "foo" as it is parsing an expression that has "auto a = foo<int>()" as it will actually return a specialized "foo<int>" as the result since the accelerator table will point to the "foo<int>" type. We need to outlaw these raw lookups from working. Worse yet, if there are multiple instantiations of "foo<T>", it will return all of them and cause LLDB to crash later in some AST copying code.

In D138618#4004866, @labath wrote:

I think that the main reason we've arrived at such different conclusions is that I treat the "user IDs of DIEs" and and "user IDs of symbol files" as essentially two different things (namespaces if you will). Obviously, one needs the symbol file ID in order to compute the DIERef ID, but theoretically those two can use completely different encodings. With this take on things, I stand by my assertion that DIERef->user_id conversions are tightly controlled. The symbol file ID computations are a mess.

You, if I understand correctly, see the ID of a symbol file as a special case of an all-encompassing user id -- essentially a user_id (or a DIERef) pointing to the first byte of the symbol file. with this world view, the entirety of user ID computation is a mess. :)
I can definitely see the appeal of viewing the world that way. It's nice and uniform and unambiguous (since you can't have a DIE at offset zero) -- it's just not the view I had when I was writing this code a couple of years ago. :) And it has the disadvantage of obscuring the DIERef->user_id transition (for DIEs at least), and that's what I'm weight against the other advantages of that approach.

In D140630#4056641, @cimacmillan wrote:

@clayborg Thanks for your feedback. I'm part the way through implementing your changes. Specifically about this point:

I seem to remember that it will disable this watchpoint as soon as a local variable goes out of scope, though I might not be remember things correctly

This is a behaviour I'd like to address, where for instance watchpoints are triggered in different functions because the stack frame addresses align. I have this example, I can add a test for:

int test_a() {
    int x = 10; <- watchpoint set here
    x = 20; <- watchpoint triggered here
}

int test_b() {
    int b = 10; <- watchpoint triggered here also
    b = 20;
}

int main() {
    test_a();
    test_b();

I've refactored the Watchpoint class to use "SBValue::Watch", and I can still reproduce this behaviour. I also can't find where this watchpoint disable on scope change might be implemented. Do you have any suggestions for this? Thanks

Looks fine to me as is seems performant. I will let owners of the DWARF linker approve for good.

Sorry for the delay!

New changes looks great, thanks for tackling this. Just a few comments that need to be updated and this will be good to go as long as the test suite passes just fine!

In D68655#4048009, @jasonmolenda wrote:

In D68655#4047126, @labath wrote:

In D68655#4045895, @jasonmolenda wrote:

Both have to be written by the dwarf linker to be correct, but only the former is written ONLY by the dwarf linker.

I don't think that's right:

$ clang -c -x c - -o - -gdwarf-aranges -g <<<"void f(){}" | llvm-readelf - --sections | grep aranges
  [ 6] .debug_aranges    PROGBITS        0000000000000000 0000a0 000030 00      0   0  1
  [ 7] .rela.debug_aranges RELA          0000000000000000 000380 000030 18   I 20   6  8

Ah thanks Pavel, clang on Darwin doesn't emit this in .o files. So in this case, the .o file has a DW_TAG_compile_unit with a DW_AT_ranges and a .debug_aranges, both generated pre-link-time by the compiler. And Greg is saying that the DW_AT_ranges list in the final executable will be better than the .debug_aranges? I don't know how strongly he asserts this - today if a .debug_aranges has an entry, we don't look at DW_TAG_compile_unit's DW_AT_ranges, or create the ranges ourself via the line table; the debug_aranges is trusted above the other methods if it includes CU.

In D68655#4045873, @jasonmolenda wrote:

I know this is all moot because the dSYM-specific patch landed, but I am curious about this part,

In D68655#4045561, @clayborg wrote:

Different things are included in DW_AT_ranges, like address ranges for global and static variables. .debug_aranges only has functions, no globals or statics, so if you are trying to find a global variable by address, you can't rely on .debug_aranges. Nothing in the DWARF spec states things clearly enough for different compilers to know what to include in .debug_aranges and the compiler uint DW_AT_ranges.

The standard says,

"Each descriptor is a triple consisting of a segment selector, the beginning address within that segment of a range of text or data covered by some entry owned by the corresponding compilation unit, followed by the non-zero length of that range"

It is pretty clear on the point that any part of the address space that can be attributed to a compile_unit can be included in the debug_aranges range list - if only code is included, that's a choice of the aranges writer. lldb itself, if debug_aranges is missing or doesn't include a CU, steps through the line table concatenating addresses for all the line entries in the CU (v. DWARFCompileUnit::BuildAddressRangeTable ) - it doesn't include data. (it will also use DW_AT_ranges from the compile_unit but I worry more about this being copied verbatim from the .o file into the linked DWARF than I worry about debug_aranges, personally)

In D68655#4045604, @JDevlieghere wrote:

In D68655#4045561, @clayborg wrote:

But we still need know if we have a dSYM file or not, because if not, we can't use .debug_aranges as .o files don't have them, but they also don't claim to be of type ObjectFile::eTypeDebugInfo.

I came to the same realization and fixed that in https://reviews.llvm.org/rG9b737f148d88501a0a778e1adacf342108286bb0

Looks good!

I mean we can not just subtract something, any number, from any address unless we have fixed size opcodes. If we do this for x86, you can get complete garbage with no hope of ever getting back on track and this disassembly just won't make sense at all and will be useless. I thought my x86 example spelled out why it is bad to backup. If we start disassembling in the middle of an opcode, we can attempt to disassemble immediate values that are encoded into the middle of an opcode. Since x86 instructions can be 1 byte to 15 bytes, we might disassembly garbage and never align up to real opcodes.

In D68655#4045461, @jasonmolenda wrote:

In D68655#4045423, @clayborg wrote:

There have also been some bugs in .debug_aranges and some folks want to get rid of .debug_aranges all together and rely only upon the DW_TAG_compile_unit's DW_AT_ranges. Tons of details in this patch:

https://reviews.llvm.org/D136395

So long story short, some people believe we should not produce or use .debug_aranges anymore, and want LLDB changed to ignore it.

I'm honestly curious how the DW_TAG_compile_unit's DW_AT_ranges (or DW_AT_high/low_pc) would be authoritative, but the ranges for the CU in debug_aranges would be incorrect. If the linker is allowed to reorder functions (does this not happen on linux?), then the compiler can't know the address ranges of the CU, only the linker (or post-linker) can know what address ranges are owned by this compile unit. So the linker had to create the DW_TAG_compile_unit's DW_AT_ranges list after linking is complete, and the linker is also the part of the toolchain that creates debug_aranges. If it is creating an incorrect debug_aranges entry, how can it possibly do the correct thing when rewriting the DW_TAG_compile_unit's DW_AT_ranges?

In D141318#4045459, @augusto2112 wrote:

In D141318#4045372, @clayborg wrote:

Each SymbolFile has its own type list that keeps the shared pointers to all types. So there should be no need for any of these changes here unless someone isn't correctly storing a newly created type in the SymbolFile's type list. You can see the types being stored in code like:

TypeSP type_sp(... /* create type here*/ ...);
dwarf->GetTypeList().Insert(type_sp); // Store the share reference in the symbol file's type list

What triggered the use after free for me was this snippet in DWARFASTParserClang::ParseTypeModifier:

  ...
  TypeSP lldb_function_type_sp = ParseTypeFromDWARF(
      sc, function_type, &function_type_is_new_pointer);

  if (lldb_function_type_sp) {
    clang_type = m_ast.CreateBlockPointerType(
        lldb_function_type_sp->GetForwardCompilerType());
    encoding_data_type = Type::eEncodingIsUID;
    attrs.type.Clear();
    resolve_state = Type::ResolveState::Full;
  }
}

lldb_function_type_sp is created, the underlying pointer is placed in the map, the SP is destroyed at the end of the scope, and any lookups for that type will get back the dangling pointer.

I could update this particular instance, but this just seems like a really easy mistake to make... There's nothing to suggest to callers that they must update the m_type_list when they ask for a new type. Wouldn't it be better we prevent this somehow, by updating the raw pointers of the map by being either unique, shared or weak pointers?

In D140358#4045442, @eloparco wrote:

In D140358#4045210, @clayborg wrote:

Are you planning on updating the reverse disassembly code still on this patch?

What do you mean by "reverse disassembly"?

In D68655#4045420, @clayborg wrote:

SymbolVendorELF.cpp is using this as well:

dsym_objfile_sp->SetType(ObjectFile::eTypeDebugInfo);

They must have copied and pasted some code. So not sure this is safe enough as is to implement. There are also a few places that try to check for a dSYM file using this kind of method, might be nice to add a method, that is done right, that detects if we really have a dSYM or not in ObjectFile.h/.cpp. Wasm also sets the file type to debug info and so does breakpad.

There have also been some bugs in .debug_aranges and some folks want to get rid of .debug_aranges all together and rely only upon the DW_TAG_compile_unit's DW_AT_ranges. Tons of details in this patch:

SymbolVendorELF.cpp is using this as well:

dsym_objfile_sp->SetType(ObjectFile::eTypeDebugInfo);

They must have copied and pasted some code. So not sure this is safe enough as is to implement. There are also a few places that try to check for a dSYM file using this kind of method, might be nice to add a method, that is done right, that detects if we really have a dSYM or not in ObjectFile.h/.cpp. Wasm also sets the file type to debug info and so does breakpad.

Storing raw pointers in DieToTypePtr may cause use-after-frees to occur, since there are no guarantees that the shared pointers that owns the underlying pointer to the type are kept around as long as the map. Change the map to store a shared pointer instead.

Each SymbolFile has its own type list that keeps the shared pointers to all types. So there should be no need for any of these changes here unless someone isn't correctly storing a newly created type in the SymbolFile's type list. You can see the types being stored in code like:

TypeSP type_sp(... /* create type here*/ ...);
dwarf->GetTypeList().Insert(type_sp); // Store the share reference in the symbol file's type list