Page MenuHomePhabricator

labath (Pavel Labath)
User

Projects

User does not belong to any projects.

User Details

User Since
Jun 4 2013, 6:02 AM (511 w, 5 d)

Recent Activity

Tue, Mar 21

labath committed rG0165b73be37c: [lldb] Relax expectation on TestMainThreadExit (authored by labath).
[lldb] Relax expectation on TestMainThreadExit
Tue, Mar 21, 3:27 AM · Restricted Project
labath committed rG090205fb57a1: [lldb] Fix TestStepOverWatchpoint (authored by labath).
[lldb] Fix TestStepOverWatchpoint
Tue, Mar 21, 2:46 AM · Restricted Project

Tue, Mar 14

labath added inline comments to D145450: [lldb] Respect empty arguments in target.run-args.
Tue, Mar 14, 12:53 PM · Restricted Project, Restricted Project
labath added a comment to D146044: [lldb] Implement CrashReason using UnixSignals.

It's not exactly what I had in mind, but I kinda like it :)

Tue, Mar 14, 10:27 AM · Restricted Project, Restricted Project
labath accepted D146043: [lldb] Refactor CrashReason.
Tue, Mar 14, 10:16 AM · Restricted Project, Restricted Project
labath added a comment to D145955: Refactor ObjectFilePlaceholder for sharing.

yay

Tue, Mar 14, 10:12 AM · Restricted Project, Restricted Project
labath added a comment to D144392: [lldb] Skip signal handlers for ignored signals on lldb-server's side when stepping.

Gdb does this for example:

GDB optimizes for stepping the mainline code. If a signal that has handle nostop and handle pass set arrives while a stepping command (e.g., stepi, step, next) is in progress, GDB lets the signal handler run and then resumes stepping the mainline code once the signal handler returns. In other words, GDB steps over the signal handler. This prevents signals that you’ve specified as not interesting (with handle nostop) from changing the focus of debugging unexpectedly.

