Page MenuHomePhabricator

labath (Pavel Labath)
User

Projects

User does not belong to any projects.

User Details

User Since
Jun 4 2013, 6:02 AM (347 w, 10 h)

Recent Activity

Today

labath committed rGc4267b7b1371: Revert "[lldb/PDB] Use the new line table constructor" (authored by labath).
Revert "[lldb/PDB] Use the new line table constructor"
Tue, Jan 28, 8:22 AM
labath added a reverting change for rGbb73210ba9f1: [lldb/PDB] Use the new line table constructor: rGc4267b7b1371: Revert "[lldb/PDB] Use the new line table constructor".
Tue, Jan 28, 8:22 AM
labath added a comment to D72158: [DebugInfo] Make most debug line prologue errors non-fatal to parsing.

If I understand this correctly, this will cause lldb to continue to read the parsed line table contribution after encountering some errors in the prologue, whereas previously we would stop straight away. That sounds reasonable if now or in the future we will be able to get some useful information (at least some subset of file names, or line numbers without file names, etc.) out of these kinds of line tables. If that is not the case, and these errors are recoverable only in the sense that they allow you to jump to the next contribution, then it might be better to treat these errors as unrecoverable for lldb purposes (jumping to the next contribution is not interesting for us since we always use DW_AT_stmt_list to locate the line table).

Tue, Jan 28, 6:50 AM · Restricted Project, Restricted Project
labath committed rGbb73210ba9f1: [lldb/PDB] Use the new line table constructor (authored by labath).
[lldb/PDB] Use the new line table constructor
Tue, Jan 28, 6:13 AM
labath added a comment to D70840: [LLDB] [DWARF] Strip out the thumb bit from addresses on ARM.

This is something that probably should be discussed with the llvm dwarf owners too (though most of them are on this review already).

However, if we assume that we can agree that this logic should be implemented at a fairly low level in the dwarf parsing code (so that e.g., when we switch to the llvm parser, it will do this thing for us),

I guess the main question here, as follows from my comment above, is who/how is the fixup controlled in the llvm side, where there's (to my knowledge) no GetOpcodeLoadAddress that can abstract away the fixup? IIRC the dwarf code itself is happily unaware of what architecture it is describing.

Tue, Jan 28, 5:45 AM · Restricted Project
labath added a reviewer for D70646: Move non-DWARF code: `DWARFUnit` -> `SymbolFileDWARF`: JDevlieghere.

This moves a lot of lldb-specific stuff out of low-level dwarf code, and as such, I think this is a great step towards the parser unification.

Tue, Jan 28, 5:27 AM · Restricted Project
labath updated subscribers of D73191: Only match mangled name in full-name function lookup (with accelerators).

I've had to revert this because of some failures on macos (http://lab.llvm.org:8080/green/view/LLDB/job/lldb-cmake/7109/testReport/). @jarin, are you able to run the test suite on a mac to investigate.

Tue, Jan 28, 5:09 AM · Restricted Project
labath committed rGd8de349951c2: Revert "[lldb/DWARF] Only match mangled name in full-name function lookup (with… (authored by labath).
Revert "[lldb/DWARF] Only match mangled name in full-name function lookup (with…
Tue, Jan 28, 4:59 AM
labath added a reverting change for rG1b1276688300: [lldb/DWARF] Only match mangled name in full-name function lookup (with…: rGd8de349951c2: Revert "[lldb/DWARF] Only match mangled name in full-name function lookup (with….
Tue, Jan 28, 4:59 AM
labath added a comment to D73539: [AVR] Basic support for remote debugging.

Thanks for the patch, and in particular for starting with a small increment instead of a giant implement-all patch.

Tue, Jan 28, 4:41 AM · Restricted Project
labath added a comment to D70840: [LLDB] [DWARF] Strip out the thumb bit from addresses on ARM.

The thing that I am not sure we have fully explored is whether there is any need for this &~1 business in the llvm dwarf code. For instance, what should llvm::DWARFUnit::getSubroutineForAddress return if you pass it an address that is equal to the actual memory address of the start of the thumb function, but the relevant DW_AT_low_pc contains address|1? Answering that might give us an indication on what is the layer at which this fixup should be applied.

From a quick check at the code there, for that particular case, either it'd be done in llvm::DWARFUnit::updateAddressDieMap or in llvm::DWARFDie::getAddressRanges. Since all levels of the API is public, the fix does become safer the closer to parsing it is, but one also generally has less context to work with in those cases.

Tue, Jan 28, 4:41 AM · Restricted Project
labath committed rG1b1276688300: [lldb/DWARF] Only match mangled name in full-name function lookup (with… (authored by jarin).
[lldb/DWARF] Only match mangled name in full-name function lookup (with…
Tue, Jan 28, 3:28 AM
labath closed D73191: Only match mangled name in full-name function lookup (with accelerators).
Tue, Jan 28, 3:28 AM · Restricted Project
labath added a comment to D73506: Auto-completion bug fix for dot operator.

Thanks for the patch.

Tue, Jan 28, 12:34 AM · Restricted Project

Yesterday

labath added a reviewer for D73279: testsuite: generalize `DWARFASTParserClangTests` based on `DWARFExpressionTest`'s YAML: aprantl.

Thanks for splitting this off.

Mon, Jan 27, 5:08 AM · Restricted Project
labath added a comment to D72751: [LLDB] Add DynamicLoaderWasmDYLD plugin for WebAssembly debugging.

Thanks. I am glad that we were able to sort that out.

Mon, Jan 27, 2:43 AM · Restricted Project
labath added a comment to D69273: ValueObject: Fix a crash related to children address type computation.

Thanks for the update Jim. I'm putting some of my thoughts inline.

Mon, Jan 27, 2:27 AM · Restricted Project
labath added a comment to D73191: Only match mangled name in full-name function lookup (with accelerators).

(I mean, if there is a real benefit to having some queries return only non-methods, then we can certainly do something like that as well, but that should be handled differently -- either we can create a new query mode, or change the behavior (and name?) of eFunctionNameTypeFull across the board).

Mon, Jan 27, 1:42 AM · Restricted Project
labath accepted D73191: Only match mangled name in full-name function lookup (with accelerators).
Mon, Jan 27, 1:25 AM · Restricted Project
labath committed rZORG2b3682001b5e: Switch lldb-x86_64-debian to python3 (authored by labath).
Switch lldb-x86_64-debian to python3
Mon, Jan 27, 1:15 AM
labath added a comment to D73383: Allow retrieving source files relative to the compilation directory..

DWARF5 still says (when describing the directory entries): "Each additional path entry is either a full path name or is relative to the current directory of the compilation.", so I believe this change is correct.

Mon, Jan 27, 1:01 AM · Restricted Project

Fri, Jan 24

labath resigned from D73345: [lldb] Don't create duplicate declarations when completing a forward declaration with a definition from another source.

The description makes sense to me but I don't know much about this stuff. I'll leave it to @shafik to ack this...

Fri, Jan 24, 5:10 AM · Restricted Project
labath added inline comments to D73191: Only match mangled name in full-name function lookup (with accelerators).
Fri, Jan 24, 4:34 AM · Restricted Project
labath committed rGd4b092b34125: [lldb/DWARF] Remove a workaround from DebugNamesDWARFIndex (authored by labath).
[lldb/DWARF] Remove a workaround from DebugNamesDWARFIndex
Fri, Jan 24, 3:22 AM
labath committed rG77cedb0cdb86: [lldb] Fix nondeterminism in TestCppBitfields (authored by labath).
[lldb] Fix nondeterminism in TestCppBitfields
Fri, Jan 24, 3:22 AM
labath added inline comments to D72953: Fix the handling of unnamed bit-fields when parsing DWARF.
Fri, Jan 24, 3:22 AM · Restricted Project
labath added a comment to D70840: [LLDB] [DWARF] Strip out the thumb bit from addresses on ARM.

Yes, I was keeping this problem in mind when I was working on that patch. :) I believe more work could be done to reduce the number of places that parse DW_AT_low/high_pc, but I haven't gotten around to that yet..

Fri, Jan 24, 1:36 AM · Restricted Project
labath added a comment to D73206: `DWARFASTParserClang::m_decl_ctx_to_die` `DWARFDIE`->`DIERef` for DWZ.

BTW, I am currently trying to clean up the dwo/dwp code, and here (I think) I ran into a very similar problem to what you are encountering.

Fri, Jan 24, 1:25 AM · Restricted Project
labath added a comment to D72751: [LLDB] Add DynamicLoaderWasmDYLD plugin for WebAssembly debugging.

Yeah, I'm not sure why the LoadModules function is calling target.SetExecutableModule. It is true that the libraries-svr4 will not include the main executable in its list.
This code was added in the context of providing qXfer:libraries support here: https://reviews.llvm.org/D9471. I don't see any mention of including the executable on that packet though: https://sourceware.org/gdb/current/onlinedocs/gdb/Library-List-Format.html. @clayborg was the main reviewer there (although this was 5 years ago or so) and he does mention multiple times in the comments this exact issue with calling target.SetExecutableModule. Maybe he can still remember and provide some light here :).

Fri, Jan 24, 12:57 AM · Restricted Project
labath added a comment to D72751: [LLDB] Add DynamicLoaderWasmDYLD plugin for WebAssembly debugging.

Thanks for the explanation! I wasn't quite clear on "executable module" here, but after your comments I realized that Target::SetExecutableModule() should not probably be called also for Wasm modules.
The point is that ObjectFileWasm::CalculateType() should return eTypeSharedLibrary, not eTypeExecutable.
With this change the first issue is easily solved: we just need to call Target::SetSectionLoadAddress() once, in ObjectFileWasm::SetLoadAddress() because Target::SetExecutableModule() -> Target::ClearModules() -> SectionLoadList::Clear() is not called, and DynamicLoaderWasmDYLD::DidAttach() can be simplified to just call ProcessGDBRemote::LoadModules().
Does this solution work for you?

Fri, Jan 24, 12:47 AM · Restricted Project

Thu, Jan 23

labath added a comment to D73206: `DWARFASTParserClang::m_decl_ctx_to_die` `DWARFDIE`->`DIERef` for DWZ.

It's been a while since we had that discussion, but the way I'm remembering it, sizeof(DWARFDIE) was not my biggest concern with this stuff (though I may have been echoing the concerns of some other people). What I was trying to prevent is diverging even more from the llvm dwarf implementation. In that sense, changing lldb's DWARFDIE without a matching change to the llvm counterpart is equally bad. I don't think the first two options are ever going to make it into llvm. The third might, but even that may be a hard sell -- a slightly more tractable option might be to reuse the low-order bits in the DWARFDIE via PointerIntPair. We might be able to get those eight bits that way.

Thu, Jan 23, 5:44 AM · Restricted Project
labath added a comment to D73191: Only match mangled name in full-name function lookup (with accelerators).

This doesn't sound right. Will that mean we won't be able to find a method even if we search for it by its proper full mangled name (e.g. _ZZ5ffbarvEN4sbaz3fooEv in this case)?

Thu, Jan 23, 2:58 AM · Restricted Project
labath updated subscribers of D72751: [LLDB] Add DynamicLoaderWasmDYLD plugin for WebAssembly debugging.

[ looping in @aadsm for the svr4 stuff ]

Thu, Jan 23, 1:33 AM · Restricted Project

Wed, Jan 22

labath added a comment to D73148: [lldb/Value] Report size of Value as size of underlying data buffer.
In D73148#1834955, @vsk wrote:

Actually it would be nice to have a test that will trigger on at least one build bot that runs ASAN?

I'll add an end-to-end test for DW_OP_piece, though I worry it might be brittle.

Wed, Jan 22, 11:36 PM
labath added a comment to D73119: [lldb/Initializers] Rename plugins to match their entry points.

As for AppleObjCRuntime, I'm not insisting on changing that, though I am wondering if that won't get it your way when autogenerating the initalizers. I'm not fully sure what are your plans for that. If you're going to generate the #include lines then it looks like this discrepancy will matter. If you're going the "extern" route, then generating #include is not needed and you headers can be called anything. With the global constructor approach (my favourite :P) we wouldn't need to autogenerate anything at all...

As much as I personally like the idea of the global constructors, I don't see how it could work. There's no guarantee in initialization order, so you might end up initializing some of the plugins before the plugin manager itself is initialized. Although definitely a bug, it has come up in the past that the initialization order amongst the plugins matters as well. Finally, it doesn't offer a solution to terminating them again. Maybe you've already thought about all this?

Wed, Jan 22, 12:14 PM · Restricted Project
labath committed rG3d7177acd751: [lldb/DWARF] Remove one more auto-dwo method (authored by labath).
[lldb/DWARF] Remove one more auto-dwo method
Wed, Jan 22, 4:08 AM
labath closed D73112: [lldb/DWARF] Remove one more auto-dwo method.
Wed, Jan 22, 4:07 AM · Restricted Project
labath added a comment to D73125: [lldb/Plugins] Move entry points out of plugin namespace.

A weaker for of that would be to do something like using ProcessGDBRemote = process_gdb_remote::ProcessGDBRemote, which would require less changes to existing plugins, but it would still require us to include all the headers...

Wed, Jan 22, 3:13 AM · Restricted Project
labath added a comment to D73125: [lldb/Plugins] Move entry points out of plugin namespace.

Some of these plugins are very simple (a single class) and so having the namespace there is not that useful. However, for the not-so-simple plugins, it seems strange to have one part of it be in the namespace, and one part outside. E.g., it's unfortunate that that ProcessGDBRemote now has to qualify all references to ThreadGDBRemote, as they are both coupled closely together.

Wed, Jan 22, 3:04 AM · Restricted Project
labath added a comment to D73119: [lldb/Initializers] Rename plugins to match their entry points.

I think the remaining discrepancies between the plugin name and the directory make sense. For example, I don't really see the benefit of renaming AppleObjCRuntime to LanguageRuntimeAppleObjeC. The ClangASTContext is the exception, but I really don't want to rename that class :-)

Wed, Jan 22, 2:36 AM · Restricted Project
labath added a comment to D65282: ObjectFileELF: permit thread-local sections with overlapping file addresses.

It took me a while, but I tracked this down to the lack of % in front of lldb in the RUN: commands. We don't have an "lldb" substitution, so this ends up running whatever it finds on the path. Normally this does not matter because we add the build dir to the lit path, but for some reason this is not happening in a standalone build (wild guess: probably we just add the "llvm" build dir).

Wed, Jan 22, 2:08 AM · Restricted Project
labath committed rG889a4f55c910: [lldb] s/lldb/%lldb in two tests (authored by labath).
[lldb] s/lldb/%lldb in two tests
Wed, Jan 22, 2:03 AM
labath committed rG0157a74bec3d: [lldb] Fix an asan error from 27df2d9f556c (authored by labath).
[lldb] Fix an asan error from 27df2d9f556c
Wed, Jan 22, 1:18 AM
labath committed rG935729e4c63a: Change the googlemock link (authored by s-nil).
Change the googlemock link
Wed, Jan 22, 1:18 AM
labath closed D73082: Changes the googlemock link.
Wed, Jan 22, 1:18 AM · Restricted Project
labath added a comment to D71770: [lldb] Don't process symlinks deep inside DWARFUnit.

Thanks. I believe this should be fixed by 0157a74be. I'll check the bot to confirm.

Wed, Jan 22, 1:18 AM · Restricted Project
labath added a comment to D73082: Changes the googlemock link.

Done with 935729e4c63a0. Thanks for the fix.

Wed, Jan 22, 1:18 AM · Restricted Project
labath accepted D73082: Changes the googlemock link.

Simple enough. Do you have commit access or do you need me to do that?

Wed, Jan 22, 12:35 AM · Restricted Project

Tue, Jan 21

labath added a comment to D73119: [lldb/Initializers] Rename plugins to match their entry points.

There are three names to consider here:

  • the name of the cmake target
  • the name of the main .h file
  • the name of the folders the plugin is in
Tue, Jan 21, 9:51 AM · Restricted Project
labath accepted D73116: [lldb/Initializers] Move all macOS initializers into PlatformMacOSX.

this seems fine to me.

Tue, Jan 21, 9:22 AM · Restricted Project
labath created D73112: [lldb/DWARF] Remove one more auto-dwo method.
Tue, Jan 21, 8:12 AM · Restricted Project
labath added a comment to D73067: [lldb/CMake] Auto-generate the Initialize and Terminate calls for plugins (WIP).

Since the initialization function will have to have predictable names anyway, instead of generating the #include directives, we could just automatically forward declare the needed functions. So, the .def file would expand to something like:

extern lldb_initialize_objectfile_elf();
lldb_initialize_objectfile_elf();
extern lldb_initialize_objectfile_macho();
lldb_initialize_objectfile_macho();
...

That way we would have only one generated file, and we could ditch the Initialization.h file for every plugin (which would contain just the two forward declarations anyway). And this approach would be pretty close to what we'd need to do for dynamically loaded plugins (where instead of an extern declaration we'd have a dlsym lookup).

