Page MenuHomePhabricator

[lldb] Add IsDebugInfoCompatible method to SBModule to allow checking compatibility between language versions
AbandonedPublic

Authored by teemperor on Nov 1 2019, 4:43 AM.

Details

Summary

In Swift there is a check if the compiled module actually fits to our Swift version in LLDB. We don't really do this
kind of version checking so far in LLDB as we only have one Clang-based TypeSystem for C languages.
C languages don't have this precise version semantic but different standards which are expressed as different language types,
so there is no precise version checking needed for them.

This adds the relevant compatibility check to SBModule and adds the relevant tests for it. The actual Swift version check is
implemented in Swift's TypeSystem class which is downstream, so we test this here with the Clang type system which is
always treating modules as compatible.

Diff Detail

Event Timeline

teemperor created this revision.Nov 1 2019, 4:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 1 2019, 4:43 AM
teemperor retitled this revision from Add IsTypeSystemCompatible method to SBModule to allow checking compatibility between language versions to [lldb] Add IsTypeSystemCompatible method to SBModule to allow checking compatibility between language versions.Nov 1 2019, 4:46 AM
teemperor added a reviewer: labath.
labath added a comment.Nov 4 2019, 8:07 AM

The part that bothers me here is that this assumes that the entirety of a module (or an least the portion of it written in the same language) uses the same version of the language. Is that true for swift? Because it definitely isn't true for c++, as you can easily mix compile units built with different -std flags (and most of the time it will work). So it's not clear to me how this would work for c/c++, or any other language that offers some kind of a version-independent ABI, thus, enabling one to combine different versions in a single module. Nevertheless, since this is how swift works, it seems reasonable to have some way to access this information. However, it's not clear to me if this is the best way to do that...

Another thing on my mind is the name (type system). AFAICT, this is the first mention of these words in all of lldb/API headers, so one would get the impression that type systems are an internal implementation detail of lldb. That's why this methods sort of jumps out in your face. Are we planning to make more type system-y stuff visible at the SB level? If not, could we make this name more generic? Maybe, IsDebugInfoCompatible (with the meaning that lldb is able to understand all of debug info (of a given language) in this module)?

lldb/source/API/SBModule.cpp
700–704

Do you want to store the actual error message in type_system_or_err into sb_error?

jingham added a comment.EditedNov 11 2019, 5:39 PM

The part that bothers me here is that this assumes that the entirety of a module (or an least the portion of it written in the same language) uses the same version of the language. Is that true for swift? Because it definitely isn't true for c++, as you can easily mix compile units built with different -std flags (and most of the time it will work). So it's not clear to me how this would work for c/c++, or any other language that offers some kind of a version-independent ABI, thus, enabling one to combine different versions in a single module. Nevertheless, since this is how swift works, it seems reasonable to have some way to access this information. However, it's not clear to me if this is the best way to do that...

This call is actually being used to tell whether the type system in a given module is compatible with LLDB, not with some other compile units in the same module or other modules in the program. That is an issue for Swift because the type information for Swift is not stored in DWARF, but in the .swiftmodule file, which is a fancy dump of internal compiler state. So for lldb to be able to debug a swift module, it has to have been built with exactly the same version of the swift compiler as was built into lldb, or things go very wrong. That also answers your question about swift: you have to build all the source inputs to a Swift module with the same version of the Swift compiler. There is a version neutral text interface file, but that is only able to describe the public interface to a module. It allows you to build code against extant modules from different versions of the Swift compiler. But the information about private methods are only in the binary format, and so you still need the same version of swift to glom them all together.

If, for instance, we decided to use .pcm files as a serialization form for type info you would need the same sort of check for modules backed by these pcm's - since the clang in lldb and the clang building the pcm would have to match. BTW I am NOT suggesting that would be a good idea, and this will probably always be a no-op for anything but Swift.

Another thing on my mind is the name (type system). AFAICT, this is the first mention of these words in all of lldb/API headers, so one would get the impression that type systems are an internal implementation detail of lldb. That's why this methods sort of jumps out in your face. Are we planning to make more type system-y stuff visible at the SB level? If not, could we make this name more generic? Maybe, IsDebugInfoCompatible (with the meaning that lldb is able to understand all of debug info (of a given language) in this module)?

IsDebugInfoCompatible is a more explicit name. It would be cool at some point to write a TypeSystem as a pure plugin, but we're nowhere near that. So I agree it's better to reserve TypeSystem for lldb_private if we aren't really exposing it.

Note, Xcode already uses this interface from the Swift branch, so we'll have to be careful about changing the name.

teemperor updated this revision to Diff 228873.Nov 12 2019, 5:08 AM
  • Renamed to IsDebugInfoCompatible
  • Pass on llvm::Error error message to caller.
labath accepted this revision.Nov 12 2019, 9:25 AM

I think this is fine.

If, for instance, we decided to use .pcm files as a serialization form for type info you would need the same sort of check for modules backed by these pcm's - since the clang in lldb and the clang building the pcm would have to match. BTW I am NOT suggesting that would be a good idea, and this will probably always be a no-op for anything but Swift.

