Page MenuHomePhabricator

clayborg (Greg Clayton)
User

Projects

User does not belong to any projects.

User Details

User Since
Sep 23 2014, 10:11 AM (443 w, 2 d)

Recent Activity

Today

clayborg accepted D146659: [LLDB] Fix for D139955 Summary:.

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

Thu, Mar 23, 12:58 PM · Restricted Project, Restricted Project
clayborg added a comment to D146659: [LLDB] Fix for D139955 Summary:.

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

Thu, Mar 23, 11:05 AM · Restricted Project, Restricted Project
clayborg added inline comments to D146659: [LLDB] Fix for D139955 Summary:.
Thu, Mar 23, 11:00 AM · Restricted Project, Restricted Project
clayborg added a comment to D146659: [LLDB] Fix for D139955 Summary:.

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

Thu, Mar 23, 10:37 AM · Restricted Project, Restricted Project

Yesterday

clayborg added a comment to D146659: [LLDB] Fix for D139955 Summary:.

Is there no test case catching this?

Wed, Mar 22, 2:29 PM · Restricted Project, Restricted Project

Fri, Mar 17

clayborg added a comment to D145624: [lldb] Make MemoryCache::Read more resilient.

LGTM

Fri, Mar 17, 4:40 PM · Restricted Project, Restricted Project

Mon, Mar 13

clayborg added inline comments to D145624: [lldb] Make MemoryCache::Read more resilient.
Mon, Mar 13, 10:50 AM · Restricted Project, Restricted Project
clayborg accepted D145955: Refactor ObjectFilePlaceholder for sharing.
Mon, Mar 13, 10:49 AM · Restricted Project, Restricted Project

Fri, Mar 10

clayborg accepted D145805: [DWARFLinker][DWARFv5] Add support for DW_FORM_addrx*.

Looks good to me.

Fri, Mar 10, 9:41 AM · Restricted Project, Restricted Project

Thu, Mar 9

clayborg added a comment to D145624: [lldb] Make MemoryCache::Read more resilient.

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

Thu, Mar 9, 11:59 AM · Restricted Project, Restricted Project
clayborg added a comment to D145624: [lldb] Make MemoryCache::Read more resilient.

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.

Thu, Mar 9, 11:58 AM · Restricted Project, Restricted Project
clayborg accepted D145637: [dsymutil] Fix offset calculation for fat binaries.
Thu, Mar 9, 9:52 AM · Restricted Project, Restricted Project

Wed, Mar 8

clayborg added a comment to D145624: [lldb] Make MemoryCache::Read more resilient.

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

Wed, Mar 8, 5:48 PM · Restricted Project, Restricted Project
clayborg accepted D145180: [lldb] Introduce new SymbolFileJSON and ObjectFileJSON.

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

Wed, Mar 8, 5:28 PM · Restricted Project, Restricted Project
clayborg added inline comments to D145180: [lldb] Introduce new SymbolFileJSON and ObjectFileJSON.
Wed, Mar 8, 3:06 PM · Restricted Project, Restricted Project

Tue, Mar 7

clayborg added a comment to D145180: [lldb] Introduce new SymbolFileJSON and ObjectFileJSON.

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.

Tue, Mar 7, 4:00 PM · Restricted Project, Restricted Project
clayborg added inline comments to D145180: [lldb] Introduce new SymbolFileJSON and ObjectFileJSON.
Tue, Mar 7, 2:45 PM · Restricted Project, Restricted Project
clayborg added inline comments to D145136: Add a Debugger interruption mechanism in parallel to the Command Interpreter interruption.
Tue, Mar 7, 2:21 PM · Restricted Project, Restricted Project
clayborg accepted D143520: Add a new SBDebugger::SetDestroyCallback() API.
Tue, Mar 7, 12:02 PM · Restricted Project, Restricted Project

Mon, Mar 6

clayborg committed rGd8e077e2caeb: Add the ability to segment GSYM files. (authored by clayborg).
Add the ability to segment GSYM files.
Mon, Mar 6, 4:09 PM · Restricted Project, Restricted Project
clayborg closed D145448: Add the ability to segment GSYM files..
Mon, Mar 6, 4:08 PM · Restricted Project, Restricted Project
clayborg added a comment to D145448: Add the ability to segment GSYM files..

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

Mon, Mar 6, 4:03 PM · Restricted Project, Restricted Project
clayborg updated the diff for D145448: Add the ability to segment GSYM files..

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

Mon, Mar 6, 4:01 PM · Restricted Project, Restricted Project
clayborg requested review of D145448: Add the ability to segment GSYM files..
Mon, Mar 6, 3:58 PM · Restricted Project, Restricted Project
clayborg abandoned D145445: Try to fix buildbot issues where some compilers can't auto move objects and most can..

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

