<br><br><div class="gmail_quote">On Thu, Nov 1, 2012 at 2:04 AM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5">On Wed, Oct 31, 2012 at 4:59 PM, John McCall <<a href="mailto:rjmccall@apple.com">rjmccall@apple.com</a>> wrote:<br>
> On Oct 31, 2012, at 4:38 PM, Delesley Hutchins wrote:<br>
>>> That's useless as user documentation because<br>
>>> it's not actionable — I can't actually modify my code in response to<br>
>>> your complaints.<br>
>>><br>
>>> You should instead allow the lock expression to more directly state<br>
>>> what the caller of this method needs to do in order to acquire the lock.<br>
>><br>
>> The thread-safety analysis is based on published research in race-free<br>
>> type systems, and it works by associating every piece of data with its<br>
>> protecting mutex. There's no universal way to acquire a particular<br>
>> mutex -- how the mutex is acquired and released depends entirely on<br>
>> the interface being provided by the class. You'd be surprised by how<br>
>> much variety I've seen in locking schemes. :-)<br>
><br>
> Sure.<br>
><br>
>> The only way to reference lock() instead of the mutex would be to use<br>
>> some variation of the scheme proposed by David Blakie, but that won't<br>
>> work in all cases.<br>
><br>
> Your analysis recognizes arbitrary expressions in these contexts as long<br>
> as they resolve to something that works like a lock, right? Why not also,<br>
> as an alternative, permit arbitrary expressions that happen to resolve<br>
> to calls to methods with the THREAD_SAFETY_ACQUIRES_LOCK<br>
> attribute? e.g.<br>
> THREAD_SAFETY_REQUIRES_LOCK(lock())<br>
> In other words, I am simply proposing that you accept an alternate way<br>
> of specifying the lock; this should not deeply affect your algorithm.<br>
<br>
</div></div>This approach makes some sense to me.<br>
<br>
It seems that the heart of the problem is that you have a region<br>
controlled by a mutex, and you're using the name of that mutex as a<br>
proxy for the name of the region, but access to the region does not<br>
necessarily imply access to the mutex. I think the right solution is<br>
to find another (accessible) way to name the region, not to provide<br>
access to the mutex.<br>
<br>
If your class only has one such region, it wouldn't seem unreasonable<br>
to me to use the instance of the class as a proxy for your region name<br>
in most cases (which I believe the thread safety annotations already<br>
support). Otherwise (or if that's not semantically appropriate), it<br>
seems that we should provide a mechanism for classes to expose a name<br>
for their lockable regions as part of their public API, and I don't<br>
think those names should be required to be tied to the names of the<br>
mutexes (or via any other mechanism which leaks implementation<br>
details). In principle, we could add another form of named declaration<br>
for that; if we stick to existing language constructs, I think using a<br>
member function as a proxy for the region name is the best option.<br>
<br>
Are there reasonable cases where it's reasonable to put a<br>
LOCKS_REQUIRED annotation on a function which doesn't have access to<br>
the mutex it requires, and also doesn't have access to a mechanism<br>
which acquires it? I could imagine such cases arising in templates:<br>
<br>
class LockedData {<br>
Mutex mu;<br>
public:<br>
template<typename F> void RunWithLockHeld(F f) {<br>
ScopedLock lock(mu);<br>
f(this);<br>
}<br>
int data GUARDED_BY(mu);<br>
};<br>
<br>
struct DoThing {<br>
void operator()(LockedData *p) const LOCKS_REQUIRED(p->mu) {<br>
p->data *= 42;<br>
}<br>
};<br>
<br>
LockedData data;<br>
data.RunWithLockHeld(DoThing());<br>
<br>
... though note that even in this case, we have both *p and p->data,<br>
which could be used as proxies for the required mutex.<br>
<div class="HOEnZb"><div class="h5"><br></div></div></blockquote><div><br>I think that from a user's perspective, I prefer John's approach.<br><br>Instead of stating the exact name of the mutex/region that I need to lock, it states what I need to do: this means that I don't have to figure out how to lock the mutex => the safety annotation becomes documentation.<br>
<br>What would you think of this "revised" example:<br><br>class TaskQueue {<br>
public:<br>
void lock() EXCLUSIVE_LOCK_FUNCTION(mu) {<br>
mu.Lock();<br>
logfile << "Task queue acquired by " << getCurrentThreadID() << "\n";<br>
}<br>
<br>
void unlock() UNLOCK_FUNCTION(mu) {<br>
mu.Unlock();<br>
}<br>
<br>
virtual void addTask(int taskID) EXCLUSIVE_LOCKS_REQUIRED(this->lock()) {<br>
queue.push_back(taskID);<br>
}<br>
<br>
void addTaskSync(int taskID) {<br>
lock();<br>
addTask(taskID);<br>
unlock();<br>
}<br>
<br>
private:<br>
Mutex mu;<br>
std::deque<int> queue GUARDED_BY(mu);<br>
};<br>
<br>
<br>
class CheckedTaskQueue : public TaskQueue {<br>
public:<br>
bool validTask(int id);<br>
<br>
// OK: TaskQueue::lock() is public<br>
virtual void addTask(int taskID) EXCLUSIVE_LOCKS_REQUIRED(this->lock()) {<br>
if (validTask(taskID)) TaskQueue::addTask(taskID);<br>
}<br>
};<br>
<br>
<br>
// OK: TaskQueue::lock() is public<br>
void addTwoTasks(TaskQueue* q, int t1, int t2) EXCLUSIVE_LOCKS_REQUIRED(q-><div id=":1ar">lock()) {<br>
q->addTask(t1);<br>
q->addTask(t2);<br>
}</div><br><br><br>Actually, it seems annoying to me that lock and unlock are exposing the name of the mutex, since it's private, but then it would probably be exposed anyway so...<br><br>However, this made me realize there is a small issue with the safety analysis as it stands: I cannot see how to express the contracts of lock and unlock in the following PIMPL case.<br>
<br>class Queue {<br>public:<br> Queue(); Queue(Queue const&); Queue(Queue&&); Queue& operator=(Queue); ~Queue();<br><br> void lock() ....... ;<br><br> void unlock() ........ ;<br><br> void push(int) EXCLUSIVE_LOCKS_REQUIRED(this->lock()) ;<br>
<br> int pop() EXCLUSIVE_LOCKS_REQUIRED(this->lock()) ;<br><br>private:<br> struct Impl;<br> Impl* _impl;<br>};<br><br>Can I express the locks in terms of _impl ?<br><br><br>-- Matthieu<br></div></div>