Tue, Jan 21, 8:05 AM · Restricted Project
labath committed rG5e70f4bdc159: [lldb/breakpad] Use new line table constructor (authored by labath).
[lldb/breakpad] Use new line table constructor
Tue, Jan 21, 6:13 AM
labath committed rG18a96fd573b1: [lldb/DWARF] Fix a leak in line table construction (authored by labath).
[lldb/DWARF] Fix a leak in line table construction
Tue, Jan 21, 5:54 AM
labath committed rG3f9b6b270f87: [lldb] Use llvm::stable_sort in Line (authored by labath).
[lldb] Use llvm::stable_sort in Line
Tue, Jan 21, 5:17 AM
labath added inline comments to D72909: Make SymbolFileDWARF::ParseLineTable use std::sort instead of insertion sort.
Tue, Jan 21, 5:17 AM · Restricted Project
labath added a comment to D73018: [lldb] Add SystemInitializerAllPlugins and delete copy-pasted Init code in SystemInitializerFull and SystemInitializerTest.

I just thought of another corner case (which may be important for the other patch too): Plugins/Process/Linux and Plugins/Process/NetBSD are used in lldb-server and are not/should not be used in liblldb (or lldb-test). Plugins/Process/Windows is even weirder because it's used from both :/.

Tue, Jan 21, 4:58 AM · Restricted Project
labath added a comment to D73018: [lldb] Add SystemInitializerAllPlugins and delete copy-pasted Init code in SystemInitializerFull and SystemInitializerTest.

