clang: Also detect Clang builtin dirs at runtime on Unix
ClosedPublic

Authored by rjvbb on Dec 29 2018, 7:24 PM.

Details

Summary

This is an extension to D15955, introducing dynamic determination of the clang builtin directory on Unix.

The builtin directory on typical Unix installs takes the for of /path/to/llvm-$clangMajor.0/lib/clang/$clangMajor.0.$clangMinor/include (e.g. /usr/lib/llvm-6.0/lib/clang/6.0.1/include for the offical LLVM packages for Debian/Ubuntu, or /opt/local/libexec/llvm-6.0/lib/clang/6.0.1/include for the MacPorts clang versions on Mac).

Minor version upgrades only change the penultimate path component. This patch updates the version in the hardcoded path to the version of the libclang actually used by dropping the last 2 components of the path and then adding the correct components.

This modification removes the need to rebuild all KDevelop packages to account for a single string change when a libclang update is pushed.

BUG: 402628

Test Plan

Works as expected: recreates the identical version if libclang hasn't been upgraded. A rebuilt kdev-clang plugin with a tweaked KDEV_CLANG_BUILTIN_DIR macro in $build.dir/plugins/clang/libclang_include_path.h (e.g. 6.0.0 instead of 6.0.1) still finds the correct header directory.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 6461
Build 6479: arc lint + arc unit
rjvbb requested review of this revision.Dec 29 2018, 7:24 PM
rjvbb created this revision.
flherne requested changes to this revision.Dec 29 2018, 8:21 PM
flherne added a subscriber: flherne.

I'm opposed to the concept of this patch.

Users shouldn't change the dependencies and expect things to work without a rebuild. KDevelop has a lot of build-time conditionals, so that will never be a safe thing to do in general.

Adding yet another codepath so that changing //one specific library/ 'works' (provided that the CIndex values didn't change, and we didn't add some version-specific workaround, and the path format remains the same in the future...) only encourages people to try that and generate further bug reports.

We should explicitly advise users not to do this in the first place, not add random hacks to workaround arbitrary problems resulting from it.

This revision now requires changes to proceed.Dec 29 2018, 8:21 PM
Users shouldn't change the dependencies and expect things to work without a rebuild.

That is not at all the idea behind this patch. The goal is that you can apply minor compiler updates without having to rebuild.
The modification only allows very specific changes to the install location, which should arise only as the result of a clang update.

I invite you to research the code to see for yourself where the string we're discussing here is used: only in a single location where libclang has to be told where to find its own private headers. I don't even have words for the idea that it could be normal or acceptable to require the rebuild of something as large as KDevelop is a single character changes in that one sad string. Yes, I know about ccache and other optimisations those of us who build KDevelop from source at least once a week can exploit. That's irrelevant for all the package maintainers of all the distributions out there who'd like to ship KDevelop with an up-to-date libclang (and probably the system copy) ... and the bandwidth used to download all those copies of the rebuilt packages.

The concern that the path format could change is legitimate but unless we already know that and how it will I'd say let's take that bridge when we get to it. Either way we'll most likely end up using the hardcoded string in that case because the chance that my patch serves up an inappropriate but existing path if the format changes must be homeopatically slim.

The env. variable that allows to override the builtin path can wreak much more havoc, maybe it should be removed too if you're so concerned about experimenting users and randomness?!

flherne accepted this revision as: flherne.Jan 8 2019, 12:27 PM

On further consideration, I think I was wrong about this, sorry.

Minor versions aren't likely to break anything of ours and there are various platform-specific conditionals already.

I don't see any code problem with the patch. Perhaps the check found in plugins/clang/clangsupport.cpp:185 should be moved into this function, so that we can try the fallback paths in case the first-chosen dir somehow fails to contain the headers, but that also applies to the current code.

mwolff requested changes to this revision.Jan 8 2019, 3:25 PM
mwolff added a subscriber: mwolff.

+1 in general

plugins/clang/duchain/clanghelpers.cpp
397

does the following simplified code work as well?

