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

In D70840#1844318, @mstorsjo wrote:

In D70840#1844279, @labath wrote:

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.

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.

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.

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

In D70840#1838249, @mstorsjo wrote:

In D70840#1838186, @labath wrote:

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.

Thanks for the patch.

Thanks for splitting this off.

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

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

(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).

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.

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

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

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.

In D72751#1837883, @aadsm wrote:

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 :).

In D72751#1837764, @paolosev wrote:

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?

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.

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

[ looping in @aadsm for the svr4 stuff ]

In D73148#1834955, @vsk wrote:

In D73148#1832897, @clayborg 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.

In D73119#1834279, @JDevlieghere wrote:

In D73119#1833232, @labath wrote:

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?

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

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.

In D73119#1832707, @JDevlieghere wrote:

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 :-)

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

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

Done with 935729e4c63a0. Thanks for the fix.

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

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

this seems fine to me.

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

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 :/.

In D73018#1830891, @teemperor wrote:

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

In D73018#1830697, @teemperor wrote:

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.

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)

In D73016#1830303, @JDevlieghere wrote:

• 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).

In D72748#1829956, @JDevlieghere wrote:

In D72748#1823945, @labath wrote:

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"

In D72920#1827480, @clayborg wrote:

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

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

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

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

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

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

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

In D72813#1825027, @jasonmolenda wrote:

If we attach while darwin-debug is executing, then we get the exec mach exception, ....

Considered making this a python script?

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

This looks good.

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

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.

In D71575#1822343, @paolosev wrote:

In D71575#1821312, @labath wrote:

Do you have commit access?

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

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

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

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