Re: [Qemu-devel] [RFC PATCH 0/5] Scoped locks using attribute((cleanup))

2017-12-15 Thread Richard Henderson 
On 12/12/2017 02:41 PM, Eric Blake wrote:
> On 12/11/2017 03:32 PM, Paolo Bonzini wrote:
>> On 11/12/2017 15:11, Eric Blake wrote:
>>> I don't know if there is a way to make gcc insert stack-unwind
>>> directives that are honored across longjmp (I know C++ does it for
>>> exceptions; so there may be a way, and I just don't know it).
>>
>> Probably -fexceptions.
>>
> 
> Well, that's what 'info gcc' mentions:
> 
> 'cleanup (CLEANUP_FUNCTION)'
>  The 'cleanup' attribute runs a function when the variable goes out
>  of scope.  This attribute can only be applied to auto function
>  scope variables; it may not be applied to parameters or variables
>  with static storage duration.  The function must take one
>  parameter, a pointer to a type compatible with the variable.  The
>  return value of the function (if any) is ignored.
> 
>  If '-fexceptions' is enabled, then CLEANUP_FUNCTION is run during
>  the stack unwinding that happens during the processing of the
>  exception.  Note that the 'cleanup' attribute does not allow the
>  exception to be caught, only to perform an action.  It is undefined
>  what happens if CLEANUP_FUNCTION does not return normally.
> 
> but adding -fexceptions to my sample program does NOT make a difference
> (apparently, unwind cleanup triggered by C++ exceptions is NOT the same
> as unwinding done by longjmp()).

longjmp isn't an exception, and so doesn't run stack cleanups.  You'd need to
use _Unwind_Throw ... except for the fact that C doesn't have a mechanism by
which we can catch it.

We would really need to use C++ and throw to get those semantics.


r~

Re: [Qemu-devel] [RFC PATCH 0/5] Scoped locks using attribute((cleanup))

2017-12-12 Thread Eric Blake 
On 12/11/2017 03:32 PM, Paolo Bonzini wrote:
> On 11/12/2017 15:11, Eric Blake wrote:
>> I don't know if there is a way to make gcc insert stack-unwind
>> directives that are honored across longjmp (I know C++ does it for
>> exceptions; so there may be a way, and I just don't know it).
> 
> Probably -fexceptions.
> 

Well, that's what 'info gcc' mentions:

'cleanup (CLEANUP_FUNCTION)'
 The 'cleanup' attribute runs a function when the variable goes out
 of scope.  This attribute can only be applied to auto function
 scope variables; it may not be applied to parameters or variables
 with static storage duration.  The function must take one
 parameter, a pointer to a type compatible with the variable.  The
 return value of the function (if any) is ignored.

 If '-fexceptions' is enabled, then CLEANUP_FUNCTION is run during
 the stack unwinding that happens during the processing of the
 exception.  Note that the 'cleanup' attribute does not allow the
 exception to be caught, only to perform an action.  It is undefined
 what happens if CLEANUP_FUNCTION does not return normally.

but adding -fexceptions to my sample program does NOT make a difference
(apparently, unwind cleanup triggered by C++ exceptions is NOT the same
as unwinding done by longjmp()).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [RFC PATCH 0/5] Scoped locks using attribute((cleanup))

2017-12-11 Thread Emilio G. Cota 
On Fri, Dec 08, 2017 at 11:55:48 +0100, Paolo Bonzini wrote:
> So I'm a bit underwhelmed by this experiment.  Other opinions?

I am on the same boat. Most use cases in this patchset are arguably
adding more complexity because they substitute already very simple
code (e.g. "lock; do_something; unlock"), as others have pointed out
as well.

I usually deal with tricky cases (i.e. functions with many return
paths) with an inline "__locked" function. In most cases this will
be clearer than using the macros. I concede though that the separate
inline is not always an option.

That said, two comments:

- We might be better off just exposing the cleanup attribute
  via some convenience macros. The systemd codebase does this,
  mostly for freeing memory or closing file descriptors. I suspect
  a large percentage of goto's in our codebase could be eliminated.

  This could be also used for locks, although we'd need a variant
  of mutex_lock that returned the mutex, so that in the cleanup
  function we could just check for NULL.

- Does the cleanup attribute work on all compilers used to build QEMU?
  (I'm thinking of Windows in particular.)

Thanks,

Emilio

Re: [Qemu-devel] [RFC PATCH 0/5] Scoped locks using attribute((cleanup))

2017-12-11 Thread Paolo Bonzini 
On 11/12/2017 15:11, Eric Blake wrote:
> I don't know if there is a way to make gcc insert stack-unwind
> directives that are honored across longjmp (I know C++ does it for
> exceptions; so there may be a way, and I just don't know it).

Probably -fexceptions.

Paolo

> Conversely, I do know that pthread_cleanup_push/pop, which does
> something similar, is permitted by POSIX to NOT work across longjmp:
> 
>Calling longjmp(3) (siglongjmp(3)) produces undefined  results
> if  any
>call  has  been made to pthread_cleanup_push() or
> pthread_cleanup_pop()
>without the matching call of the pair since the jump buffer was
> filled
>by   setjmp(3)  (sigsetjmp(3)).   Likewise,  calling  longjmp(3)
> (sig‐
>longjmp(3)) from inside a clean-up handler produces  undefined
> results
>unless  the  jump  buffer  was  also filled by setjmp(3)
> (sigsetjmp(3))
>inside the handler.




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [RFC PATCH 0/5] Scoped locks using attribute((cleanup))

2017-12-11 Thread Eric Blake 
On 12/11/2017 03:38 AM, Peter Maydell wrote:
> On 8 December 2017 at 19:40, Eric Blake  wrote:
>> On 12/08/2017 04:55 AM, Paolo Bonzini wrote:
>>> Likewise,
>>>
>>> QEMU_WITH_LOCK(QemuMutex, guard_name, &some_mutex) {
>>> ...
>>> }
>>>
>>> is the same as
>>>
>>> qemu_mutex_lock(&some_mutex);
>>> ...
>>> qemu_mutex_unlock(&some_mutex);
>>>
>>> except that any returns within the region will unlock the mutex.
>>
>> Not just returns, but ANY manner that you leave the scope, including a
>> goto or just falling out of the end of the scope.
> 
> How about longjmp()ing out of it?

Easy to test:

==
#include 
#include 
#include 

void my_cleanup (int *ptr) {
  int *i = ptr;
  printf("in my_cleanup(%d)\n", *i);
}

jmp_buf jmp;

void foo(int i) {
  while (1) {
__attribute__((cleanup(my_cleanup))) int j = i;
if (i == 0) {
  printf("before leaving scope by return\n");
  return;
}
if (i == 1) {
  goto label;
}
if (i == 3) {
  longjmp(jmp, 1);
}
if (i == 4) {
  printf("before leaving scope by exit\n");
  exit(0);
}
break;
  }
  printf("after leaving scope by break\n");
  return;
 label:
  printf("after leaving scope by return\n");
}

int main(void) {
  foo(0);
  foo(1);
  foo(2);
  if (!setjmp(jmp)) {
foo(3);
  }
  printf("after leaving scope by longjmp\n");
  foo(4);
}


But the results aren't necessarily nice, depending on how we currently
(ab)use longjmp. Under Fedora 27
gcc (GCC) 7.2.1 20170915 (Red Hat 7.2.1-2)

I get

$ ./foo
before leaving scope by return
in my_cleanup(0)
in my_cleanup(1)
after leaving scope by return
in my_cleanup(2)
after leaving scope by break
after leaving scope by longjmp
before leaving scope by exit

I don't know if there is a way to make gcc insert stack-unwind
directives that are honored across longjmp (I know C++ does it for
exceptions; so there may be a way, and I just don't know it).
Conversely, I do know that pthread_cleanup_push/pop, which does
something similar, is permitted by POSIX to NOT work across longjmp:

   Calling longjmp(3) (siglongjmp(3)) produces undefined  results
if  any
   call  has  been made to pthread_cleanup_push() or
pthread_cleanup_pop()
   without the matching call of the pair since the jump buffer was
filled
   by   setjmp(3)  (sigsetjmp(3)).   Likewise,  calling  longjmp(3)
(sig‐
   longjmp(3)) from inside a clean-up handler produces  undefined
results
   unless  the  jump  buffer  was  also filled by setjmp(3)
(sigsetjmp(3))
   inside the handler.


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [RFC PATCH 0/5] Scoped locks using attribute((cleanup))

2017-12-11 Thread Peter Maydell 
On 8 December 2017 at 19:40, Eric Blake  wrote:
> On 12/08/2017 04:55 AM, Paolo Bonzini wrote:
>> Likewise,
>>
>> QEMU_WITH_LOCK(QemuMutex, guard_name, &some_mutex) {
>> ...
>> }
>>
>> is the same as
>>
>> qemu_mutex_lock(&some_mutex);
>> ...
>> qemu_mutex_unlock(&some_mutex);
>>
>> except that any returns within the region will unlock the mutex.
>
> Not just returns, but ANY manner that you leave the scope, including a
> goto or just falling out of the end of the scope.

How about longjmp()ing out of it?

thanks
-- PMM

Re: [Qemu-devel] [RFC PATCH 0/5] Scoped locks using attribute((cleanup))

2017-12-10 Thread no-reply 
Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 20171208105553.12249-1-pbonz...@redhat.com
Subject: [Qemu-devel] [RFC PATCH 0/5] Scoped locks using attribute((cleanup))
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
8e9ffc046d thread-pool: convert to use lock guards
007b5bc7b3 qht: convert to use lock guards
6fd333cacc qemu-timer: convert to use lock guards
1a895608bb lock-guard: add scoped lock implementation
2188a83be2 compiler: add a helper for C99 inline functions

=== OUTPUT BEGIN ===
Checking PATCH 1/5: compiler: add a helper for C99 inline functions...
Checking PATCH 2/5: lock-guard: add scoped lock implementation...
Checking PATCH 3/5: qemu-timer: convert to use lock guards...
WARNING: line over 80 characters
#54: FILE: util/qemu-timer.c:410:
+QEMU_LOCK_GUARD(QemuMutex, timers_guard, 
&timer_list->active_timers_lock);

ERROR: space required before the open parenthesis '('
#115: FILE: util/qemu-timer.c:519:
+for(;;) {

total: 1 errors, 1 warnings, 129 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 4/5: qht: convert to use lock guards...
Checking PATCH 5/5: thread-pool: convert to use lock guards...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-de...@freelists.org

Re: [Qemu-devel] [RFC PATCH 0/5] Scoped locks using attribute((cleanup))

2017-12-10 Thread no-reply 
Hi,

This series failed automatic build test. Please find the testing commands and
their output below. If you have docker installed, you can probably reproduce it
locally.

Message-id: 20171208105553.12249-1-pbonz...@redhat.com
Subject: [Qemu-devel] [RFC PATCH 0/5] Scoped locks using attribute((cleanup))
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
set -e
git submodule update --init dtc
# Let docker tests dump environment info
export SHOW_ENV=1
export J=8
time make docker-test-quick@centos6
time make docker-test-build@min-glib
time make docker-test-mingw@fedora
# iotests is broken now, skip
# time make docker-test-block@fedora
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]   patchew/20171208105553.12249-1-pbonz...@redhat.com 
-> patchew/20171208105553.12249-1-pbonz...@redhat.com
Switched to a new branch 'test'
8e9ffc046d thread-pool: convert to use lock guards
007b5bc7b3 qht: convert to use lock guards
6fd333cacc qemu-timer: convert to use lock guards
1a895608bb lock-guard: add scoped lock implementation
2188a83be2 compiler: add a helper for C99 inline functions

=== OUTPUT BEGIN ===
Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc'
Cloning into '/var/tmp/patchew-tester-tmp-k2f3p02l/src/dtc'...
Submodule path 'dtc': checked out '558cd81bdd432769b59bff01240c44f82cfb1a9d'
  BUILD   centos6
make[1]: Entering directory '/var/tmp/patchew-tester-tmp-k2f3p02l/src'
  GEN 
/var/tmp/patchew-tester-tmp-k2f3p02l/src/docker-src.2017-12-11-01.38.48.5601/qemu.tar
Cloning into 
'/var/tmp/patchew-tester-tmp-k2f3p02l/src/docker-src.2017-12-11-01.38.48.5601/qemu.tar.vroot'...
done.
Checking out files:  46% (2622/5663)   
Checking out files:  47% (2662/5663)   
Checking out files:  48% (2719/5663)   
Checking out files:  49% (2775/5663)   
Checking out files:  50% (2832/5663)   
Checking out files:  51% (2889/5663)   
Checking out files:  52% (2945/5663)   
Checking out files:  53% (3002/5663)   
Checking out files:  54% (3059/5663)   
Checking out files:  55% (3115/5663)   
Checking out files:  56% (3172/5663)   
Checking out files:  57% (3228/5663)   
Checking out files:  58% (3285/5663)   
Checking out files:  59% (3342/5663)   
Checking out files:  60% (3398/5663)   
Checking out files:  61% (3455/5663)   
Checking out files:  62% (3512/5663)   
Checking out files:  63% (3568/5663)   
Checking out files:  64% (3625/5663)   
Checking out files:  65% (3681/5663)   
Checking out files:  66% (3738/5663)   
Checking out files:  67% (3795/5663)   
Checking out files:  68% (3851/5663)   
Checking out files:  69% (3908/5663)   
Checking out files:  70% (3965/5663)   
Checking out files:  71% (4021/5663)   
Checking out files:  72% (4078/5663)   
Checking out files:  73% (4134/5663)   
Checking out files:  74% (4191/5663)   
Checking out files:  75% (4248/5663)   
Checking out files:  76% (4304/5663)   
Checking out files:  77% (4361/5663)   
Checking out files:  78% (4418/5663)   
Checking out files:  79% (4474/5663)   
Checking out files:  80% (4531/5663)   
Checking out files:  81% (4588/5663)   
Checking out files:  82% (4644/5663)   
Checking out files:  83% (4701/5663)   
Checking out files:  84% (4757/5663)   
Checking out files:  85% (4814/5663)   
Checking out files:  86% (4871/5663)   
Checking out files:  87% (4927/5663)   
Checking out files:  88% (4984/5663)   
Checking out files:  89% (5041/5663)   
Checking out files:  90% (5097/5663)   
Checking out files:  91% (5154/5663)   
Checking out files:  92% (5210/5663)   
Checking out files:  93% (5267/5663)   
Checking out files:  94% (5324/5663)   
Checking out files:  95% (5380/5663)   
Checking out files:  96% (5437/5663)   
Checking out files:  97% (5494/5663)   
Checking out files:  98% (5550/5663)   
Checking out files:  99% (5607/5663)   
Checking out files: 100% (5663/5663)   
Checking out files: 100% (5663/5663), done.
Your branch is up-to-date with 'origin/test'.
Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc'
Cloning into 
'/var/tmp/patchew-tester-tmp-k2f3p02l/src/docker-src.2017-12-11-01.38.48.5601/qemu.tar.vroot/dtc'...
Submodule path 'dtc': checked out '558cd81bdd432769b59bff01240c44f82cfb1a9d'
Submodule 'ui/keycodemapdb' (git://git.qemu.org/keycodemapdb.git) registered 
for path 'ui/keycodemapdb'
Cloning into 
'/var/tmp/patchew-tester-tmp-k2f3p02l/src/docker-src.2017-12-11-01.38.48.5601/qemu.tar.vroot/ui/keycodemapdb'...
Submodule path 'ui/keycodemapdb': checked out 
'10739aa26051a5d49d88132604539d3ed085e72e'
  COPYRUNNER
RUN test-quick in qemu:centos6 
Packages installed:
SDL-devel-1.2.14-7.el6_7.1.x86_64
bison-2.4.1-5.el6.x86_64
bzip2-devel-1.0.5-7.el6_0.x86_64
ccache-3.1.6-2.el6.x86_64
cs

Re: [Qemu-devel] [RFC PATCH 0/5] Scoped locks using attribute((cleanup))

2017-12-08 Thread Eric Blake 
On 12/08/2017 04:55 AM, Paolo Bonzini wrote:
> This is an attempt to make a C API that resembles the C++
> std::unique_lock (mostly untested).  The idea is that you can write
> 
> QEMU_LOCK_GUARD(QemuMutex, guard_name, &some_mutex);
> 
> instead of
> 
> qemu_mutex_lock(&some_mutex);
> ...
> out:
> qemu_mutex_unlock(&some_mutex);
> 
> and the mutex will be unlocked on all exit paths.  In C++ that
> would be "std::unique_lock guard_name(some_mutex);".
> Likewise,
> 
> QEMU_WITH_LOCK(QemuMutex, guard_name, &some_mutex) {
> ...
> }
> 
> is the same as
> 
> qemu_mutex_lock(&some_mutex);
> ...
> qemu_mutex_unlock(&some_mutex);
> 
> except that any returns within the region will unlock the mutex.

Not just returns, but ANY manner that you leave the scope, including a
goto or just falling out of the end of the scope.

(and, as Stefan pointed out, this poisons the use of 'break' when this
is used inside a loop, as it now breaks the scope of the QEMU_WITH_LOCK
instead of the outer loop).

> It's possible to use QemuLockGuard also with a spinlock or a
> CoMutex.  However, it is _not_ possible to return a QemuLockGuard
> since C doesn't have an equivalent of C++'s "move semantics", so
> there are other "constructor" macros such as QEMU_ADOPT_LOCK
> and QEMU_TAKEN_LOCK.
> 
> On the positive side, I checked that the entire abstraction
> is removed by the compiler, the generated code is more or less
> the same.  Also, it would let us for example change block/curl.c
> to use a CoQueue instead of a home-grown list+QemuMutex.
> 
> However, I am still not sure about the readability, and it doesn't play
> well with another common idiom in QEMU, which is to wrap global mutexes
> with function such as
> 
> static void block_job_lock(void)
> {
> qemu_mutex_lock(&block_job_mutex);
> }
> 
> static void block_job_unlock(void)
> {
> qemu_mutex_unlock(&block_job_mutex);
> }
> 
> So I'm a bit underwhelmed by this experiment.  Other opinions?

The semantics you list here:

> QEMU_WITH_LOCK(QemuMutex, guard_name, &some_mutex) {
> ...
> }

are slightly different from the semantics in nbdkit:

{
  ACQUIRE_LOCK_FOR_CURRENT_SCOPE(&some_lock);
  ...
}

In that your version requires the user to supply the scope after your
macro, instead of using your macro within the scope.  But I guess that's
because you added other helpers like QEMU_WITH_ADOPTED_LOCK,
QEMU_RETURN_LOCK, and QEMU_TAKEN_LOCK, where you have multiple
possibilities for which state the lock should be in.

Is it worth playing around with the user providing the scope around your
macros, instead of using your macro to introduce the scope?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [RFC PATCH 0/5] Scoped locks using attribute((cleanup))

2017-12-08 Thread Paolo Bonzini 
This is an attempt to make a C API that resembles the C++
std::unique_lock (mostly untested).  The idea is that you can write

QEMU_LOCK_GUARD(QemuMutex, guard_name, &some_mutex);

instead of

qemu_mutex_lock(&some_mutex);
...
out:
qemu_mutex_unlock(&some_mutex);

and the mutex will be unlocked on all exit paths.  In C++ that
would be "std::unique_lock guard_name(some_mutex);".
Likewise,

QEMU_WITH_LOCK(QemuMutex, guard_name, &some_mutex) {
...
}

is the same as

qemu_mutex_lock(&some_mutex);
...
qemu_mutex_unlock(&some_mutex);

except that any returns within the region will unlock the mutex.
It's possible to use QemuLockGuard also with a spinlock or a
CoMutex.  However, it is _not_ possible to return a QemuLockGuard
since C doesn't have an equivalent of C++'s "move semantics", so
there are other "constructor" macros such as QEMU_ADOPT_LOCK
and QEMU_TAKEN_LOCK.

On the positive side, I checked that the entire abstraction
is removed by the compiler, the generated code is more or less
the same.  Also, it would let us for example change block/curl.c
to use a CoQueue instead of a home-grown list+QemuMutex.

However, I am still not sure about the readability, and it doesn't play
well with another common idiom in QEMU, which is to wrap global mutexes
with function such as

static void block_job_lock(void)
{
qemu_mutex_lock(&block_job_mutex);
}

static void block_job_unlock(void)
{
qemu_mutex_unlock(&block_job_mutex);
}

So I'm a bit underwhelmed by this experiment.  Other opinions?

Paolo

Paolo Bonzini (5):
  compiler: add a helper for C99 inline functions
  lock-guard: add scoped lock implementation
  qemu-timer: convert to use lock guards
  qht: convert to use lock guards
  thread-pool: convert to use lock guards

 include/qemu/compiler.h   | 15 +++
 include/qemu/coroutine.h  |  4 ++
 include/qemu/lock-guard.h | 99 +++
 include/qemu/thread.h |  7 
 util/Makefile.objs|  1 +
 util/qemu-thread.c| 17 
 util/qemu-timer.c | 84 
 util/qht.c| 59 ++--
 util/thread-pool.c| 26 ++---
 9 files changed, 225 insertions(+), 87 deletions(-)
 create mode 100644 include/qemu/lock-guard.h
 create mode 100644 util/qemu-thread.c

-- 
2.14.3