Mon, Mar 6, 3:49 PM · Restricted Project, Restricted Project
clayborg requested review of D145445: Try to fix buildbot issues where some compilers can't auto move objects and most can..
Mon, Mar 6, 3:48 PM · Restricted Project, Restricted Project
clayborg added inline comments to D145180: [lldb] Introduce new SymbolFileJSON and ObjectFileJSON.
Mon, Mar 6, 3:35 PM · Restricted Project, Restricted Project
clayborg added inline comments to D145180: [lldb] Introduce new SymbolFileJSON and ObjectFileJSON.
Mon, Mar 6, 3:06 PM · Restricted Project, Restricted Project
clayborg added inline comments to D145180: [lldb] Introduce new SymbolFileJSON and ObjectFileJSON.
Mon, Mar 6, 2:41 PM · Restricted Project, Restricted Project
clayborg added a comment to D145180: [lldb] Introduce new SymbolFileJSON and ObjectFileJSON.

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.

Mon, Mar 6, 2:39 PM · Restricted Project, Restricted Project
clayborg added a comment to D143520: Add a new SBDebugger::SetDestroyCallback() API.

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

Mon, Mar 6, 9:47 AM · Restricted Project, Restricted Project

Thu, Mar 2

clayborg added inline comments to D145136: Add a Debugger interruption mechanism in parallel to the Command Interpreter interruption.
Thu, Mar 2, 9:27 PM · Restricted Project, Restricted Project
clayborg accepted D145203: Add HitCount into Breakpoint statistics.
Thu, Mar 2, 9:15 PM · Restricted Project, Restricted Project
clayborg added a comment to D145180: [lldb] Introduce new SymbolFileJSON and ObjectFileJSON.

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.

Thu, Mar 2, 9:11 PM · Restricted Project, Restricted Project
clayborg committed rGfe758254181a: Add the ability to segment GSYM files. (authored by clayborg).
Add the ability to segment GSYM files.
Thu, Mar 2, 8:40 PM · Restricted Project, Restricted Project
clayborg closed D143793: Add the ability to segment GSYM files..
Thu, Mar 2, 8:40 PM · Restricted Project, Restricted Project

Thu, Feb 23

clayborg added a comment to D140630: [lldb-vscode] Add data breakpoint support.

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!

Thu, Feb 23, 1:41 PM · Restricted Project, Restricted Project
clayborg added inline comments to D143793: Add the ability to segment GSYM files..
Thu, Feb 23, 1:34 PM · Restricted Project, Restricted Project
clayborg updated the diff for D143793: Add the ability to segment GSYM files..

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
Thu, Feb 23, 1:32 PM · Restricted Project, Restricted Project

Feb 10 2023

clayborg added a comment to D143793: Add the ability to segment GSYM files..

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!

Feb 10 2023, 4:28 PM · Restricted Project, Restricted Project
clayborg added reviewers for D143793: Add the ability to segment GSYM files.: urnathan, MaskRay.
Feb 10 2023, 4:23 PM · Restricted Project, Restricted Project
clayborg requested review of D143793: Add the ability to segment GSYM files..
Feb 10 2023, 4:17 PM · Restricted Project, Restricted Project

Feb 9 2023

clayborg added a comment to D142926: [lldb] Replace SB swig interfaces with API headers.

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.

Feb 9 2023, 2:00 PM · Restricted Project, Restricted Project
clayborg added a comment to D142926: [lldb] Replace SB swig interfaces with API headers.

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();
Feb 9 2023, 10:43 AM · Restricted Project, Restricted Project

Feb 8 2023

clayborg added a comment to D138618: [LLDB] Enable 64 bit debug/type offset.

Looks good to me. Pavel?

Feb 8 2023, 10:20 PM · Restricted Project, Restricted Project
clayborg added inline comments to D143623: [lldb] Print an error for unsupported combinations of log options.
Feb 8 2023, 10:07 PM · Restricted Project, Restricted Project
clayborg added a comment to D142926: [lldb] Replace SB swig interfaces with API headers.

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
Feb 8 2023, 11:47 AM · Restricted Project, Restricted Project
clayborg added a comment to D142926: [lldb] Replace SB swig interfaces with API headers.

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?)
Feb 8 2023, 11:43 AM · Restricted Project, Restricted Project
clayborg added inline comments to D138618: [LLDB] Enable 64 bit debug/type offset.
Feb 8 2023, 11:35 AM · Restricted Project, Restricted Project
clayborg accepted D143564: [LLDB] Add missing newline to "image lookup" output.
Feb 8 2023, 11:24 AM · Restricted Project, Restricted Project