auto dir = QDir(QStringLiteral(KDEV_CLANG_BUILTIN_DIR);
if (dir.cd(QStringLiteral("../../%1/include").arg(clangVersion()) {
    clangDebug() << "Using builtin dir:" << dir.currentPath();
    return dir.currentPath();
}
arrowd added a subscriber: arrowd.Jan 8 2019, 5:08 PM

Sorry for being late to the party. Checked this on FreeBSD - no regressions.

rjvbb marked an inline comment as done.Jan 9 2019, 3:03 PM

Perhaps the check found in plugins/clang/clangsupport.cpp:185 should be moved into this function

You mean something like

dir = ...
if (QFileInfo(dir).isDir() && QFile::exists(dir + QLatin1Char('/') + headerToCheck)) {
    return dir;
}

Or more succinctly (QFile::exists() returns false or /path/to/aFile/foo, on Unix at least ;)),

dir = ...
if (QFile::exists(dir + QLatin1Char('/') + headerToCheck)) {
    return dir;
}

I've followed that idea (except on Win32 where I cannot test) but I'm not sure if the result is to be preferred over the current implementation. More code...

Come to think of it, what's with the lambda in ClangHelpers::clangBuiltinIncludePath()?

plugins/clang/duchain/clanghelpers.cpp
397

It does - after counting the braces, and when I use dir.path() (currentPath() is the app's cwd ;) )

rjvbb updated this revision to Diff 49080.Jan 9 2019, 3:16 PM
rjvbb marked an inline comment as done.

Updated as discussed in my previous comment. I've also followed flherne's suggestion to move the check for the reference file file into ClangHelpers::clangBuiltinIncludePath(). This does require some changes elsewhere because it can now fail (return an empty string).

rjvbb set the repository for this revision to R32 KDevelop.Jan 9 2019, 3:16 PM
mwolff requested changes to this revision.Jan 9 2019, 7:22 PM
mwolff added inline comments.
plugins/clang/duchain/clanghelpers.cpp
396

simplify this to

if (hardcodedDir.cd(...) && hardcodedDir.exists(entryToCheck))
plugins/clang/duchain/clanghelpers.h
115 ↗(On Diff #49080)

we should always use the same header, everytime. i.e. hardcode "cpuid" in this function and remove the argument here, and also hardcode it in the error message

plugins/clang/duchain/parsesession.cpp
277 ↗(On Diff #49080)

it cannot be empty at this point, it would indicate serious brokenness (since the plugin would be loaded even though it shouldn't be). remove this if and revert back to the old code

This revision now requires changes to proceed.Jan 9 2019, 7:22 PM
mwolff added a comment.Jan 9 2019, 7:44 PM

btw, I just tested the v1 of this patch set just now and it worked like a charm (my kdev was build against clang 7.0.0, but I got updated to 7.0.1)

rjvbb marked 3 inline comments as done.Jan 9 2019, 7:58 PM

Should I de-obfuscate and remove that lambda expression too while we're at it? Its body could be the body of clangBuiltinIncludePath() itself, AFAICT.

plugins/clang/duchain/clanghelpers.h
115 ↗(On Diff #49080)

That works too, of course. Is cpuid.h the most appropriate (least likely to be removed/renamed), or would e.g. stddef.h or stdarg.h be safer choices?

plugins/clang/duchain/parsesession.cpp
277 ↗(On Diff #49080)

There is 1 situation where this might actually occur: when you install a clang update and don't think of restarting KDevelop.

You'll probably still get errors from the plugin, but at least they'd point to the actual problem, instead of suggesting a weird bug in the code.

I can't seem to trigger it though (moving the builtin dir should have the same effect); how does one trigger the creation of a new ParseSessionData instance?

rjvbb updated this revision to Diff 49101.Jan 9 2019, 8:01 PM
rjvbb marked 2 inline comments as done.

V3

rjvbb set the repository for this revision to R32 KDevelop.Jan 9 2019, 8:01 PM
mwolff requested changes to this revision.Jan 10 2019, 8:41 AM

one more round of cleanups please

plugins/clang/duchain/clanghelpers.cpp
374

the lambda is there to store the result in the static const value that will be returned. we don't want to compute the result multiple times

386

the QFileInfo(dir).isDir() can be removed, since the file check in the line below implicitly checks that too

395

actually, thinking about this once more, it can be further simplified similar to the path above:

dir = QStringLiteral(KDEV_CLANG_BUILTIN_DIR "/../../%1/include").arg(clangVersion());
if (QFile::exists(dir + QLatin1Char('/') + entryToCheck)) {
    return dir;
}

then all code paths are basically the same in regard to the entryToCheck check, just for different base paths. This could thus be shared, too:

auto isValidDir = [](const QString &dir) {
    return QFile::exists(dir + QLatin1String("/cpuid.h"));
}

then use this in the three conditionals

This revision now requires changes to proceed.Jan 10 2019, 8:41 AM
rjvbb updated this revision to Diff 49145.Jan 10 2019, 11:20 AM
rjvbb marked 2 inline comments as done.

++V

rjvbb set the repository for this revision to R32 KDevelop.Jan 10 2019, 11:20 AM
mwolff requested changes to this revision.Jan 10 2019, 11:57 AM

I'm fine with his minus the change to return an empty path as that breaks the error message

the code comments talking about minor versions can probably just be removed - we just try to use whatever is thrown at us and if it fails, then so be it and shit happens ;-)

if you really want to check only for minor updates, you'd have to compare (via QVersionNumber) the clang version to the one we used at compile time...

plugins/clang/clangsupport.cpp
188 ↗(On Diff #49145)

the path is empty, so nothing will be printed here

please revert to the old behavior and check the validity of the returned path in the conditional, then print builtinDir here

plugins/clang/duchain/clanghelpers.cpp
412

don't change this (see above)

plugins/clang/duchain/clanghelpers.h
108 ↗(On Diff #49145)

the code isn't actually only checking for minor upgrades, or?

This revision now requires changes to proceed.Jan 10 2019, 11:57 AM
rjvbb marked 3 inline comments as done.Jan 10 2019, 3:04 PM
rjvbb added inline comments.
plugins/clang/clangsupport.cpp
188 ↗(On Diff #49145)

Doh, yes. With my previous implementation the path was never empty when clangBuiltinIncludePath() was called with its default argument. Evidently I had to forget that this was no longer true in the current implementation.

Reverting means we'll be checking the file presence twice in most cases, right? It bothers me a bit to hardcode the filename in 2 locations.

plugins/clang/duchain/clanghelpers.cpp
374

Now why does that remind me of the obfuscated C contests of old? :)

I'll add a comment so others won't make the same mistake.

395

Done. I just added a cleanPath() as in the WIN32 code path, so we don't return a path containing a "detour" (../..).

plugins/clang/duchain/clanghelpers.h
108 ↗(On Diff #49145)

No, the code just tries to generate the path corresponding to the current version from what it has. In practice this will correspond to minor upgrades because the typical path has 2 version components and we only correct one:

/usr/lib/llvm-6.0/lib/clang/6.0.1/include

We could correct the first one too but it seems very unlikely that the code will ever get to be executed after major upgrades because libclang itself will no longer be in /usr/lib/llvm-6.0/lib (but under /usr/lib/llvm-X.y or, theoretically, /usr/lib/llvm-6.y).

But yeah, if someone decides to install LLVM to a location that does NOT contain the major version number then my code should handle any upgrade or even downgrade, as long as the plugin still loads against the new libclang.

mwolff added inline comments.Jan 10 2019, 3:23 PM
plugins/clang/clangsupport.cpp
188 ↗(On Diff #49145)

ah I see. just make the check code (currently in the lambda) available through isValidClangBuiltingIncludePath - no need to hardcode it twice.

rjvbb updated this revision to Diff 49170.Jan 10 2019, 4:23 PM
rjvbb marked 2 inline comments as done.

+++V

rjvbb set the repository for this revision to R32 KDevelop.Jan 10 2019, 4:23 PM
mwolff accepted this revision.Jan 10 2019, 8:33 PM

thakns, lgtm now! please commit

plugins/clang/duchain/clanghelpers.cpp
404

this hunk could be undone now

This revision is now accepted and ready to land.Jan 10 2019, 8:33 PM
This revision was automatically updated to reflect the committed changes.