This still leaves the question of the script interpreter plugins, which are suspiciously *not* included in "all plugins". The script interpreters are quite special, so I think it's fine to handle them separately -- the question is just how to convey that distinction. Move them into a different top level folder? Call this SystemInitializerMostPlugins ?

Yeah, those 'plugins' aren't actually standalone plugins but require the SWIG wrapper code to link (and the SWIG wrapper code is compiled in the API/ folder). We can fix this by moving the SWIG code into the AllPlugins folder (which would either be done in this commit or as a follow-up).

Tue, Jan 21, 4:57 AM · Restricted Project
labath added inline comments to D68961: Add support for DW_AT_export_symbols for anonymous structs .
Tue, Jan 21, 4:25 AM · Restricted Project
labath added a comment to D73018: [lldb] Add SystemInitializerAllPlugins and delete copy-pasted Init code in SystemInitializerFull and SystemInitializerTest.

I moved the single non-plugin call back to the original Full/Test subclasses so the name is now correct. Also I removed all the duplicated linking flags that I forgot to remove before.

Tue, Jan 21, 2:38 AM · Restricted Project
labath added a comment to D73067: [lldb/CMake] Auto-generate the Initialize and Terminate calls for plugins (WIP).

Overall, I like this, but I have a lot of comments:

  • I actually very much like the plugin namespace idea -- it makes it harder to use plugin code from non-plugin code, and allows plugins of the same category (e.g. ProcessLinux and ProcessNetBSD) to define similar concepts without fear of name clashes. I do see how that gets in the way of auto-generation though, so putting the initialization endpoints into a more predictable place seems like a good compromise. I wouldn't put the entire main class into the lldb_private namespace though, as that is also something that should not be accessed by generic code. Ideally, I'd just take the Initialize and Terminate (TBH, I don't think we really need the terminate function, but that may be more refactoring than you're ready for right now), and put it into a completely separate, predictibly-named file (void lldb_private::InitializeProcessLinux() in Plugins/Process/Linux/Initialization.h ?). That way the "main" file could be free to include anything it wants, without fear of polluting the namespace of anything, and we could get rid of the ScriptInterpreterPythonImpl thingy.
  • it would be nice to auto-generate the #include directives too. Including everything and then not using it sort of works, but it does not feel right. It should be fairly easy to generate an additional .def file with just the includes...
  • The way you disable ScriptInterpreterPython/Lua plugins is pretty hacky. Maybe the .def file should offer a way to exclude entire plugin classes?
