Re: [OE-core] [PATCH] qemu: always define unknown_lock_type

["Joe Slater" <joe.slater@windriver.com>]

[-- Attachment #1: Type: text/plain, Size: 2077 bytes --]

Your patch makes the error always occur at run time.  We can preserve the intent of producing an error at link time with the attached patch which was NOT accepted upstream.  I think they do not like the name QLNULL.  In any event, it does work for all optimizations and applies to the latest master branch of qemu.

Joe

-----Original Message-----
From: openembedded-core@lists.openembedded.org <openembedded-core@lists.openembedded.org> On Behalf Of Khem Raj
Sent: Monday, September 14, 2020 8:23 AM
To: Yu, Mingli <Mingli.Yu@windriver.com>
Cc: Ross Burton <ross@burtonini.com>; OE-core <openembedded-core@lists.openembedded.org>
Subject: Re: [OE-core] [PATCH] qemu: always define unknown_lock_type

On Sun, Sep 13, 2020 at 11:29 PM Yu, Mingli <mingli.yu@windriver.com> wrote:
>
>
>
> On 9/14/20 2:02 PM, Khem Raj wrote:
> >
> >
> > On 9/13/20 10:44 PM, Yu, Mingli wrote:
> >>
> >>
> >> On 9/14/20 1:26 PM, Khem Raj wrote:
> >>>
> >>>
> >>> On 9/13/20 6:50 PM, Yu, Mingli wrote:
> >>>>
> >>>>
> >>>> On 9/10/20 6:41 PM, Ross Burton wrote:
> >>>>> On Thu, 10 Sep 2020 at 04:03, Yu, Mingli 
> >>>>> <mingli.yu@windriver.com>
> >>>>> wrote:
> >>>>>> +Upstream-Status: Submitted [qemu-devel mailing list]
> >>>>>
> >>>>> https://lists.nongnu.org/archive/cgi-bin/namazu.cgi?query=unknow
> >>>>> n_lock_type&submit=Search%21&idxname=qemu-devel&max=20&result=no
> >>>>> rmal&sort=score
> >>>>>
> >>>>>
> >>>>> doesn't find this patch, can you link to it please?
> >>>>>
> >>>>
> >>>> Hi Ross,
> >>>>
> >>>> The link is
> >>>> https://lists.nongnu.org/archive/html/qemu-devel/2020-09/msg03873
> >>>> .html,
> >>>> will include the link in V2.
> >>>
> >>> are you compiling without __OPTIMIZE__ defined ? qemu may not work
> >>
> >> Hi Khem,
> >>
> >> I didn't especially customize something for __OPTIMIZE__,  could 
> >> you help guide where to define it?
> >
> > perhaps you are not using one of -O<n> option ?
>
> -Og passed to the compiler as DEBUG_BUILD = "1" defined in local.conf.

Does qemu work when built with -Og

[-- Attachment #2: 0001-lockable-use-QLNULL-for-a-null-lockable.patch --]
[-- Type: application/octet-stream, Size: 7122 bytes --]

From b85299fc301440b661bfc4805f366073fc1dda62 Mon Sep 17 00:00:00 2001
From: Joe Slater <joe.slater@windriver.com>
Date: Wed, 3 Jun 2020 11:37:03 -0700
Subject: [oe-core][PATCH 1/1] lockable:  use QLNULL for a null lockable

Allows us to build with -Og and optimizations that
do not clean up dead-code.

If we use QLNULL for null lockables, we can always
use referencing unknown_lock_type as a link time
error indicator.

Signed-off-by: Joe Slater <joe.slater@windriver.com>
---
 block/block-backend.c          |  4 ++--
 block/block-copy.c             |  2 +-
 block/mirror.c                 |  4 ++--
 fsdev/qemu-fsdev-throttle.c    |  6 +++---
 hw/9pfs/9p.c                   |  2 +-
 include/qemu/lockable.h        | 20 ++++++++------------
 util/qemu-co-shared-resource.c |  2 +-
 7 files changed, 18 insertions(+), 22 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 24dd0670d1..dee953a0ff 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1176,7 +1176,7 @@ static void coroutine_fn blk_wait_while_drained(BlockBackend *blk)
 
     if (blk->quiesce_counter && !blk->disable_request_queuing) {
         blk_dec_in_flight(blk);
-        qemu_co_queue_wait(&blk->queued_requests, NULL);
+        qemu_co_queue_wait(&blk->queued_requests, QLNULL);
         blk_inc_in_flight(blk);
     }
 }
@@ -2380,7 +2380,7 @@ static void blk_root_drained_end(BdrvChild *child, int *drained_end_counter)
         if (blk->dev_ops && blk->dev_ops->drained_end) {
             blk->dev_ops->drained_end(blk->dev_opaque);
         }
-        while (qemu_co_enter_next(&blk->queued_requests, NULL)) {
+        while (qemu_co_enter_next(&blk->queued_requests, QLNULL)) {
             /* Resume all queued requests */
         }
     }
diff --git a/block/block-copy.c b/block/block-copy.c
index cd9bc47c8f..36e6ec5809 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -120,7 +120,7 @@ static bool coroutine_fn block_copy_wait_one(BlockCopyState *s, int64_t offset,
         return false;
     }
 
-    qemu_co_queue_wait(&task->wait_queue, NULL);
+    qemu_co_queue_wait(&task->wait_queue, QLNULL);
 
     return true;
 }
diff --git a/block/mirror.c b/block/mirror.c
index 26acf4af6f..0c7ca7a221 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -158,7 +158,7 @@ static void coroutine_fn mirror_wait_on_conflicts(MirrorOp *self,
             if (ranges_overlap(self_start_chunk, self_nb_chunks,
                                op_start_chunk, op_nb_chunks))
             {
-                qemu_co_queue_wait(&op->waiting_requests, NULL);
+                qemu_co_queue_wait(&op->waiting_requests, QLNULL);
                 break;
             }
         }
