- User Since
- Jun 4 2013, 6:02 AM (347 w, 10 h)
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).
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.
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.
Fri, Jan 24
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.
Thu, Jan 23
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 ]
Wed, Jan 22
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.
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?
Tue, Jan 21
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 :/.
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)
Mon, Jan 20
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...
Fri, Jan 17
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.
Thu, Jan 16
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.
Wed, Jan 15
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...