[prev in list] [next in list] [prev in thread] [next in thread]
List: kdevelop-devel
Subject: Re: Review Request: Include resolving using 'make' optional
From: "Andreas Pakulat" <apaku () gmx ! de>
Date: 2012-05-16 17:41:49
Message-ID: 20120516174149.11287.83900 () vidsolbach ! de
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
> On May 14, 2012, 1:15 a.m., Alexandre Courbot wrote:
> > I noticed that this patch has been reverted a while ago:
> >
> > commit 17fa62314a30071296533e8e1cd53d9fca730f18
> > Author: Milian Wolff <mail@milianw.de>
> > Date: Tue Mar 20 17:02:20 2012 +0100
> >
> > Revert "make include-path resolution using make optional on a per-project basis"
> >
> > include-path auto completion creates a includepath computer in the background
> > thread which would break this patch (and assert, actually).
> >
> > shows that we desparately need a unit test for this. I'll revive the patch
> > once I have time to fix it properly.
> >
> > This reverts commit ff1744bcb8b125f6bcb0227925973a59d176de39.
> >
> > Milian, could you elaborate on what needs to be fixed for this patch to make it \
> > again? I'd like to give it a try. Thanks.
>
> Milian Wolff wrote:
> Oh sorry! I really meant to work on it...
>
> The issue is basically that the project model was accessed from a background \
> thread, through findIncludePaths() in cpputils.cpp which is e.g. eventually used \
> via CodeCompletionContext::doIncludeCompletion (well, just revert the revert and \
> try to do include-path auto completion and get a backtrace, you'll see where it \
> asserts). One potential fix for that would be to wrap the code in the \
> includepathcomputer with the foregroundlock, you could try that out... Not sure \
> there is a better alternative I'm afraid :(
> Alexandre Courbot wrote:
> Sorry, I still fail to understand. The project controller and project model are \
> only accessed read-only by the background thread and are only used to read a \
> configuration option - how can this mess with include-path auto completion and \
> cause a crash?
> I can see the following assert in IncludePathComputer:
>
> Q_ASSERT(QThread::currentThread() == QCoreApplication::instance()->thread());
>
> I understand the issue of having that run in a background thread, but unless I \
> missed something this is not what the current patch is doing - or is it?
> Milian Wolff wrote:
> even a read-only access from another thread to thread-unsafe data (like the project \
> model) is a race condition an hence could lead to crashes and must be serialized \
> using e.g. the foreground lock.
> also look at the diff of ff1744bcb8b125f6bcb0227925973a59d176de39, it slightly \
> diverges from the diff in this review request.
> oh and: yes that assert is the reason it got reverted, it showed that this could be \
> accessed from the background thread and then it would lead to the race above.
That is actually not 100% correct. It depends on how the function which you access \
from the background thread is written and what type of data is being returned and \
what is then done with that. For example acessing the profject configuration and \
returning a pod type from that access should be a pretty safe thing to do if you can \
be sure the background thread is not running while the project is being deleted. Then \
the function itself would always be able to get the config object and the rest of the \
work doesn't use any data from outside the function.
- Andreas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/102883/#review13796
-----------------------------------------------------------
On Oct. 16, 2011, 8:31 a.m., Alexandre Courbot wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102883/
> -----------------------------------------------------------
>
> (Updated Oct. 16, 2011, 8:31 a.m.)
>
>
> Review request for KDevelop and Milian Wolff.
>
>
> Description
> -------
>
> Include resolving using 'make' optional
>
> Make can be invoked by the background parser if some include paths are
> unresolved. However some projects do not use make for building and this
> potentially induces an overhead for large projects. This patch adds a
> project-wide option to disable this behavior.
>
>
> Diffs
> -----
>
> languages/cpp/includepathresolver.cpp d0b1cc1ede30409f77123573b269ea866a71ff96
> projectbuilders/makebuilder/makebuilderconfig.kcfg \
> 8b521f12a92ce1bddef51c6a4a4126d8c9d1893c projectbuilders/makebuilder/makeconfig.ui \
> 6c047c1bda5ec8ffc9af98546a33d083e065185b
> Diff: http://git.reviewboard.kde.org/r/102883/diff/
>
>
> Testing
> -------
>
> Checked that the setting is correctly set and taken into account by the \
> IncludePathResolver.
>
> Thanks,
>
> Alexandre Courbot
>
>
[Attachment #5 (text/html)]
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 \
solid;"> <tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/102883/">http://git.reviewboard.kde.org/r/102883/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On May 14th, 2012, 1:15 a.m., <b>Alexandre \
Courbot</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;">I noticed that this patch has been reverted a while ago:
commit 17fa62314a30071296533e8e1cd53d9fca730f18
Author: Milian Wolff <mail@milianw.de>
Date: Tue Mar 20 17:02:20 2012 +0100
Revert "make include-path resolution using make optional on a per-project \
basis"
include-path auto completion creates a includepath computer in the background
thread which would break this patch (and assert, actually).
shows that we desparately need a unit test for this. I'll revive the patch
once I have time to fix it properly.
This reverts commit ff1744bcb8b125f6bcb0227925973a59d176de39.
Milian, could you elaborate on what needs to be fixed for this patch to make it \
again? I'd like to give it a try. Thanks.</pre> </blockquote>
<p>On May 16th, 2012, 10:23 a.m., <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;">Oh sorry! I really meant \
to work on it...
The issue is basically that the project model was accessed from a background thread, \
through findIncludePaths() in cpputils.cpp which is e.g. eventually used via \
CodeCompletionContext::doIncludeCompletion (well, just revert the revert and try to \
do include-path auto completion and get a backtrace, you'll see where it \
asserts). One potential fix for that would be to wrap the code in the \
includepathcomputer with the foregroundlock, you could try that out... Not sure there \
is a better alternative I'm afraid :(</pre> </blockquote>
<p>On May 16th, 2012, 2:55 p.m., <b>Alexandre Courbot</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;">Sorry, I still fail to \
understand. The project controller and project model are only accessed read-only by \
the background thread and are only used to read a configuration option - how can this \
mess with include-path auto completion and cause a crash?
I can see the following assert in IncludePathComputer:
Q_ASSERT(QThread::currentThread() == QCoreApplication::instance()->thread());
I understand the issue of having that run in a background thread, but unless I missed \
something this is not what the current patch is doing - or is it?</pre> \
</blockquote>
<p>On May 16th, 2012, 3:08 p.m., <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;">even a read-only access \
from another thread to thread-unsafe data (like the project model) is a race \
condition an hence could lead to crashes and must be serialized using e.g. the \
foreground lock.
also look at the diff of ff1744bcb8b125f6bcb0227925973a59d176de39, it slightly \
diverges from the diff in this review request.
oh and: yes that assert is the reason it got reverted, it showed that this could be \
accessed from the background thread and then it would lead to the race above.</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;">That is actually not \
100% correct. It depends on how the function which you access from the background \
thread is written and what type of data is being returned and what is then done with \
that. For example acessing the profject configuration and returning a pod type from \
that access should be a pretty safe thing to do if you can be sure the background \
thread is not running while the project is being deleted. Then the function itself \
would always be able to get the config object and the rest of the work doesn't \
use any data from outside the function.</pre> <br />
<p>- Andreas</p>
<br />
<p>On October 16th, 2011, 8:31 a.m., Alexandre Courbot wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" \
style="background-image: \
url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); \
background-position: left top; background-repeat: repeat-x; border: 1px black \
solid;"> <tr>
<td>
<div>Review request for KDevelop and Milian Wolff.</div>
<div>By Alexandre Courbot.</div>
<p style="color: grey;"><i>Updated Oct. 16, 2011, 8:31 a.m.</i></p>
<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;">Include resolving using 'make' optional
Make can be invoked by the background parser if some include paths are
unresolved. However some projects do not use make for building and this
potentially induces an overhead for large projects. This patch adds a
project-wide option to disable this behavior.</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;">Checked that the setting is correctly set and taken into account by the \
IncludePathResolver.</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/cpp/includepathresolver.cpp <span style="color: \
grey">(d0b1cc1ede30409f77123573b269ea866a71ff96)</span></li>
<li>projectbuilders/makebuilder/makebuilderconfig.kcfg <span style="color: \
grey">(8b521f12a92ce1bddef51c6a4a4126d8c9d1893c)</span></li>
<li>projectbuilders/makebuilder/makeconfig.ui <span style="color: \
grey">(6c047c1bda5ec8ffc9af98546a33d083e065185b)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/102883/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
--
KDevelop-devel mailing list
KDevelop-devel@kdevelop.org
https://barney.cs.uni-potsdam.de/mailman/listinfo/kdevelop-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic