Unused struct field bucket_mask (was: Re: [PATCH 04/11] Port: removed useless msg->cancelled == 0 checks.)

[Alejandro Colomar]

Hi Andrew,

On 6/18/22 00:03, Andrew Clayton wrote:
> Aye. So getting values other than 1 or 0 likely indicates an issue
> (even though it's backed by an unsigned int).

Unless we're doing type punning on purpose.  Type punning from a pointer 
to a bool might be useful...

> 
> [...]
> 
>> $ grepc nxt_lvlhsh_entry_value
>> ./src/nxt_lvlhsh.c:103:
>> #define nxt_lvlhsh_entry_value(e)
>>         \
>>       (void *) (((uintptr_t) (e)[1] << 32) + (e)[0])
>>
>>
>> ./src/nxt_lvlhsh.c:125:
>> #define nxt_lvlhsh_entry_value(e)
>>         \
>>       (void *) (e)[0]
>>
>>
>> (Above, the first definition is for 64 bit archs, the other is for else.)
> 
> So they just convert a (uint32_t *) to a (void *) ? Although isn't the
> shift superfluous? AFAICT

So, bucket, which was set from some masking stuff, seems to be an 
`uint32_t[]`?  Then, this code merges them into a (64-bit) `uintptr_t` 
(I guess the shift avoids endianness problems here) (it also makes sense 
that in 32-bit, the array has a size of 1, and therefore it has no 
endianness problems with the type punning.

This kind of makes sense.

So we got this pointer, created from magic, that is assigned into 
lhq->value, and it seems to have a field that is supposed to have a 
meaning as a boolean.  It could make sense.

But, I still wonder what kind of punning is going on here, since I can 
find nothing about the initialization; it may be using a completely 
different type to initialize, then reinterpret as `nxt_port_recv_msg_t`...

> 
> #define nxt_lvlhsh_entry_value(e) \
>          (void *)((uintptr_t)(e)[0])
> 
> does the same thing. Of course I may be totally misunderstanding this.
> Happy to be corrected either way ;).
> 
>> $ grepc nxt_lvlhsh_bucket
>> ./src/nxt_lvlhsh.c:78:
>> #define nxt_lvlhsh_bucket(proto, bkt)
>>         \
>>       (uint32_t *) ((uintptr_t) bkt & ~(uintptr_t) proto->bucket_mask)

This could be moved to inline functions, to improve readability (by 
removing some casts), and safety (by removing some casts, and limiting 
the effects of the remaining).


>>
>>
>> $ grepc nxt_lvlhsh_bucket_entries
>> ./src/nxt_lvlhsh.c:82:
>> #define nxt_lvlhsh_bucket_entries(proto, bkt)
>>         \
>>       (((uintptr_t) bkt & (uintptr_t) proto->bucket_mask) >> 1)

bucket_mask is just an uint32_t mask (I don't understand what it means, 
but it looks good).  And bkt comes from a `void **`, which, again, is 
opaque enough to stop my attempts to follow it.

And there are many call sites, so it would be painful to follow them all 
(but if you feel like doing that, it could be interesting).

>>
>>
>> So my impression is that it's being used, although through the darkest
>> of magics (uintptr_t), and I can't really follow it more to find the
>> rabbit; maybe you do.
>> I hope this helps.
> 
> I admit I don't immediately see the connection with the hashtable stuff
> and ->cancelled...
> 
> What I'd really like to do is to test and trace the code in
> src/nxt_port_socket.c::nxt_port_read_msg_process() that's enabled when
> msg->port_msg.nf != 0
> 
> 1194     if (nxt_slow_path(msg->port_msg.nf != 0)) {
> 
> I just don't know how to trigger that code, i.e what unit config would
> make use of this?

Heh, good look triggering that! :p

I guess you could add an abort there, and run the whole pytest (maybe 
even with some modules, if the base doesn't trigger it), to see if some 
of that triggers it?

> 
>> I wonder how the world did this without grepc(1) :p
> 
> I have ctags set up for unit, useful.

I read about it, but didn't feel easy to use.  I just wanted to grep for 
stuff, without having to set up anything.  Also, it works integrated 
with the editor, which is something I try to avoid (I don't like IDEs, 
and wouldn't want to transform vim(1) into one).  The UNIX way (one-shot 
run, dump stuff into stdout) seems better to my taste.

Cheers,

Alex

-- 
Alejandro Colomar
<http://www.alejandro-colomar.es/>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://mailman.nginx.org/pipermail/unit/attachments/20220618/f73cc0e2/attachment.bin>