Hello Samuel,
Thank you for your review.
I understand the above comment, and I fully agree that we shouldn't
use two for loops for obvious reasons.
However, the structure presented in my patch is not a regular nested
loop. It uses a C macro idiom where the outer loop does not actually
iterate;
it acts solely as a scoped variable cache with a built-in termination trigger.
If a user calls break; inside the iterator block:
- It escapes the inner for loop.
- Control immediately passes to the outer loop's increment
statement, which executes: _ihash_ht_##val = NULL;
- The outer loop checks its condition (_ihash_ht_##val != NULL),
which evaluates to false.
- The entire macro terminates completely.
Because of this NULL assignment in the increment block, the
double-loop behaves identically to a single loop regarding scope,
break, and continue, while successfully solving the
multiple-evaluation hazard.
To verify this behavior in practice, I've attached a small, non-merge
demonstration patch. If you apply this on top of my original patch,
you will see that whenever you call sync in a shell, you get:
We have 1 of executions.
printed to the Mach console.
If the break didn't work as expected, the exit(1) would trigger and
the translator wouldn't even survive boot!
Let me know what you think, or if anything else needs refinement.
Thanks,
Milos
On Sat, Mar 28, 2026 at 3:44 PM Samuel Thibault <[email protected]> wrote:
>
> Hello,
>
> Milos Nikic, le ven. 27 mars 2026 22:04:22 -0700, a ecrit:
> > In HURD_IHASH_ITERATE and HURD_IHASH_ITERATE_ITEMS, the hash table
> > argument (ht) was evaluated multiple times during loop initialization
> > and condition checking. If a caller passed an expression with side
> > effects (e.g., a function call returning a table pointer), it would
> > be executed repeatedly per loop, leading to performance degradation
> > or unintended behavior.
> >
> > Fix this by wrapping the iterators in an outer `for` loop that
> > evaluates and caches `(ht)` exactly once using `__typeof__`.
>
> Please read the comment above:
>
> We can not use two for-loops, because we want a break statement inside
> the iterator block to terminate the operation.
>
> Samuel
>
> > Token
> > pasting (##) is used to generate a unique cache variable name based
> > on the caller's item identifier, safely allowing these macros to be
> > nested without variable shadowing or naming collisions.
> > ---
> > libihash/ihash.h | 46 +++++++++++++++++++++++++++++-----------------
> > 1 file changed, 29 insertions(+), 17 deletions(-)
> >
> > diff --git a/libihash/ihash.h b/libihash/ihash.h
> > index 80679f14..5f73f3e1 100644
> > --- a/libihash/ihash.h
> > +++ b/libihash/ihash.h
> > @@ -322,17 +322,22 @@ hurd_ihash_value_t hurd_ihash_locp_find (hurd_ihash_t
> > ht,
> > pointer pointed to must not have an influence on the condition
> > result, so the comma operator is used to make sure this
> > subexpression is always true). */
> > -#define HURD_IHASH_ITERATE(ht, val) \
> > - for (hurd_ihash_value_t val,
> > \
> > - *_hurd_ihash_valuep = (ht)->size ? &(ht)->items[0].value : 0;
> > \
> > - (ht)->size \
> > - && (size_t) ((_hurd_ihash_item_t) _hurd_ihash_valuep \
> > - - &(ht)->items[0]) \
> > - < (ht)->size \
> > - && (val = *_hurd_ihash_valuep, 1); \
> > - _hurd_ihash_valuep = (hurd_ihash_value_t *) \
> > - (((_hurd_ihash_item_t) _hurd_ihash_valuep) + 1)) \
> > - if (val != _HURD_IHASH_EMPTY && val != _HURD_IHASH_DELETED)
> > +#define HURD_IHASH_ITERATE(ht, val)
> > \
> > + /* Cache 'ht' to prevent multiple evaluation of functions. */
> > \
> > + for (__typeof__(ht) _ihash_ht_##val = (ht);
> > \
> > + _ihash_ht_##val != NULL;
> > \
> > + _ihash_ht_##val = NULL)
> > \
> > + for (hurd_ihash_value_t val,
> > \
> > + *_ihash_p_##val = _ihash_ht_##val->size
> > \
> > + ? &_ihash_ht_##val->items[0].value : 0;
> > \
> > + _ihash_ht_##val->size
> > \
> > + && (size_t) ((_hurd_ihash_item_t) _ihash_p_##val
> > \
> > + - &_ihash_ht_##val->items[0])
> > \
> > + < _ihash_ht_##val->size
> > \
> > + && (((val) = *_ihash_p_##val), 1);
> > \
> > + _ihash_p_##val = (hurd_ihash_value_t *)
> > \
> > + (((_hurd_ihash_item_t) _ihash_p_##val) + 1))
> > \
> > + if ((val) != _HURD_IHASH_EMPTY && (val) != _HURD_IHASH_DELETED)
> >
> > /* Iterate over all elements in the hash table making both the key and
> > the value available. You use this macro with a block, for example
> > @@ -344,12 +349,19 @@ hurd_ihash_value_t hurd_ihash_locp_find (hurd_ihash_t
> > ht,
> > The block will be run for every element in the hash table HT. The
> > key and value of the current element is available as ITEM->key and
> > ITEM->value. */
> > -#define HURD_IHASH_ITERATE_ITEMS(ht, item) \
> > - for (_hurd_ihash_item_t item = (ht)->size? &(ht)->items[0]: 0; \
> > - (ht)->size && item - &(ht)->items[0] < (ht)->size; \
> > - item++) \
> > - if (item->value != _HURD_IHASH_EMPTY && \
> > - item->value != _HURD_IHASH_DELETED)
> > +#define HURD_IHASH_ITERATE_ITEMS(ht, item)
> > \
> > + /* Cache 'ht' to prevent multiple evaluation of functions. */
> > \
> > + for (__typeof__(ht) _ihash_items_ht_##item = (ht);
> > \
> > + _ihash_items_ht_##item != NULL;
> > \
> > + _ihash_items_ht_##item = NULL)
> > \
> > + for (_hurd_ihash_item_t item = _ihash_items_ht_##item->size
> > \
> > + ? &_ihash_items_ht_##item->items[0] :
> > 0; \
> > + _ihash_items_ht_##item->size
> > \
> > + && (size_t) ((item) - &_ihash_items_ht_##item->items[0])
> > \
> > + < _ihash_items_ht_##item->size;
> > \
> > + (item)++)
> > \
> > + if ((item)->value != _HURD_IHASH_EMPTY &&
> > \
> > + (item)->value != _HURD_IHASH_DELETED)
> >
> > /* Remove the entry with the key KEY from the hash table HT. If such
> > an entry was found and removed, 1 is returned, otherwise 0. */
> > --
> > 2.53.0
> >
> >
>
> --
> Samuel
> <s> T'as pas de portable ?
> <m> J'ai un nokia, dans le bassin d'arcachon
From b7734504414225f447e79ad4d206bc9a473b1cc7 Mon Sep 17 00:00:00 2001
From: Milos Nikic <[email protected]>
Date: Sat, 28 Mar 2026 16:44:43 -0700
Subject: [PATCH] Do not merge, lets see how break behaves.
---
libdiskfs/node-cache.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/libdiskfs/node-cache.c b/libdiskfs/node-cache.c
index 0157d55d..51a9c0bd 100644
--- a/libdiskfs/node-cache.c
+++ b/libdiskfs/node-cache.c
@@ -228,6 +228,24 @@ diskfs_node_iterate (error_t (*fun)(struct node *))
get called. */
refcounts_ref (&node->refcounts, NULL);
}
+
+ /* Small test for behavior of break inside HURD_IHASH_ITERATE */
+ int loop_executions = 0;
+ HURD_IHASH_ITERATE (&nodecache, test_val)
+ {
+ loop_executions++;
+ /* Escapes the inner loop. Will it escape the "outer loop"? */
+ break;
+ }
+
+
+ char buffer[100];
+ snprintf(buffer, sizeof(buffer), "We have %i of executions.\n", loop_executions);
+ mach_print(buffer);
+ /* If break doesn't work this will be more than 1...stop everything */
+ if (loop_executions > 1)
+ exit (1);
+
pthread_rwlock_unlock (&nodecache_lock);
p = node_list;
--
2.53.0
