<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/125123/">https://git.reviewboard.kde.org/r/125123/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On September 10th, 2015, 4 p.m. CEST, <b>Milian Wolff</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;">where does it crash? we get the warnings even on linux but it never crashes. are there parts of this patch you want us to include or not?</p></pre>
</blockquote>
<p>On September 10th, 2015, 4:27 p.m. CEST, <b>René J.V. Bertin</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;">It crashes in the dynamic_cast from <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">f</code> to <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">folder</code>, which is why I create a fully new folder instance when there was an error in adding the path. I don't know why it crashes nor exactly on what, but for now it hasn't crashed with this approach.
For now I'm just putting this up for reference. I'm testing the patch on Linux too, and haven't even yet been able to get warnings. I was expecting the kqueue backend to be used on Linux too, but apparently another one is used there, which uses less file descriptors (or none at all?).</p></pre>
</blockquote>
<p>On September 10th, 2015, 4:42 p.m. CEST, <b>Milian Wolff</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;">Crashing in a dynamic cast means the pointer is invalid, and non-null. This should not happen, and I don't see how that is related to what you touch here. Is there an uninitialized ptr being returned? Is it dangling? And inotify does use file descriptors for every watcher as well, and there is (by default) a low limit (was it 1024?) for that in the kernel. Baloo and other apps like KDevelop need more, which is why people and distros regularly increase that limit now to a couple of ten thousands. And again - even if the limit is reached (I've seen that many times) it never crashed. Find out why it crashed (see below). This patch here looks like it's working around an issue, instead of fixing it.</p></pre>
</blockquote>
</blockquote>
<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;">I have no idea either how it is related, I only know that it only happens after QFileSystemWatcher printed several errors about too many open files. And that in turns happens a while after errno starts being set. Actually, that might mean that it's possible for <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">open()</code> to return a value that is not -1 (but still set errno), and if that's the case Qt's kqueue backend could be accepting a file descriptor that it shouldn't accept, leading to memory corruption. It wouldn't be too hard to check that theory.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Regardless, I wouldn't know how to figure out what (else) could lead to <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">f</code> being a corrupt pointer, or even to check if it's corrupt or not.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">There's another consideration: hitting and staying at the open file limit has other effects later on. I don't know if you can get away with it on Linux, but on OS X you're lucky if you can even quit the application gracefully. So I think that an internal limit on the number of watchers isn't a bad idea at all.</p></pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On September 10th, 2015, 4 p.m. CEST, <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/125123/diff/2/?file=402393#file402393line227" style="color: black; font-weight: bold; text-decoration: underline;">projectmanagers/cmake/cmakecommitchangesjob.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">void CMakeCommitChangesJob::start()</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">227</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">qFatal</span><span class="p">(</span> <span class="s">"In the wrong thread, %s:%d"</span><span class="p">,</span> <span class="n">__FILE__</span><span class="p">,</span> <span class="n">__LINE__</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;">wth? just use the assert above. if you hit that in a release build then you'll hit it in a debug build - and that should <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">not</em> happen. thus assertion is fine no need to add this for release only</p></pre>
</blockquote>
<p>On September 10th, 2015, 4:27 p.m. CEST, <b>René J.V. Bertin</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;">I didn't add these for nothing: the assert macros evaluate to noops in the release builds. The application is likely to crash in that case, of course, but in this case I find it better to abort it with an explicit message rather than crash in a random later location.</p></pre>
</blockquote>
<p>On September 10th, 2015, 4:42 p.m. CEST, <b>Milian Wolff</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;">Right, and you'll hit these issues in a debug build and fix them there. We do not litter our code base with "oh snap what if this happened?" in release mode for a reason - it's slowing things down.</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;">Well, as I said, I put this up for reference. This happens only as an indirect result of issues elsewhere, which are not reproducible and thus neigh impossible to debug. You may not see a justification to incorporate this kind of check in a release build, I disagree and will keep it as long as I am not able to find the culprit.</p></pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On September 10th, 2015, 4 p.m. CEST, <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/125123/diff/2/?file=402393#file402393line233" style="color: black; font-weight: bold; text-decoration: underline;">projectmanagers/cmake/cmakecommitchangesjob.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">void CMakeCommitChangesJob::start()</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">224</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">m_manager</span><span class="o">-></span><span class="n">addWatcher</span><span class="p">(</span><span class="n">m_project</span><span class="p">,</span> <span class="n">m_path</span><span class="p">.</span><span class="n">toLocalFile</span><span class="p">());</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">233</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">m_manager</span><span class="o">-></span><span class="n">addWatcher</span><span class="p">(</span><span class="n">m_project</span><span class="p">,</span> <span class="n">m_path</span><span class="p">.</span><span class="n">toLocalFile</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;">in Qt 5 you can simply use the return value of this function. Why do you keep working on Qt 4 based KDevplatform on Mac - I really don't get it... You are making your life unneccessarily complicated.</p></pre>
</blockquote>
<p>On September 10th, 2015, 4:27 p.m. CEST, <b>René J.V. Bertin</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;">It's really not hard to get once you remember that KF5 doesn't work on OS X. Parts of it build in CI, but that's about it. I've more or less given up hope that this situation will approve anytime soon.</p></pre>
</blockquote>
<p>On September 10th, 2015, 4:42 p.m. CEST, <b>Milian Wolff</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;">Your valuable OS X time would be much better spent on making KF5 work, than fixing random things like this one on Qt 4 / KDE4 / KDevelop 4.</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;">You may be right, but that's not a one-man task. I've proposed a patch to deblock what's probably the central problem, but nobody seems interested in picking it up (and I'm not interested in pursuing any other option until I'm convinced that that will indeed lead to working KF5 apps that don't live in a kf5-only universe). All the more so because I'm not sure how long I'll still be working under OS X (given how Apple no longer make computers I'd consider buying) but even on Linux/*BSD I'm staying away from KF5 desktops until one appears in a Ubuntu-based LTS.</p></pre>
<br />
<p>- René J.V.</p>
<br />
<p>On September 9th, 2015, 10:18 p.m. CEST, René J.V. Bertin 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 KDE Software on Mac OS X and KDevelop.</div>
<div>By René J.V. Bertin.</div>
<p style="color: grey;"><i>Updated Sept. 9, 2015, 10:18 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;">This is a confirmed bug in the 4.7 branch (and possibly in the KF5 version) where the cmake project importer can add (many) more paths to QFileSystemWatcher objects than the maximum supported number of open files (https://bugs.kde.org/show_bug.cgi?id=341551). On OS X at least this leads almost inevitably to a crash.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This patch prevents this by keeping track of the number of added paths, and by not adding more than a preconfigured maximum (determined empirically in this draft implementation).
The crash occurs in <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">folder = dynamic_cast<CMakeFolderItem*>(f);</code> in <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">CMakeCommitChangesJob::makeChanges()</code>; as an additional protection that function watches the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">errno</code> variable for errors occurring during the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">addPath()</code> operation; if an error is signalled, <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">folder</code> is not obtained by dynamic casting but by initialision a new <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">CMakeFolderItem</code>.
<code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">makeChanges()</code> also contains a few ASSERT statements that should remain effective even when building with <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">QT_NO_DEBUG</code>; the patch takes care of that as well.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'm presenting this mostly for reference, in hope it may be useful for Kdevelop/KF5 too. In a real implementation the maximum number of patch watchers would of course be configurable.</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;">On OS X 10.9.5 with kdelibs 4.14.11 and Qt 4.8.7 .
m_MaxAllowedWatchedPaths is set to a value that is reasonably close to the limit where errors start occurring, and large enough to allow loading for instance kdepimlibs, kdepim and kdepim-runtime in a single session.</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>projectmanagers/cmake/cmakecommitchangesjob.cpp <span style="color: grey">(7ce24fb)</span></li>
<li>projectmanagers/cmake/cmakemanager.h <span style="color: grey">(19fc0c1)</span></li>
<li>projectmanagers/cmake/cmakemanager.cpp <span style="color: grey">(2caecdb)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/125123/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>