#ifdef LLDB_WANT_SCRIPT_INTERPRETERS
ScriptInterpreterPython::Initialize(); // or whatever
#endif
  • some of the things you initialize are definitely not plugins (e.g. Plugins/Process/Utilty, Plugins/Language/ClangCommon). I think that things which don't need to register their entry points anywhere should not need to have the Initialize/Terminate goo... Can we avoid that, perhaps by making the "plugin" special at the cmake level. Since this code is used only by other plugins, it makes sense to have it somewhere under Plugins/, but it is not really a plugin, it does not need to register anything, and we do not need to be able to explicitly disable it (as it will get enabled/disabled automatically when used by the other plugins).
  • Having this as one bit patch is fine for now, but once we agree on the general idea, it should be split off into smaller patches, as some of the changes are not completely trivial (like the bunching of the macos platform plugins for instance)
Tue, Jan 21, 2:20 AM · Restricted Project
labath added a comment to D73016: [lldb/CMake] Make it possible to disable plugins at configuration time.
  • I'm aware of the linker issue when you reconfigure. I've spent quite some time investigating this and this seems related to the way libraries to link are cached by CMake. I verified that we're no longer passing the disabled library to target_link_libraries and yet the linker error remains. It looks like this might be fix starting with CMake 3.12 (https://cmake.org/cmake/help/git-stage/policy/CMP0073.html).