@@ -298,7 +298,7 @@ mirror_wait_for_any_operation(MirrorBlockJob *s, bool active)
         if (!op->is_pseudo_op && op->is_in_flight &&
             op->is_active_write == active)
         {
-            qemu_co_queue_wait(&op->waiting_requests, NULL);
+            qemu_co_queue_wait(&op->waiting_requests, QLNULL);
             return;
         }
     }
diff --git a/fsdev/qemu-fsdev-throttle.c b/fsdev/qemu-fsdev-throttle.c
index 5c83a1cc09..78d256dc31 100644
--- a/fsdev/qemu-fsdev-throttle.c
+++ b/fsdev/qemu-fsdev-throttle.c
@@ -22,13 +22,13 @@
 static void fsdev_throttle_read_timer_cb(void *opaque)
 {
     FsThrottle *fst = opaque;
-    qemu_co_enter_next(&fst->throttled_reqs[false], NULL);
+    qemu_co_enter_next(&fst->throttled_reqs[false], QLNULL);
 }
 
 static void fsdev_throttle_write_timer_cb(void *opaque)
 {
     FsThrottle *fst = opaque;
-    qemu_co_enter_next(&fst->throttled_reqs[true], NULL);
+    qemu_co_enter_next(&fst->throttled_reqs[true], QLNULL);
 }
 
 int fsdev_throttle_parse_opts(QemuOpts *opts, FsThrottle *fst, Error **errp)
@@ -100,7 +100,7 @@ void coroutine_fn fsdev_co_throttle_request(FsThrottle *fst, bool is_write,
     if (throttle_enabled(&fst->cfg)) {
         if (throttle_schedule_timer(&fst->ts, &fst->tt, is_write) ||
             !qemu_co_queue_empty(&fst->throttled_reqs[is_write])) {
-            qemu_co_queue_wait(&fst->throttled_reqs[is_write], NULL);
+            qemu_co_queue_wait(&fst->throttled_reqs[is_write], QLNULL);
         }
 
         throttle_account(&fst->ts, is_write, iov_size(iov, iovcnt));
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 7bb994bbf2..c9a34ab399 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -2891,7 +2891,7 @@ static void coroutine_fn v9fs_flush(void *opaque)
         /*
          * Wait for pdu to complete.
          */
-        qemu_co_queue_wait(&cancel_pdu->complete, NULL);
+        qemu_co_queue_wait(&cancel_pdu->complete, QLNULL);
         if (!qemu_co_queue_next(&cancel_pdu->complete)) {
             cancel_pdu->cancelled = 0;
             pdu_free(cancel_pdu);
diff --git a/include/qemu/lockable.h b/include/qemu/lockable.h
index b620023141..b928de01a9 100644
--- a/include/qemu/lockable.h
+++ b/include/qemu/lockable.h
@@ -24,18 +24,14 @@ struct QemuLockable {
     QemuLockUnlockFunc *unlock;
 };
 
-/* This function gives an error if an invalid, non-NULL pointer type is passed
- * to QEMU_MAKE_LOCKABLE.  For optimized builds, we can rely on dead-code elimination
- * from the compiler, and give the errors already at link time.
+/*
+ *  If unknown_lock_type() is referenced, it means we have tried to passed something
+ *  not recognized as lockable to the macros below.  Use QLNULL to intentionally pass
+ *  a null lockable.  Using NULL will cause (unused) references to unknown_lock_type()
+ *  which may or may not be eliminated by optimization.
  */
-#if defined(__OPTIMIZE__) && !defined(__SANITIZE_ADDRESS__)
+#define QLNULL ((QemuLockable *)NULL)
 void unknown_lock_type(void *);
-#else
-static inline void unknown_lock_type(void *unused)
-{
-    abort();
-}
-#endif
 
 static inline __attribute__((__always_inline__)) QemuLockable *
 qemu_make_lockable(void *x, QemuLockable *lockable)
@@ -46,7 +42,7 @@ qemu_make_lockable(void *x, QemuLockable *lockable)
     return x ? lockable : NULL;
 }
 
-/* Auxiliary macros to simplify QEMU_MAKE_LOCABLE.  */
+/* Auxiliary macros to simplify QEMU_MAKE_LOCKABLE.  */
 #define QEMU_LOCK_FUNC(x) ((QemuLockUnlockFunc *)    \
     QEMU_GENERIC(x,                                  \
                  (QemuMutex *, qemu_mutex_lock),     \
@@ -79,7 +75,7 @@ qemu_make_lockable(void *x, QemuLockable *lockable)
  *
  * Returns a QemuLockable object that can be passed around
  * to a function that can operate with locks of any kind, or
- * NULL if @x is %NULL.
+ * NULL if @x is %QLNULL.
  */
 #define QEMU_MAKE_LOCKABLE(x)                        \
     QEMU_GENERIC(x,                                  \
diff --git a/util/qemu-co-shared-resource.c b/util/qemu-co-shared-resource.c
index 1c83cd9d29..7423ea45c2 100644
--- a/util/qemu-co-shared-resource.c
+++ b/util/qemu-co-shared-resource.c
@@ -64,7 +64,7 @@ void coroutine_fn co_get_from_shres(SharedResource *s, uint64_t n)
 {
     assert(n <= s->total);
     while (!co_try_get_from_shres(s, n)) {
-        qemu_co_queue_wait(&s->queue, NULL);
+        qemu_co_queue_wait(&s->queue, QLNULL);
     }
 }
 
-- 
2.17.1