Review Request 128480: fix clang plugin highlighting screwup

[David Nolden]

Since reviewboard is currently somehow broken, I'm writing this by e-mail.

While reviewing the clang duchain code, I found some severe problems which
are probably the main reason for these glitches to appear.

The core of the problem is, that the code assumes that for "unmodifed"
files, it doesn't need to do any revision mapping, which is wrong. When you
save a file in revision 10, the revision will still be 10 after saving, and
the mapping still needs to be performed in the same way.

Some of these things were done on-demand with the foreground lock in the
old cpp plugin, but that isn't possible straightforwardly any more, because
the "unsaved files" need to be prepared _before_ parsing now.

So in principle, what needs to be done is:
* Before parsing, either in foreground or with the foreground lock, the
text of _all_ open documents need to be read, and the exact corresponding
document revisions need to be locked and stored in the parse session. When
the extracted text is used for parsing, then this can be used for
glitch-free highlighting and navigation.

Greetings, David

2016-07-19 1:21 GMT+02:00 Sven Brauch <mail at svenbrauch.de>:

> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128480/
> Review request for KDevelop, Kevin Funk and Milian Wolff.
> By Sven Brauch.
> *Repository: * kdevelop
> Description
>
> Terrible patch, I know. Sorry. It cost a lot of nerves and time though and I want somebody to look at / try out whether this is a working fix in principle.
>
> In the end, the issue(s) are simple:
>
>    1.
>
>    we need to hold the foreground lock between reading the modification revision and reading the contents. Otherwise they can mismatch.
>    2.
>
>    in buildDUChain(), there was
>
>    envFile->setModificationRevision(ModificationRevision::revisionForFile(context->url()));
>
> which is wrong. It sets the modification revision of the parsing environment file to the revision the file has *right now*. The contents, however, were read before in the constructor, and several milliseconds might have passed until this line is reached, with no locks held to prevent anything.
> Instead, we have to set the revision to the one which was stored when the tracker was last reset (which happened right after reading the contents, coupled to the contents by the foreground lock).
>
> Testing
>
> Very hard to reproduce. Apply this patch to kdevplatform:
>
> diff --git a/language/backgroundparser/documentchangetracker.cpp b/language/backgroundparser/documentchangetracker.cppindex 294bc14..6f56250 100644--- a/language/backgroundparser/documentchangetracker.cpp+++ b/language/backgroundparser/documentchangetracker.cpp@@ -237,6 +237,11 @@ KDevelop::RangeInRevision DocumentChangeTracker::transformBetweenRevisions(KDeve
> {
>     VERIFY_FOREGROUND_LOCKED
> +    if ( !((fromRevision == -1 || holdingRevision(fromRevision)) && (toRevision == -1 || holdingRevision(toRevision)) ) ) {+        qWarning() << "invalid transform: from" << fromRevision << "to" << toRevision+                   << "but not both revisions held: [from/to]:" << holdingRevision(fromRevision) << holdingRevision(toRevision);+    };+
>     if((fromRevision == -1 || holdingRevision(fromRevision)) && (toRevision == -1 || holdingRevision(toRevision)))
>     {
>         m_moving->transformCursor(range.start.line, range.start.column, KTextEditor::MovingCursor::MoveOnInsert, fromRevision, toRevision);@@ -348,6 +353,8 @@ void DocumentChangeTracker::unlockRevision(qint64 revision)
>         m_moving->unlockRevision(revision);
>         m_revisionLocks.erase(it);
>     }++    qDebug() << "** clearing revision" << revision;
> }
>
> qint64 RevisionLockerAndClearer::revision() const
>
> You get *lots* of those warnings iff you manage to trigger the issue. The
> best bet seems to be open a relatively large cpp file, and keep removing
> text and readding it with Ctrl+Z with varying wait times in between
> (500ms-parse-delay-ish). Do that for a minute or two and it will trigger
> eventually.
>
> You still get a few of the warning messages sometimes even with the patch
> applied (but far more without). I think they are from the problem reporter
> plugin, and I'm not sure if the plugin does something wrong or the parse
> job.
>
> Diffs
>
>    - languages/clang/clangparsejob.cpp (8375eb5)
>    - languages/clang/duchain/clanghelpers.cpp (cea8cd9)
>
> View Diff <https://git.reviewboard.kde.org/r/128480/diff/>
>
> _______________________________________________
> KDevelop-devel mailing list
> KDevelop-devel at kde.org
> https://mail.kde.org/mailman/listinfo/kdevelop-devel
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20160720/071427aa/attachment.html>