fix clang plugin highlighting screwup

Review Request #128480 - Created July 18, 2016 and submitted

Information
Sven Brauch
kdevelop
Reviewers
kdevelop
kfunk, mwolff

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).

Actually I'm not sure if the foreground lock is required in this case, because clang for some reason reads the contents in the constructor, which afaik runs in the main thread anyways.

Very hard to reproduce. Apply this patch to kdevplatform:

diff --git a/language/backgroundparser/documentchangetracker.cpp b/language/backgroundparser/documentchangetracker.cpp
index 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.

Issues

  • 2
  • 1
  • 0
  • 3
Description From Last Updated
not required here, this happens in the foreground already Milian Wolff Milian Wolff
we set the tracker revision in the parse job (see bottom of ::run), is that too late? must it happen ... Milian Wolff Milian Wolff
Sven Brauch
Sven Brauch
Milian Wolff
David Nolden
Sven Brauch
Milian Wolff
Sven Brauch
Review request changed

Status: Closed (submitted)

Change Summary:

Submitted with commit c9265847835d8d534906ae7ec42aa3784c54d0df by Sven Brauch to branch 5.0.
Loading...