Hm... If I understand modules correctly (and I admit I know very little about them), I think this actually illustrates the my reservations about this. Assuming we were fetching type info from a pcm file etc., there is no relationship between a module (shared library) and a pcm file... I would imagine a module can reference multiple pcm files, and each one could be theoretically built with a different compiler. That then brings up a question of what does it mean for lldb to be "compatible" with the module as a whole...

lldb/source/API/SBModule.cpp
700–704

Actually, you can do that by just saying sb_error.ref() = type_system_or_err.takeError() or something along those lines. :)

This revision is now accepted and ready to land.Nov 12 2019, 9:25 AM
shafik added a subscriber: shafik.Nov 12 2019, 10:36 AM
shafik added inline comments.
lldb/packages/Python/lldbsuite/test/python_api/module_compability/TestModuleDebugInfoCompatible.py
62

I note that we don't have one for C++17 but I think that is because we just don't have the enumerator although I think we could.

I think this is fine.

If, for instance, we decided to use .pcm files as a serialization form for type info you would need the same sort of check for modules backed by these pcm's - since the clang in lldb and the clang building the pcm would have to match. BTW I am NOT suggesting that would be a good idea, and this will probably always be a no-op for anything but Swift.

Hm... If I understand modules correctly (and I admit I know very little about them), I think this actually illustrates the my reservations about this. Assuming we were fetching type info from a pcm file etc., there is no relationship between a module (shared library) and a pcm file... I would imagine a module can reference multiple pcm files, and each one could be theoretically built with a different compiler. That then brings up a question of what does it mean for lldb to be "compatible" with the module as a whole...

It doesn't seem like these are mutually exclusive. If we were being exhaustive about this we would have two levels of test, (1) is all the debug information in the module uningestible and (2) are there some CU's in the module that have debug information that lldb can't ingest. If we had (2) then a language could decide "am I able to construct types from the debug info for a module that has some broken parts?" Deciding that depends on how the debug info is laid out. If the answer is yes, then (1) would return "false if all CU's return false from (2)." if no, (1) would return "false if any CU's return false from (2)".

Also, IRL if we introduce some incompatibility in the toolchain from compiler version -> lldb version, the vastly more common situation would be for all of a module to be built in an incompatible way. So having this larger scale check still seems to me useful. That lldb or some GUI using it can put up a dialog saying "This module is not compatible with lldb". If the whole module is not compatible it would be awkward to complain about it CU by CU.

Then historically, in the case of swift the part that causes problems is a whole-module resource and if that is not compatible with lldb, lldb can't construct a valid TypeSystem for that module. So we didn't need to implement (2), we just did (1). But that shouldn't stop anybody from implementing (2) should the need arise.

teemperor retitled this revision from [lldb] Add IsTypeSystemCompatible method to SBModule to allow checking compatibility between language versions to [lldb] Add IsDebugInfoCompatible method to SBModule to allow checking compatibility between language versions.Nov 12 2019, 10:53 AM
teemperor requested review of this revision.Nov 12 2019, 11:32 AM
teemperor marked an inline comment as done.

Marking this as changes needed that I don't land this without applying Pavel's comment.

lldb/source/API/SBModule.cpp
700–704

Oh, TIL!

teemperor updated this revision to Diff 229092.Nov 13 2019, 7:09 AM
  • Simplified error handling (Thanks Pavel)

It doesn't seem like these are mutually exclusive. If we were being exhaustive about this we would have two levels of test, (1) is all the debug information in the module uningestible and (2) are there some CU's in the module that have debug information that lldb can't ingest. If we had (2) then a language could decide "am I able to construct types from the debug info for a module that has some broken parts?" Deciding that depends on how the debug info is laid out. If the answer is yes, then (1) would return "false if all CU's return false from (2)." if no, (1) would return "false if any CU's return false from (2)".

Right, it's not that the two are mutually exclusive -- it's just that the semantics becomes fuzzier. You seem to think that the "natural" behavior would be to flag if "all" debug info is incompatible. My first though was that we should report if "any" debug info is incompatible.

Also, IRL if we introduce some incompatibility in the toolchain from compiler version -> lldb version, the vastly more common situation would be for all of a module to be built in an incompatible way. So having this larger scale check still seems to me useful. That lldb or some GUI using it can put up a dialog saying "This module is not compatible with lldb". If the whole module is not compatible it would be awkward to complain about it CU by CU.

I actually don't think this is that uncommon in the c++ world. While it's true that all *user* code in a given library will likely use the same compiler, a typical module will also contain some startup/glue code taken from the libc and/or compiler support libraries. It's fairly likely that these were built with a different compiler, or at least different flags of the same compiler. And while these things will often have no debug info, there are also a lot of cases when they will (android NDK ships debug info for everything, most linux distros enable one to install it optionally, etc.)

BTW, what is the expected effect of this method returning "false"? I would expect that one can still normally use this module everywhere, only it will appear as if it contains no debug info (or just a limited subset).. Is that so?

teemperor abandoned this revision.Nov 15 2019, 11:38 AM

Turns out this is actually not even used by anyone but was just (somehow?) added to the SBAPI in lldb-swift... Anyway, deleting this downstream...