Feb 7 2023

clayborg added a comment to D138618: [LLDB] Enable 64 bit debug/type offset.

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

Feb 7 2023, 1:57 PM · Restricted Project, Restricted Project
clayborg requested changes to D143520: Add a new SBDebugger::SetDestroyCallback() API.

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.

Feb 7 2023, 1:49 PM · Restricted Project, Restricted Project

Feb 6 2023

clayborg added a comment to D143347: [lldb][DWARF] Infer no_unique_address attribute.

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

Feb 6 2023, 2:38 PM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
clayborg edited reviewers for D143347: [lldb][DWARF] Infer no_unique_address attribute, added: JDevlieghere, jingham; removed: k8stone.
Feb 6 2023, 2:37 PM · Restricted Project, Restricted Project, Restricted Project, Restricted Project

Feb 2 2023

clayborg added inline comments to D142926: [lldb] Replace SB swig interfaces with API headers.
Feb 2 2023, 3:48 PM · Restricted Project, Restricted Project

Jan 31 2023

clayborg added a comment to D138618: [LLDB] Enable 64 bit debug/type offset.
  • 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.
Jan 31 2023, 3:04 PM · Restricted Project, Restricted Project

Jan 30 2023

clayborg added inline comments to D142926: [lldb] Replace SB swig interfaces with API headers.
Jan 30 2023, 5:10 PM · Restricted Project, Restricted Project
clayborg added a comment to D137139: [DWARF] Fix handling of .debug_aranges with -g1.

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?

Jan 30 2023, 5:01 PM · Restricted Project, Restricted Project
clayborg added a comment to D138618: [LLDB] Enable 64 bit debug/type offset.

So looks like this function needs to be fixed:

llvm::Optional<DIERef> DWARFBaseDIE::GetDIERef() const {
  if (!IsValid())
    return llvm::None;
Jan 30 2023, 3:19 PM · Restricted Project, Restricted Project
clayborg added a comment to D138618: [LLDB] Enable 64 bit debug/type offset.

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.

Jan 30 2023, 2:44 PM · Restricted Project, Restricted Project
clayborg added a comment to D138618: [LLDB] Enable 64 bit debug/type offset.

Looks good to me. Pavel?

Jan 30 2023, 2:32 PM · Restricted Project, Restricted Project
clayborg added a comment to D137139: [DWARF] Fix handling of .debug_aranges with -g1.

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?

Jan 30 2023, 11:23 AM · Restricted Project, Restricted Project
clayborg added a comment to D137139: [DWARF] Fix handling of .debug_aranges with -g1.

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?

Jan 30 2023, 11:05 AM · Restricted Project, Restricted Project

Jan 27 2023

clayborg added a comment to D137139: [DWARF] Fix handling of .debug_aranges with -g1.

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.

Jan 27 2023, 1:57 PM · Restricted Project, Restricted Project
clayborg updated the diff for D137900: Make only one function that needs to be implemented when searching for types..

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.

Jan 27 2023, 1:27 PM · Restricted Project, Restricted Project
clayborg accepted D142683: Manual DWARF index: don't skip over -gmodules debug info.

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

Jan 27 2023, 1:14 PM · Restricted Project, Restricted Project

Jan 26 2023

clayborg added inline comments to D142683: Manual DWARF index: don't skip over -gmodules debug info.
Jan 26 2023, 6:12 PM · Restricted Project, Restricted Project
clayborg added a comment to D137139: [DWARF] Fix handling of .debug_aranges with -g1.

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.

Jan 26 2023, 5:56 PM · Restricted Project, Restricted Project
clayborg accepted D142552: [lldb] Make GetDIENamesAndRanges() allow 0-valued decl and call lines.

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

Jan 26 2023, 1:40 PM · Restricted Project, Restricted Project
clayborg added a comment to D137139: [DWARF] Fix handling of .debug_aranges with -g1.

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?

Jan 26 2023, 10:41 AM · Restricted Project, Restricted Project
clayborg added a comment to D138618: [LLDB] Enable 64 bit debug/type offset.

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; }
Jan 26 2023, 10:34 AM · Restricted Project, Restricted Project
clayborg added a comment to D138618: [LLDB] Enable 64 bit debug/type offset.

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.

Jan 26 2023, 10:22 AM · Restricted Project, Restricted Project

Jan 19 2023

clayborg requested changes to D142052: [lldb] Implement SymbolFile::CopyType.

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.

Jan 19 2023, 5:15 PM · Restricted Project, Restricted Project
clayborg requested changes to D140630: [lldb-vscode] Add data breakpoint support.
Jan 19 2023, 3:30 PM · Restricted Project, Restricted Project
clayborg added a comment to D142052: [lldb] Implement SymbolFile::CopyType.