Tue, Jan 21, 1:17 AM · Restricted Project
labath added a comment to D72748: [lldb/IOHandler] Change the way we manage IO handler.

I didn't actually try it but I am pretty sure this will deadlock with nested lldb command files (running command source from a file that is itself being sourced). Changing the mutex to a recursive_mutex would fix that, but I don't believe it would make this fully correct -- it would just make it harder to demonstrate that it's wrong. OTOH, that may be the best thing we can do in the current state of affairs.

The thing I don't understand now is why do we even need this stack in the first place. It seems like this could be handled by just running a new iohandler "main loop" instead of pushing something. Take the "expr" command for example. In the single-line mode it evaluates the expression synchronously, but in a multi-line expression, it returns immediately after pushing it's own IOHandler (which then gathers the expression and calls back into the command to run it). I don't see why we couldn't achieve the same thing by "running" the iohandler directly, instead of pushing it to some stack and waiting for it to be executed at the top level. The same thing could be said for the "script" command and various other things which "hijack" the main (lldb) iohandler.

Isn't the problem that you can't be sure your IO handler pushes another one on top of the stack? I considered an alternative implementation, where the synchronous IO handlers has its own stack and everything that's pushed while it is executing ends up on that stack. It adds a lot of complexity and you still need to synchronize with the "main loop"

Tue, Jan 21, 1:08 AM · Restricted Project

Mon, Jan 20

labath committed rG468ca490c603: [lldb] Allow loading of minidumps with no process id (authored by labath).
[lldb] Allow loading of minidumps with no process id
Mon, Jan 20, 4:17 AM
labath closed D70238: [lldb] Allow loading of minidumps with no process id.
Mon, Jan 20, 4:17 AM · Restricted Project
labath committed rG27df2d9f556c: [lldb] Don't process symlinks deep inside DWARFUnit (authored by labath).
[lldb] Don't process symlinks deep inside DWARFUnit
Mon, Jan 20, 4:08 AM
labath closed D71770: [lldb] Don't process symlinks deep inside DWARFUnit.
Mon, Jan 20, 4:08 AM · Restricted Project
labath added inline comments to D71770: [lldb] Don't process symlinks deep inside DWARFUnit.
Mon, Jan 20, 4:07 AM · Restricted Project
labath committed rG39f1335486ea: Make SymbolFileDWARF::ParseLineTable use std::sort instead of insertion sort (authored by unnar).
Make SymbolFileDWARF::ParseLineTable use std::sort instead of insertion sort
Mon, Jan 20, 3:58 AM
labath closed D72909: Make SymbolFileDWARF::ParseLineTable use std::sort instead of insertion sort.
Mon, Jan 20, 3:58 AM · Restricted Project
labath committed rGb7af1bfa6e31: [lldb/DWARF] Simplify DWARFDebugInfoEntry::LookupAddress (authored by labath).
[lldb/DWARF] Simplify DWARFDebugInfoEntry::LookupAddress
Mon, Jan 20, 3:49 AM
labath closed D72920: [lldb/DWARF] Simplify DWARFDebugInfoEntry::LookupAddress.
Mon, Jan 20, 3:49 AM · Restricted Project
labath added a comment to D72920: [lldb/DWARF] Simplify DWARFDebugInfoEntry::LookupAddress.

