This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Make the LLDB version a first class citizen
ClosedPublic

Authored by JDevlieghere on Dec 6 2021, 6:22 PM.

Details

Summary

Instead of trying to squeeze GetVersion into lldb-private.h, give it its own header and implementation file in its own directory. I called it Basic, partially because the library was already called lldbBase (?) but mostly to align with clang/Basic/Version.h (and swift/Basic/Version.h) which might explain the current library name. I'm pretty flexible as far as naming goes, I think Version/Version.h would work pretty well too.

Additional benefits of this patch include that we can get rid of the quoting macros and that other place of LLDB can read the version number from Version.inc. A downstream change requiring the latter was the main motivation for this patch. I considered adding a few other functions similar to what clang offers but decided against it since there would be no existing uses for them.

Diff Detail

Event Timeline

JDevlieghere created this revision.Dec 6 2021, 6:22 PM
JDevlieghere requested review of this revision.Dec 6 2021, 6:22 PM
JDevlieghere edited the summary of this revision. (Show Details)

I am definitely not sad to see lldbBase go away, but I am wondering if we really need a separate library for this functionality (function).

The only reason I can think of is that this function forces a dependency on clang. If we put it in say Utility, then all of lldb would depend on a (very tiny) piece of clang. I don't think that most of lldb would care about that, but this isn't technically correct for lldb-server. It doesn't use clang, and only depends on it because of the general tangledness of lldb dependencies. In an ideal world one would even be able to build lldb-server without having clang checked out. However, given that:

  • we are pretty far from an ideal world, and if we ever reach it, we could make this small dependency conditional on the existence of clang
  • it's really not clear (to me) what else should belong in the Basic library

I am thinking that we could just put this stuff into Utility. It's true that the clang version stuff lives in a library called "Basic", but that's because this is the name clang chose for its lowest level library. In lldb the lowest level library is Utility.

WDYT?

tschuett added a subscriber: tschuett.EditedDec 7 2021, 2:09 AM

I believe the Basic library in Clang does not depend on other Clang libraries. It is a tail library. Do you envision a Basic, Utility, ... library that does not depend on Clang and provides basic utilities.

Maybe 2 tail libraries with utilities. One with dependencies on Clang and one without.

I am definitely not sad to see lldbBase go away, but I am wondering if we really need a separate library for this functionality (function).

The only reason I can think of is that this function forces a dependency on clang. If we put it in say Utility, then all of lldb would depend on a (very tiny) piece of clang. I don't think that most of lldb would care about that, but this isn't technically correct for lldb-server. It doesn't use clang, and only depends on it because of the general tangledness of lldb dependencies. In an ideal world one would even be able to build lldb-server without having clang checked out. However, given that:

  • we are pretty far from an ideal world, and if we ever reach it, we could make this small dependency conditional on the existence of clang
  • it's really not clear (to me) what else should belong in the Basic library

I am thinking that we could just put this stuff into Utility. It's true that the clang version stuff lives in a library called "Basic", but that's because this is the name clang chose for its lowest level library. In lldb the lowest level library is Utility.

WDYT?

Yep, the clang dependency is the reason I kept it separate, and the problem gets worse downstream where you might have other compilers (i.e. Swift, but maybe Rust one day :-) that want to be part of the version number string too. Generally speaking we are pretty far from an ideal world dependency-wise, but I think the clang dependencies is one place where Alex has made quite a bit of progress over the past years. If we don't need to have the version be available in the "lowest level library", we can do it relatively nicely by having each TypeSystem provide a compilers-specific version string and incorporating that in the final version. That stuff lives in Target, which is a bit of an awkward place for an LLDB version number. That approach wouldn't work today anyway, because we need the version in Utility for the reproducers, and while I have different plants for those, a version number is going to remain pretty critical. I can imagine other things like Greg's statistics wanting to include a version number too.

What's your main concern about making it a separate library? I'm asking because I think the tradeoff between that and linking clang/swift into Utility leans in favor of a separate library, but I'd like to better understand your perspective.

I believe the Basic library in Clang does not depend on other Clang libraries. It is a tail library. Do you envision a Basic, Utility, ... library that does not depend on Clang and provides basic utilities.

Maybe 2 tail libraries with utilities. One with dependencies on Clang and one without.

No, other than the version I can't imagine another utility that would have to rely on clang. I picked Basic just for consistency in the header path, I don't envision it being anything more than the version (which is why I floated Version as an alternative).

The main reason I wanted to avoid a separate library is because I could not imagine how would it fit into the general library landscape in lldb. It's also moderately strange to have a library for just a single function, but that is something I can live with.

If the idea is that this library would consist of just the single function ~forever, then I think that Version is a better name, and I don't really have a problem with that -- that is pretty much the status quo anyway.

One disadvantage of that is that the version number becomes hard to get from lower level parts of lldb. So far the only use we had for that were the reproducers, so if you say you can handle that, then I guess it should be fine.

One advantage of that is that it opens up the possibility for different components of lldb to report different version string. For example, lldb-server could write its own GetVersion function (which wouldn't include clang), and that would look a lot more natural if the "main" version function lives in a library that it does not even use.

Rename Basic -> Version

labath accepted this revision.Dec 7 2021, 11:37 PM
This revision is now accepted and ready to land.Dec 7 2021, 11:37 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptDec 8 2021, 3:14 PM
thakis added a subscriber: thakis.Dec 8 2021, 4:44 PM

Thanks for this change! I always wondered why this strange lldbBase target existed in lldb/source instead of what's now done in this patch.

lldb/source/CMakeLists.txt
4

should lldb.cpp be deleted too?

lldb/source/Version/Version.cpp
18 ↗(On Diff #392948)

does this intentionally use CLANG_VERSION_STRING instead of LLDB_VERSION_STRING?

(if not: remove clang/Basic/Version.h include too)

thakis added inline comments.Dec 10 2021, 6:01 AM
lldb/source/CMakeLists.txt
4

fixed in D115438

lldb/source/Version/Version.cpp
18 ↗(On Diff #392948)

This is still wrong, I think.

(The include is needed for the clang::getclangRevision() call in lldb_private::GetVersion() though.)

JDevlieghere added inline comments.Dec 10 2021, 8:58 AM
lldb/source/Version/Version.cpp
18 ↗(On Diff #392948)

Yes, I think this is a remnant from before we computed LLDB_VERSION_* correctly in CMake. I'll put up a patch.