(https://sourceware.org/gdb/onlinedocs/gdb/Signals.html), which seems reasonable.

Tue, Mar 14, 10:11 AM · Restricted Project, Restricted Project
labath added inline comments to D145180: [lldb] Introduce new SymbolFileJSON and ObjectFileJSON.
Tue, Mar 14, 9:53 AM · Restricted Project, Restricted Project
labath added inline comments to D145450: [lldb] Respect empty arguments in target.run-args.
Tue, Mar 14, 9:41 AM · Restricted Project, Restricted Project

Tue, Mar 7

labath accepted D145487: [lldb][test] TestDataFormatterCpp.py: add test-case for member function pointer format.
  • Remove virtual function pointer test for now
Tue, Mar 7, 5:44 AM · Restricted Project, Restricted Project
labath added inline comments to D145487: [lldb][test] TestDataFormatterCpp.py: add test-case for member function pointer format.
Tue, Mar 7, 4:43 AM · Restricted Project, Restricted Project
labath added a comment to D145242: [lldb][TypeSystemClang] Use the CXXFunctionPointerSummaryProvider for member-function pointers.

You may want to check that pointers to virtual functions get printed here. In particular, that 0x00000000000000010000000000000000 (pointer to the first virtual function) does not get printed as nullptr or something like that (due to the truncation of the 128-bit value to zero).

Tue, Mar 7, 3:10 AM · Restricted Project, Restricted Project

Mon, Mar 6

labath added a comment to D145136: Add a Debugger interruption mechanism in parallel to the Command Interpreter interruption.

I kinda like this.

Mon, Mar 6, 6:56 AM · Restricted Project, Restricted Project
labath added inline comments to D145377: [LLDB] Show sub type of memory tagging SEGV when reading a core file.
Mon, Mar 6, 6:43 AM · Restricted Project, Restricted Project, Restricted Project
labath added inline comments to D143104: [lldb/Plugins] Add Attach capabilities to ScriptedProcess.
Mon, Mar 6, 5:43 AM · Restricted Project, Restricted Project
labath accepted D145115: [LLDB][NativePDB] Check string table in PDB files..
Mon, Mar 6, 3:32 AM · Restricted Project, Restricted Project
labath added a comment to D144181: [clang][DebugInfo] Add abi-tags on constructors/destructors as LLVM annotations.

We thought a bit about what it would take to link a constructor declaration DIE to the various definitions (e.g., via a DW_AT_LLVM_complete_ctor_linkage_name or DW_AT_LLVM_complete_ctor_ref). The issue with this is that it would mess with type uniquing. E.g., what if two different CUs each had a constructor declaration but differed only in which definitions they linked to? Even if dsymutil were to be able to merge the declarations into a single one, LLDB would still have to support the case where two constructor DIEs for the same type point to different definitions depending on how they were used in the corresponding object files.

One could instead have some new attribute on the various constructor definitions specifying which constructor type it is, and then implement @pavel's suggestion of attaching multiple asm labels to the CXXConstructorDecl. To support that in LLDB we'd have to do a lookup in the DWARF index and filter appropriately.

Mon, Mar 6, 3:26 AM · debug-info, Restricted Project, Restricted Project

Wed, Mar 1

labath committed rGfad60105ace5: [lldb/test] Update error message in debug-types-signature-loop.s (authored by labath).
[lldb/test] Update error message in debug-types-signature-loop.s
Wed, Mar 1, 4:52 AM · Restricted Project
labath accepted D144904: [Linux] Add kernel.yama.ptrace_scope note when ptrace fails to attach..
Wed, Mar 1, 3:03 AM · Restricted Project, Restricted Project

Feb 23 2023

labath committed rGd8bd179a1738: Clear read_fd_set if EINTR received (authored by emrekultursay).
Clear read_fd_set if EINTR received
Feb 23 2023, 4:27 AM · Restricted Project
labath closed D144240: Clear read_fd_set if EINTR received.
Feb 23 2023, 4:27 AM · Restricted Project, Restricted Project

Feb 21 2023

labath added a comment to D144181: [clang][DebugInfo] Add abi-tags on constructors/destructors as LLVM annotations.

The construction of the mangled name does not require getting just the abi tags of the constructor itself right. Rather, it requires knowing the abi tag annotations of all arguments (template and regular) of the constructor, and template arguments of all enclosing classes. Is the information in this patch enough to reconstruct this name (_ZN2S1I1AB4asdfE2S2I1BB4asdfEC2I1CB4asdfEET_1DB4asdf, a.k.a, S1<A[abi:asdf]>::S2<B[abi:asdf]>::S2<C[abi:asdf]>(C[abi:asdf], D[abi:asdf]))?

That's a fair point. I purposefully didn't support abi_tags on all possible declarations because that would've ballooned the debug-info size too much. And we get away with it because libc++ exclusively tags functions. But of course there's no guarantee that they won't start tagging structures or namespaces

Feb 21 2023, 4:40 AM · debug-info, Restricted Project, Restricted Project
labath added a comment to D144181: [clang][DebugInfo] Add abi-tags on constructors/destructors as LLVM annotations.

Hmm, I'd sort of still be inclined to look further at the linkage name option - but maybe the existence of a situation where the usage is built with debug info, but the out of line ctor definitions are all in another library without debug info is a sufficiently motivating use case to justify the addition of these attributes to the ctor declaration. *shrug* Carry on then.

That would've been convenient but I don't think we can get it right with attaching the linkage name (even if we are willing to do the search for the correct linkage name). That would essentially coalesce the possibly multiple definitions of the constructor into a single one (since there's only ever a single CXXConstructorDecl in the AST for a given constructor in the source)

(CC @labath)

Feb 21 2023, 12:44 AM · debug-info, Restricted Project, Restricted Project

Feb 20 2023

labath accepted D144240: Clear read_fd_set if EINTR received.

Nice catch.

Feb 20 2023, 7:51 AM · Restricted Project, Restricted Project
labath added inline comments to D144224: [lldb] Extend SWIG SBProcess interface with WriteMemoryAsCString method.
Feb 20 2023, 7:46 AM · Restricted Project, Restricted Project
labath added a comment to D142926: [lldb] Replace SB swig interfaces with API headers.

Well... right now, there isn't anything else to do there.

Anyway, I don't think this is particularly important. I don't know even if the distros would like to do this or whether to they'd prefer to ship unchanged headers. However, to me, it seems like the choice of the distribution method (framework vs. "traditional" unix layout) should be orthogonal to the choice of deswig-ifying the headers.

I think I agree with you about the choice of distribution method being orthogonal to choice of removing the swig ifdefs from the headers. That being said, I don't know if any other distributors would care for this and I'm not sure who I would ask. I added support for using unifdef in the LLDB.framework case because I can ensure it is used and tested. I am hesitant to add support without somebody to test/use this.

Sort of tangential to this, the only other target where I think this would be used would be the lldb-headers target. That seems to install all the lldb headers, including all the private lldb headers. I could use unifdef there (assuming it's available). Is it intended that it installs all the private lldb headers though?

Feb 20 2023, 7:39 AM · Restricted Project, Restricted Project
labath requested changes to D144392: [lldb] Skip signal handlers for ignored signals on lldb-server's side when stepping.

I'm afraid you're fixing this at the wrong end. This kind of complex thread control does not belong inside the debug stub.

Feb 20 2023, 7:33 AM · Restricted Project, Restricted Project
labath added a comment to D143698: Support Debugging TLS variable with lldb.

There's one thing that's not clear to me about this patch. What is the relationship between qGetTLSAddr, as implemented here, and its implementation in GDB? I guess they're not compatible since we're not sending or using the offset and link map fields, but it's not clear to me whether we're at least implementing some subset of gdb functionality (e.g., what would gdbserver do if it received one of the packets that we're sending here). If we're not, then I don't think we should be using the same packet for this purpose.

Feb 20 2023, 7:19 AM · Restricted Project, Restricted Project

Feb 13 2023

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

I like this.

I'm not sure what it would take, but I think it'd be nice if the de-swig-ification was not specific to the framework build. Ideally, I'd make it controlled by a separate cmake variable, (autodetected to on if the relevant tool is found, but also overridable by the user).

Yeah we could search for unified earlier in the process and use it when we distribute headers. The de-swig-ification of the headers is primarily for distribution, do any other platforms distribute the headers?

Feb 13 2023, 9:49 AM · Restricted Project, Restricted Project
labath added inline comments to D143652: [lldb][DWARFASTParserClang] Attach linkage name to ctors/dtors if missing.
Feb 13 2023, 9:30 AM · Restricted Project, Restricted Project
labath added inline comments to D143652: [lldb][DWARFASTParserClang] Attach linkage name to ctors/dtors if missing.
Feb 13 2023, 6:07 AM · Restricted Project, Restricted Project
labath added a comment to D138618: [LLDB] Enable 64 bit debug/type offset.

Thank you for your patience. I'm really happy with the overall result here.

Feb 13 2023, 5:41 AM · Restricted Project, Restricted Project
labath accepted D138618: [LLDB] Enable 64 bit debug/type offset.
Feb 13 2023, 5:40 AM · Restricted Project, Restricted Project

Feb 8 2023

labath added inline comments to D138618: [LLDB] Enable 64 bit debug/type offset.
Feb 8 2023, 10:44 AM · Restricted Project, Restricted Project
labath accepted D143564: [LLDB] Add missing newline to "image lookup" output.
Feb 8 2023, 9:25 AM · Restricted Project, Restricted Project

Feb 3 2023

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

I like this.

Feb 3 2023, 4:21 AM · Restricted Project, Restricted Project
labath updated subscribers of D142309: [LLDB][NFC] Fix a incorrect use of shared_ptr in RenderScriptRuntime.cpp.

LGTM, but I wonder when we'll be able to get rid of RenderScript completely (CC @labath)

Feb 3 2023, 4:14 AM · Restricted Project, Restricted Project
labath 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.

I do want to state that if we fix things the way I describe it will work seamlessly with OSO or DWO files. The current state of things is the DWO stuff only uses the fancy DIERef constructor and fills in the dwo number correctly only to have it overwritten in SymbolFile::GetUID(...). The SymbolFile::GetUID(...) is needed for OSOs currently because the DIERef that SymbolFileDWARF (which is used for OSO) doesn't correctly create DIERef objects since they always return llvm::None for SymbolFileDWARF::GetDwoNum(). But the new API will have SymbolFileDWARF::GetFileIndex() to be used instead of SymbolFileDWARF::GetDwoNum(), and the file index will be set correctly for both DWO and OSO files. We can then change DIERef away from DWO specific naming, and have DIERef have a "m_file_index" and "m_file_index_valid" instead of the dwo specific members. As long as both OSO and DWO files can be found from the user_id_t API calls we are all good. Not sure if this addresses all of your issues or not.

If all of your concerns are not clarified above, can you clarify what is still being overlooked? Both Alexander and I are usually thinking we are addressing everything you want, but we obviously still aren't, so restating your remaining concerns would help us get this patch moving.

Feb 3 2023, 4:04 AM · Restricted Project, Restricted Project
labath accepted D143127: [LLDB] Fix assertion failure by removing `CopyType` in `std::coroutine_handle` pretty printer.

Well.. I don't believe we have any ASTImporter calls in any of the other pretty printers, so I think this change is good. And if it causes some other issues, then maybe we need to look at solving them in some other way...

Feb 3 2023, 3:57 AM · Restricted Project, Restricted Project

Feb 2 2023

labath committed rG88ac9138f4eb: [lldb] Try harder to optimize away a test variable (authored by labath).
[lldb] Try harder to optimize away a test variable
Feb 2 2023, 2:25 AM · Restricted Project

Jan 31 2023

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

How about we make DIERef constructor always take all the info that is needed to construct the objects correctly:

DIERef(DWARFDie die);
DIERef(SymbolFileDWARF *dwarf, dw_offset_t die_offset); // might not need this one?
DIERef(user_id_t uid);

We might not need all of these. But in this case, we can't incorrectly use the APIs since all of the objects that are needed to fill it in are in the constructor args. We take away the ability to manually fill in the DWO num and other fields. Would that fix the issues you have with this patch Pavel?

Jan 31 2023, 12:26 PM · Restricted Project, Restricted Project

Jan 30 2023

labath committed rG8b4d263799e1: [lldb] Fix TestVSCode_completions for D141828 (authored by labath).
[lldb] Fix TestVSCode_completions for D141828
Jan 30 2023, 4:20 AM · Restricted Project
labath added a comment to D142672: [lldb] Make SBSection::GetSectionData call Section::GetSectionData..

I'm sorry Omair, my comment was meant for Jorge, not you. @skipIfXml is not the right decorator here, although it will probably kinda fix the failure (because the bot does not have xml support either). I *think* the compressed section support is controlled by presence of zlib.

Jan 30 2023, 4:14 AM · Restricted Project, Restricted Project
labath added a comment to D142672: [lldb] Make SBSection::GetSectionData call Section::GetSectionData..

I'm pretty sure that's because that bot builds without zlib support. You'll need to add something like @skipIfXmlSupportMissing to this test (I think we don't have a decorator for zlib now, so you'll probably need to add one).

Jan 30 2023, 3:02 AM · Restricted Project, Restricted Project

Jan 27 2023

labath 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.

Jan 27 2023, 11:10 AM · Restricted Project, Restricted Project
labath accepted D142709: [lldb][Target] GetScratchTypeSystems: sort TypeSystems with strict weak ordering.

oops

Jan 27 2023, 5:10 AM · Restricted Project, Restricted Project
labath added inline comments to D139957: [LLDB] Change OSO to use DieRef.
Jan 27 2023, 5:09 AM · Restricted Project, Restricted Project
labath 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; }

Then anyone can set the file index correctly for DWO or OSO files. And we avoid using user_id_t values for the symbol files since they aren't needed.

Jan 27 2023, 5:07 AM · Restricted Project, Restricted Project
labath accepted D142266: [lldb] Add PlatformMetadata for ScriptedPlatform.

I wouldn't exactly use the word happy, but I also don't have the time to come up with an alternative proposal.

Jan 27 2023, 4:54 AM · Restricted Project, Restricted Project
labath accepted D142672: [lldb] Make SBSection::GetSectionData call Section::GetSectionData..
Jan 27 2023, 4:49 AM · Restricted Project, Restricted Project

Jan 25 2023

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

Jan 25 2023, 11:08 AM · Restricted Project, Restricted Project

Jan 23 2023

labath added inline comments to D139957: [LLDB] Change OSO to use DieRef.
Jan 23 2023, 6:10 AM · Restricted Project, Restricted Project
labath added a comment to D138618: [LLDB] Enable 64 bit debug/type offset.

...
Since the user IDs of SymbolFileDWARF plug-ins mean nothing to anyone else, we can make them what we need them to be so they work for us. I would suggest to remove the use of DIERef from calculating the IDs of symbol files and have .o files for mac and .dwo files for fission use a 1 based index as their ID to make it easy to encode into a DIERef when needed for lldb::user_id_t values that _are_ included in objects that we hand out. Is there anything else that would need to be done to keep everyone happy here?

Jan 23 2023, 6:10 AM · Restricted Project, Restricted Project
labath added a comment to D141605: [lldb] Detach the child process when stepping over a fork.

Thanks for your response, Jim.

Jan 23 2023, 5:18 AM · Restricted Project, Restricted Project
labath added a comment to D78026: add patch to print lldb path.

I'm sorry, but I still don't feel I can properly review. Can you answer the questions from my previous comment? I need to understand what are the locations of the various lldb components and the values of relevant variables (LLDB_PYTHON_RELATIVE_LIBDIR for one), before and after this patch.

Jan 23 2023, 3:11 AM · Restricted Project
labath accepted D141687: [LLDB] Remove return value from DumpRegisterValue.
Jan 23 2023, 2:43 AM · Restricted Project, Restricted Project

Jan 13 2023

labath added a comment to D139249: [lldb] Add Debugger & ScriptedMetadata reference to Platform::CreateInstance.

In addition to that, it breaks tests with ubsan. :)

Jan 13 2023, 7:49 AM · Restricted Project, Restricted Project

Jan 12 2023

labath 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.

It does. It just doesn't do it by default -- same as on linux (but not on PS4 I believe). You have to use the -gdwarf-aranges flag explicitly (which, quite possibly, noone does),

Jan 12 2023, 11:39 AM · Restricted Project, Restricted Project
labath added a comment to D141605: [lldb] Detach the child process when stepping over a fork.

(The fix seems to make LLDB do the right thing, although I can't claim to fully understand what is going on. From what I can tell, we're currently detaching from the fork child when the fork event gets pulled off the public event queue, and this code is preventing that from happening. Intuitively it seems like the public event queue pull is slightly too late to be doing this kind of thing.)

Jan 12 2023, 5:18 AM · Restricted Project, Restricted Project
labath requested review of D141605: [lldb] Detach the child process when stepping over a fork.
Jan 12 2023, 5:13 AM · Restricted Project, Restricted Project
labath accepted D139250: [lldb] Add ScriptedPlatform python implementation.
Jan 12 2023, 4:52 AM · Restricted Project, Restricted Project
labath added a comment to D141330: [lldb] Limit 8b259fe573e1 to dSYMs .

Just to clarify: I was responding to the question about the ELF file usage, not questioning the overall approach. Given that MachO has a special code for dsym files, I think it makes sense to use it.

Jan 12 2023, 4:50 AM · Restricted Project, Restricted Project
labath added a comment to D141021: [lldb] Remove tools copied into LLDB.framework before install.

I don't dispute the value of determinism, but it seems like there ought to be a way to achieve this with corrupting the build tree (which in itself is not very 'deterministic').
What if we had two copies of the framework in the build-tree? One pristine copy, which would only contain liblldb (and any stuff that cmake puts there by default), and then another one which would be used for running, and which would contain all of the manually added stuff (with the right rpaths and all). When installing, one would use the pristine copy as the source instead of the adulterated one.

Jan 12 2023, 4:40 AM · Restricted Project, Restricted Project
labath 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.

Jan 12 2023, 4:33 AM · Restricted Project, Restricted Project

Jan 11 2023

labath added inline comments to D139250: [lldb] Add ScriptedPlatform python implementation.
Jan 11 2023, 6:35 AM · Restricted Project, Restricted Project
labath added a comment to D141021: [lldb] Remove tools copied into LLDB.framework before install.

Does this mean that the in-tree lldb will cease to be functional once someone "installs" it? (at least until the next rebuild)

Jan 11 2023, 6:04 AM · Restricted Project, Restricted Project
labath updated subscribers of rG8b259fe573e1: [lldb] Trust the arange accelerator tables in dSYMs.

@dblaikie, who was looking for aranges vs. DW_AT_ranges comparisons

Jan 11 2023, 6:01 AM · Restricted Project
labath added a comment to D141330: [lldb] Limit 8b259fe573e1 to dSYMs .

Should this be true for a fully linked ELF executable, too?

Sounds reasonable, but adding @labath and @DavidSpickett to confirm. This is trivial to extend later.

Jan 11 2023, 6:00 AM · Restricted Project, Restricted Project

Jan 9 2023

labath accepted D139955: [LLDB] Change formatting to use llvm::formatv.

I'm sorry for the delay, I was out for the holidays. Looks now, thanks for your patience.

Jan 9 2023, 4:28 AM · Restricted Project, Restricted Project

Dec 22 2022

labath accepted D138344: [test][lldb-vscode] Relax assertion to allow multiple compile units returned..

LGTM

Dec 22 2022, 3:38 AM · Restricted Project, Restricted Project

Dec 21 2022

labath added a comment to D139955: [LLDB] Change formatting to use llvm::formatv.

This is looking much better -- and focused. I still have comments though. :)

Dec 21 2022, 1:38 PM · Restricted Project, Restricted Project
labath added a comment to D139945: [lldb] Add scripted process launch/attach option to {,platform }process commands.

For a "plugin", the scripted process is sure getting a lot of special handling in generic code. (I know this isn't being introduced here, but I wasn't involved in the previous review -- and I'm not actually sure I want to be involved here). I don't think that's necessarily a bad thing in this case, but maybe we should not be calling it a plugin in that case? We already have a couple of precedents for putting implementations of "pluggable" classes into generic code -- ProcessTrace for instance. And just like in the case of ProcessTrace (where the real plugin is the thing which handles the trace file format), here the real plugin would the the scripting language backing the scripted process?

(Apart from that, this patch seems fine.)

Maybe one way around this is to have some generic metadata that gets passed to the plugin, that can be different per plugin?

Dec 21 2022, 1:07 PM · Restricted Project, Restricted Project
labath added inline comments to D139250: [lldb] Add ScriptedPlatform python implementation.
Dec 21 2022, 12:58 PM · Restricted Project, Restricted Project

Dec 19 2022

labath added inline comments to D139250: [lldb] Add ScriptedPlatform python implementation.
Dec 19 2022, 6:53 AM · Restricted Project, Restricted Project
labath 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.

Dec 19 2022, 6:26 AM · Restricted Project, Restricted Project
labath added inline comments to D140262: [lldb][TypeSystemClang][NFC] Introduce TemplateParameterInfos::ArgumentMetadata.
Dec 19 2022, 4:45 AM · Restricted Project, Restricted Project
labath committed rG071c62c5d3ed: [lldb] Modernize sprintf in FormatEntity.cpp (authored by labath).
[lldb] Modernize sprintf in FormatEntity.cpp
Dec 19 2022, 1:54 AM · Restricted Project
labath accepted D140262: [lldb][TypeSystemClang][NFC] Introduce TemplateParameterInfos::ArgumentMetadata.
Dec 19 2022, 1:17 AM · Restricted Project, Restricted Project
labath accepted D140030: [lldb][TypeSystemClang][NFC] Make TemplateParameterInfos members private.

That looks great. I have just one small nit. I think the test would read better if you made the names of temporary TemplateArgument objects more descriptive -- i.e. encode the kind and value information in the name. For example. int47Arg for an integer template argument whose value is 47, and intTypeArg for a type template argument (where int is the "value").

Dec 19 2022, 1:07 AM · Restricted Project, Restricted Project

Dec 15 2022

labath added a comment to D139955: [LLDB] Change formatting to use llvm::formatv.

I tried to modify all the places wehre getOffset() is used. Which will later return 64bit instead of 32 bit.

Sorry I guess there was miss communication.
I am not quite clear in what you are suggesting.
The way I did it we now have two distinct interfaces. One for old way that takes in char* and args and applies formatting internally, and a new one which takes in std::string. So all formatting is done on caller side with std::string(llvm::formatv(..)).
Are you suggesting to change implementation of

void ReportError(const char *format, ...)
      __attribute__((format(printf, 2, 3)));

and others to use llvm::formatv under the hood instead?

Yes, that's pretty much it.

Dec 15 2022, 2:07 PM · Restricted Project, Restricted Project
labath added inline comments to D139248: [lldb/Interpreter] Improve ScriptedPythonInterface::GetStatusFromMethod.
Dec 15 2022, 2:01 PM · Restricted Project, Restricted Project
labath added inline comments to D139249: [lldb] Add Debugger & ScriptedMetadata reference to Platform::CreateInstance.
Dec 15 2022, 1:57 PM · Restricted Project, Restricted Project
labath added inline comments to D139249: [lldb] Add Debugger & ScriptedMetadata reference to Platform::CreateInstance.
Dec 15 2022, 1:52 PM · Restricted Project, Restricted Project
labath added inline comments to D139250: [lldb] Add ScriptedPlatform python implementation.
Dec 15 2022, 1:40 PM · Restricted Project, Restricted Project
labath added a comment to D139945: [lldb] Add scripted process launch/attach option to {,platform }process commands.

For a "plugin", the scripted process is sure getting a lot of special handling in generic code. (I know this isn't being introduced here, but I wasn't involved in the previous review -- and I'm not actually sure I want to be involved here). I don't think that's necessarily a bad thing in this case, but maybe we should not be calling it a plugin in that case? We already have a couple of precedents for putting implementations of "pluggable" classes into generic code -- ProcessTrace for instance. And just like in the case of ProcessTrace (where the real plugin is the thing which handles the trace file format), here the real plugin would the the scripting language backing the scripted process?

Dec 15 2022, 1:24 PM · Restricted Project, Restricted Project
labath added a comment to D140030: [lldb][TypeSystemClang][NFC] Make TemplateParameterInfos members private.

I think that test should just be rewritten to not modify the object in such a way. Instead of cumulatively modifying a single object, it could just create a fresh one each time. That will also make it clearer what is being tested. And I think it's fair game to add some constructors to the production object to make that easy.

Dec 15 2022, 12:56 PM · Restricted Project, Restricted Project
labath added a comment to D140092: [NFC][LLDB] Using namespace llvm in EmulateInstructionRISCV.
Dec 15 2022, 12:41 PM · Restricted Project, Restricted Project
labath added a comment to D138618: [LLDB] Enable 64 bit debug/type offset.

Thanks for splitting this up. We still need to figure out what to do with the first patch, but these two are looking very good now.

Dec 15 2022, 12:39 PM · Restricted Project, Restricted Project
labath added a comment to D139955: [LLDB] Change formatting to use llvm::formatv.

Yeah, I'm afraid this is all my fault. I suggested going in this direction, but this isn't exactly how I thought it would turn out. I was expecting that the inferface would be something along the lines of what Jonas posted in his comment. And I was not expecting that you will try to convert every single instance of PRIx32 dwarf offset printing. The majority of the modified lines are the ReportError and ReportWarning statements, so I though we would just be converting those. And the majority of the callers of these functions are coming from DWARF code, so I agree with Jonas that it would be nice to convert all callers of these functions -- but I won't insist on it.

Dec 15 2022, 11:41 AM · Restricted Project, Restricted Project
labath added inline comments to D139252: [lldb/Plugins] Introduce Scripted Platform Plugin.
Dec 15 2022, 11:06 AM · Restricted Project, Restricted Project
labath added inline comments to D140092: [NFC][LLDB] Using namespace llvm in EmulateInstructionRISCV.
Dec 15 2022, 10:54 AM · Restricted Project, Restricted Project
labath accepted D140112: [lldb][Test] Propagate llvm::yaml error message in TestFile::fromYaml.
Dec 15 2022, 10:50 AM · Restricted Project, Restricted Project

Dec 13 2022

labath added a comment to D139853: [lldb/Process] Populate queues in Scripted Process.

That definitely looks much better. As I understand it, the scripted thread interface is more high-level that our internal thread interface, and that means it is computing its queue by itself, instead of delegating the work to a separate plugin. That makes sense to me.

Dec 13 2022, 7:54 AM · Restricted Project, Restricted Project
labath added inline comments to D139853: [lldb/Process] Populate queues in Scripted Process.
Dec 13 2022, 7:24 AM · Restricted Project, Restricted Project

Dec 12 2022

labath accepted D139734: [LLDB] Remove redundant XFAIL.
Dec 12 2022, 6:47 AM · Restricted Project, Restricted Project
labath added a comment to D138724: [lldb][Target] Flush the scratch TypeSystem when process gets deleted.

I just found out that this test (the non-shared-library version) fails on linux if the executable is built with -no-pie (which is the default if the CLANG_DEFAULT_PIE_ON_LINUX option is not set, which happened to be the case for my build). I think the important fact here is that a PIE ELF executable gets its e_type field set to ET_DYN, which causes lldb to identify it as a shared library. However, it is not clear to me why would that matter in this case...

Dec 12 2022, 5:03 AM · Restricted Project, Restricted Project
labath added inline comments to D139463: Fix breakpoint-command.test when no script interpreter is compiled in..
Dec 12 2022, 4:37 AM · Restricted Project, Restricted Project
labath added inline comments to D139250: [lldb] Add ScriptedPlatform python implementation.
Dec 12 2022, 4:29 AM · Restricted Project, Restricted Project
labath added a comment to D139734: [LLDB] Remove redundant XFAIL.

You can just delete the XFAIL line. I probably should have done that when I added UNSUPPORTED: system-linux in 1004d6e7e2eb24edc01d7e330c538ce5cb590001.

Dec 12 2022, 2:24 AM · Restricted Project, Restricted Project
labath added a comment to D133376: Allow epilogue_begin to be emitted when generating DWARF.

I don't think that this indicates a problem with this patch, but you might be interested to know that this broke a test in lldb which was trying to change the PC value to a given line (and it failed because that line was now associated with multiple line entries). I have worked around the problem with a06342d250ec7bee37dc93477f233e43e597aca5 and filed PR59458 to track possible improvements in the lldb implementation.

Dec 12 2022, 12:36 AM · debug-info, Restricted Project, Restricted Project