LGTM. Much cleaner using the newer DWARFDIE code. Not sure if we can unit test this?

Mon, Jan 20, 3:39 AM · Restricted Project
labath committed rG06e73f071ae1: [lldb/DWARF] Change how we construct a llvm::DWARFContext (authored by labath).
[lldb/DWARF] Change how we construct a llvm::DWARFContext
Mon, Jan 20, 2:53 AM
labath closed D72917: [lldb/DWARF] Change how we construct a llvm::DWARFContext.
Mon, Jan 20, 2:53 AM · Restricted Project
labath added inline comments to D72917: [lldb/DWARF] Change how we construct a llvm::DWARFContext.
Mon, Jan 20, 2:53 AM · Restricted Project
labath added a comment to D73018: [lldb] Add SystemInitializerAllPlugins and delete copy-pasted Init code in SystemInitializerFull and SystemInitializerTest.

This seems like a good idea, but I really don't like the AllPlugins name, when it does not, in fact, initialize all plugins. I'm not really sure what to name it because criterion is pretty complex (though reasonable).

Mon, Jan 20, 1:21 AM · Restricted Project
labath added a comment to D73016: [lldb/CMake] Make it possible to disable plugins at configuration time.

In principle, I think this is a good idea. We do have many optional components (and even more components that should be optional but aren't), that one may not need/want when building lldb for a specific use case. Also, libLLVM is configurable in a similar way. However:

  • as Raphael pointed out, this is incomplete. I'm not too worried about managing dependencies -- I'd consider this an expert feature, and I am happy with leaving it up to the user to ensure he does not disable a plugin that is used elsewhere (he should get a link error if he does). But that still leaves the question of how to register the plugins at startup. We currently have a hand-curated list of plugins in the SystemInitializer classes, and that will need to change somehow. This is further complicated by the fact that some plugins are also needed for lldb-server, while others (most) are used in liblldb only. The simplest solution (for usage, but maybe not for implementation) would be to have the plugins auto-register themselves via a global constructor. Some parts of llvm already to that, and we could create a special class for this which would also carry the annotations necessary to surpress the global constructor warnings. The alternative is to throw in a bunch of ifdefs into the SystemInitializers, which isn't particularly nice (though maybe it could be generated in cmake or via some .def tricks).
  • there will also be a need to somehow disable the tests for the relevant plugins. For instance all the NativePDB tests will fail when disabling that plugin (even on darwin, as the tests are written in a way to not require a windows host). I don't think we need to annotate all tests with the exact list of plugins that the require (that can be up to whoever wants to ensure that some configuration keeps working), but we should create some mechanism to do that.
  • I also wouldn't advertise this too much. Using these settings can easily send someone into uncharted waters, with random unexpected build errors or test failures -- we have enough of those even without new settings. And unlike LLVM_TARGETS_TO_BUILD, I doubt disabling some plugins will have measurable impact on the build time...
Mon, Jan 20, 1:02 AM · Restricted Project
labath accepted D72748: [lldb/IOHandler] Change the way we manage IO handler.

Let's give this a shot. I'm don't think this is fully right, but I also don't know how to improve that, exactly...

Mon, Jan 20, 12:25 AM · Restricted Project
labath accepted D72823: [Reproducers] Add a tool to transparently capture and replay lldb sessions.
Mon, Jan 20, 12:07 AM · Restricted Project

Fri, Jan 17

labath created D72920: [lldb/DWARF] Simplify DWARFDebugInfoEntry::LookupAddress.
Fri, Jan 17, 6:30 AM · Restricted Project
labath accepted D72909: Make SymbolFileDWARF::ParseLineTable use std::sort instead of insertion sort.

Yes, that's looks pretty much like it, but it seems you uploaded the diff incorrectly -- it looks like its based on the previous version of your patch and not master (you should always upload the full set of changes not just the recent additions).

Fri, Jan 17, 6:20 AM · Restricted Project
labath added inline comments to D72751: [LLDB] Add DynamicLoaderWasmDYLD plugin for WebAssembly debugging.
Fri, Jan 17, 5:51 AM · Restricted Project
labath created D72917: [lldb/DWARF] Change how we construct a llvm::DWARFContext.
Fri, Jan 17, 5:32 AM · Restricted Project
labath added a comment to D72748: [lldb/IOHandler] Change the way we manage IO handler.

Not super ideal, but not too bad either. Not clicking accept yet because of the missing test case...

Fri, Jan 17, 4:44 AM · Restricted Project
labath added a comment to D72909: Make SymbolFileDWARF::ParseLineTable use std::sort instead of insertion sort.

Thanks for the patch. I've been wondering how to improve this, and this solution is pretty neat.

Fri, Jan 17, 3:02 AM · Restricted Project
labath added inline comments to D72823: [Reproducers] Add a tool to transparently capture and replay lldb sessions.
Fri, Jan 17, 1:21 AM · Restricted Project
labath added inline comments to D72880: Fix a buffer-size bug when the first DW_OP_piece is undefined.
Fri, Jan 17, 12:49 AM · Restricted Project
labath added a comment to D72813: Fixes to lldb's eLaunchFlagLaunchInTTY feature on macOS.
If we attach while darwin-debug is executing, then we get the exec mach exception, ....
Fri, Jan 17, 12:30 AM · Restricted Project

Thu, Jan 16

labath committed rGee05138515ab: [lldb/test] Revert changes to debug-names-compressed.cpp (authored by labath).
[lldb/test] Revert changes to debug-names-compressed.cpp
Thu, Jan 16, 10:04 AM
labath committed rG15a6df52efaa: [lldb/DWARF/test] Freshen up debug_names tests (authored by labath).
[lldb/DWARF/test] Freshen up debug_names tests
Thu, Jan 16, 7:32 AM
labath added a comment to D72823: [Reproducers] Add a tool to transparently capture and replay lldb sessions.

Considered making this a python script?

Thu, Jan 16, 7:30 AM · Restricted Project
labath added a comment to D72823: [Reproducers] Add a tool to transparently capture and replay lldb sessions.

This seems like it could be useful. Some random questions inline..

Thu, Jan 16, 7:30 AM · Restricted Project
labath accepted D72650: [LLDB] Add SymbolVendorWasm plugin for WebAssembly debugging.

This looks good.

Thu, Jan 16, 7:01 AM · Restricted Project
labath accepted D72684: [lldb][NFC] Rename ClangASTContext to TypeSystemClang.

It looks like everyone is on board with this...

Thu, Jan 16, 6:51 AM · Restricted Project
labath added a comment to D72748: [lldb/IOHandler] Change the way we manage IO handler.

I didn't actually try it but I am pretty sure this will deadlock with nested lldb command files (running command source from a file that is itself being sourced). Changing the mutex to a recursive_mutex would fix that, but I don't believe it would make this fully correct -- it would just make it harder to demonstrate that it's wrong. OTOH, that may be the best thing we can do in the current state of affairs.

Thu, Jan 16, 6:33 AM · Restricted Project
labath added a comment to D71575: [LLDB] Add ObjectFileWasm plugin for WebAssembly debugging.

Do you have commit access?

No, I certainly don't have commit access, this would be my first accepted patch. :)

Thu, Jan 16, 5:23 AM · Restricted Project

Wed, Jan 15

labath accepted D70314: [lldb] Add expect_expr function for testing expression evaluation in dotests..

I think this is a great start. We can see how we can extend this later...

Wed, Jan 15, 3:24 AM · Restricted Project
labath added a comment to D70314: [lldb] Add expect_expr function for testing expression evaluation in dotests..

This seems fine, assuming it is sufficient to achieve your goals.

Wed, Jan 15, 3:05 AM · Restricted Project
labath added a comment to D72751: [LLDB] Add DynamicLoaderWasmDYLD plugin for WebAssembly debugging.

Testing dynamic loaders is a bit tricky as they require an actual process around. The best thing available to us right now is the "gdb-client" approach, which consists of mocking the responses of the gdb server. It's not the easiest way to write tests, but I don't think it should be that difficult in this case -- you shouldn't need to mock that many packets -- the main one is qXfer:libraries. Then you should be able to run something like "image lookup -a" (or SBTarget::ResolveLoadAddress, if you want to try your hand at the scripting API) and check that it resolves to the correct section+offset pair. You can look at the existing tests in packages/Python/lldbsuite/test/functionalities/gdb_remote_client/ to see how this works...

Wed, Jan 15, 1:23 AM · Restricted Project