@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?

Jan 19 2023, 2:54 PM · Restricted Project, Restricted Project
clayborg added a comment to D142052: [lldb] Implement SymbolFile::CopyType.

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.
Jan 19 2023, 2:51 PM · Restricted Project, Restricted Project
clayborg accepted D141633: [lldb-vscode] Use SBFrame.GetDisplayFunctionName() instead of SBFrame.GetFunctionName().
Jan 19 2023, 2:37 PM · Restricted Project, Restricted Project
clayborg added a comment to D141637: [lldb-vscode] Fix an issue where lldb-vscode tries to display a source file for generated code.

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

Jan 19 2023, 2:36 PM · Restricted Project, Restricted Project
clayborg added a comment to D137900: Make only one function that needs to be implemented when searching for types..

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.

Jan 19 2023, 10:35 AM · Restricted Project, Restricted Project

Jan 17 2023

clayborg added a comment to D138618: [LLDB] Enable 64 bit debug/type offset.

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.

Jan 17 2023, 4:04 PM · Restricted Project, Restricted Project

Jan 16 2023

clayborg added inline comments to D140358: [lldb-vscode] Add support for disassembly view.
Jan 16 2023, 1:34 PM · Restricted Project, Restricted Project
clayborg added a comment to D140630: [lldb-vscode] Add data breakpoint support.

@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

Jan 16 2023, 1:11 PM · Restricted Project, Restricted Project
clayborg added a comment to D140841: [DWARFLinkerNext] Add StringPool class..

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

Jan 16 2023, 1:06 PM · Restricted Project, Restricted Project

Jan 13 2023

clayborg accepted D136928: [LLDB] Fix help text for "platform settings".

Sorry for the delay!

Jan 13 2023, 4:14 PM · Restricted Project, Restricted Project
clayborg added a comment to D141318: [lldb] Store shared pointers in DieToTypePtr map instead of raw pointers.

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!

Jan 13 2023, 4:00 PM · Restricted Project, Restricted Project
clayborg added a comment to D68655: Trust the arange accelerator tables in dSYMs.

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.

Jan 13 2023, 2:26 PM · Restricted Project, Restricted Project
clayborg added a comment to D68655: Trust the arange accelerator tables in dSYMs.

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

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)

Jan 13 2023, 2:15 PM · Restricted Project, Restricted Project

Jan 11 2023

clayborg added inline comments to D140358: [lldb-vscode] Add support for disassembly view.
Jan 11 2023, 5:13 PM · Restricted Project, Restricted Project
clayborg added a comment to D68655: Trust the arange accelerator tables in dSYMs.

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

Jan 11 2023, 4:41 PM · Restricted Project, Restricted Project
clayborg accepted rG9b737f148d88: [lldb] Limit trusting aranges to dSYMs only..

Looks good!

Jan 11 2023, 4:40 PM · Restricted Project
clayborg added a comment to D140358: [lldb-vscode] Add support for disassembly view.

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.

Jan 11 2023, 4:39 PM · Restricted Project, Restricted Project
clayborg added a comment to D68655: Trust the arange accelerator tables in dSYMs.

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?

Jan 11 2023, 4:15 PM · Restricted Project, Restricted Project
clayborg added a comment to D141318: [lldb] Store shared pointers in DieToTypePtr map instead of raw pointers.

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?

Jan 11 2023, 4:09 PM · Restricted Project, Restricted Project
clayborg added a comment to D140358: [lldb-vscode] Add support for disassembly view.

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

What do you mean by "reverse disassembly"?

Jan 11 2023, 3:40 PM · Restricted Project, Restricted Project
clayborg added a comment to D68655: Trust the arange accelerator tables in dSYMs.

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.

Jan 11 2023, 3:39 PM · Restricted Project, Restricted Project
clayborg added a comment to D68655: Trust the arange accelerator tables in dSYMs.

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:

Jan 11 2023, 3:29 PM · Restricted Project, Restricted Project
clayborg added a comment to D68655: Trust the arange accelerator tables in dSYMs.

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.

Jan 11 2023, 3:27 PM · Restricted Project, Restricted Project
clayborg added a comment to D141318: [lldb] Store shared pointers in DieToTypePtr map instead of raw pointers.

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.

Jan 11 2023, 3:12 PM · Restricted Project, Restricted Project
clayborg requested changes to D141318: [lldb] Store shared pointers in DieToTypePtr map instead of raw pointers.

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
Jan 11 2023, 3:09 PM · Restricted Project, Restricted Project
clayborg requested changes to D140630: [lldb-vscode] Add data breakpoint support.
Jan 11 2023, 2:58 PM · Restricted Project, Restricted Project