<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/128480/">https://git.reviewboard.kde.org/r/128480/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 19th, 2016, 11:53 p.m. UTC, <b>Milian Wolff</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/128480/diff/2/?file=472085#file472085line169" style="color: black; font-weight: bold; text-decoration: underline;">languages/clang/duchain/clanghelpers.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">165</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">envFile</span><span class="o">-></span><span class="n">setModificationRevision</span><span class="p">(</span><span class="n">ModificationRevision</span><span class="o">::</span><span class="n">revisionForFile</span><span class="p">(</span><span class="n">context</span><span class="o">-></span><span class="n">url</span><span class="p">()));</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">169</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">auto</span> <span class="n">tracker</span> <span class="o">=</span> <span class="n">ICore</span><span class="o">::</span><span class="n">self</span><span class="p">()</span><span class="o">-></span><span class="n">languageController</span><span class="p">()</span><span class="o">-></span><span class="n">backgroundParser</span><span class="p">()</span><span class="o">-></span><span class="n">trackerForUrl</span><span class="p">(</span><span class="n">context</span><span class="o">-></span><span class="n">url</span><span class="p">());</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">we set the tracker revision in the parse job (see bottom of ::run), is that too late? must it happen before we add declarations or modify ranges?</p></pre>
</blockquote>
<p>On July 20th, 2016, 7:06 a.m. UTC, <b>Sven Brauch</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Hmm, right, didn't see that. Why do you even set it again in buildDUChain then?
Another weird thing is that emitUpdateReady() is called <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">before</em> that happens. There are too many locks and queued connections for me to see in an instant if that is still correct, it is probably ok since the URLParseLock is still held, but it's strange.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">And another thing which is imo wrong in this case is that it only sets the revision for unsaved files, i.e. where isModified() is true (see constructor of ClangParseJob). A file being modified() or not means nothing wrt. revisions, I can e.g. type some text and then press Ctrl+Z and return the document to non-modified state with that (or just type some text and press Ctrl+S before the job triggers), and then the code at the end of run() will do nothing.</p></pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Setting the correct revision before creating declarations is not required afaik, it's just important that it is a) correct and b) acquired when the highlighter runs (which is probably the case even though updateReady() is emitted before setting the correct revision, because the URLParseLock will block the highlighter until then).</p></pre>
<br />
<p>- Sven</p>
<br />
<p>On July 19th, 2016, 9:49 p.m. UTC, Sven Brauch wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KDevelop, Kevin Funk and Milian Wolff.</div>
<div>By Sven Brauch.</div>
<p style="color: grey;"><i>Updated July 19, 2016, 9:49 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdevelop
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">In the end, the issue(s) are simple:</p>
<ol style="padding: 0;text-rendering: inherit;margin: 0 0 0 2em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">we need to hold the foreground lock between reading the modification revision and reading the contents. Otherwise they can mismatch.</p>
</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">in buildDUChain(), there was</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">envFile->setModificationRevision(ModificationRevision::revisionForFile(context->url()));</p>
</li>
</ol>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">which is wrong. It sets the modification revision of the parsing environment file to the revision the file has <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">right now</em>. 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).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Very hard to reproduce. Apply this patch to kdevplatform:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%"><span style="color: #000080; font-weight: bold">diff --git a/language/backgroundparser/documentchangetracker.cpp b/language/backgroundparser/documentchangetracker.cpp</span>
<span style="color: #000080; font-weight: bold">index 294bc14..6f56250 100644</span>
<span style="color: #A00000">--- a/language/backgroundparser/documentchangetracker.cpp</span>
<span style="color: #00A000">+++ b/language/backgroundparser/documentchangetracker.cpp</span>
<span style="color: #800080; font-weight: bold">@@ -237,6 +237,11 @@ KDevelop::RangeInRevision DocumentChangeTracker::transformBetweenRevisions(KDeve</span>
{
VERIFY_FOREGROUND_LOCKED
<span style="color: #00A000">+ if ( !((fromRevision == -1 || holdingRevision(fromRevision)) && (toRevision == -1 || holdingRevision(toRevision)) ) ) {</span>
<span style="color: #00A000">+ qWarning() << "invalid transform: from" << fromRevision << "to" << toRevision</span>
<span style="color: #00A000">+ << "but not both revisions held: [from/to]:" << holdingRevision(fromRevision) << holdingRevision(toRevision);</span>
<span style="color: #00A000">+ };</span>
<span style="color: #00A000">+</span>
if((fromRevision == -1 || holdingRevision(fromRevision)) && (toRevision == -1 || holdingRevision(toRevision)))
{
m_moving->transformCursor(range.start.line, range.start.column, KTextEditor::MovingCursor::MoveOnInsert, fromRevision, toRevision);
<span style="color: #800080; font-weight: bold">@@ -348,6 +353,8 @@ void DocumentChangeTracker::unlockRevision(qint64 revision)</span>
m_moving->unlockRevision(revision);
m_revisionLocks.erase(it);
}
<span style="color: #00A000">+</span>
<span style="color: #00A000">+ qDebug() << "** clearing revision" << revision;</span>
}
qint64 RevisionLockerAndClearer::revision() const
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">You get <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">lots</em> 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.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>languages/clang/clangparsejob.cpp <span style="color: grey">(8375eb5)</span></li>
<li>languages/clang/duchain/clanghelpers.cpp <span style="color: grey">(cea8cd9)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/128480/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>