[Toybox] [PATCH] devmem: Fix 8 byte wide writes

[Rob Landley]

On 8/31/23 07:26, Michael Shavit wrote:
> On Thu, Aug 31, 2023 at 6:57 PM Rob Landley <rob at landley.net> wrote:
>> Could you send me the test case you hit that motivated this change? (I haven't
>> got a tests/devmem.test yet and am not 100% sure how I'd even set that up in
>> mkroot under qemu, but I should work out something...)
> 
> I ran something like this:
> # devmem 0x000000008b300000 8 0x8FFFFFFFFFFFFFFF
> # devmem 0x000000008b300000 8
> 0x8fffffffffffffff
> 
> # devmem 0x000000008b300000 8 500k
> # devmem 0x000000008b300000 8
> 0x0000000007d000
> 
> # devmem 0x000000008b300000 8 0xFFFFFFFFFFFFFFFF
> # devmem 0x000000008b300000 8
> 0xffffffffffffffff

That would be the one that failed then?

> # devmem 0x000000008b300000 4 0xFFFFFFFF
> # devmem 0x000000008b300000 4
> 0xffffffff
> 
> # devmem 0x000000008b300000  4 0x1FFFFFFFF
> devmem: data: 8589934591 exceeds write width: 4

Which is true...

> I don't, but problem is I don't know if other users of devmem have
> come to expect and depend on this behavior. Are we ok potentially
> breaking compatibility for a bug fix?
> 
>> Is sizeof(unsigned long long) ever _not_ 8?
> 
> IIUC, technically the C spec only guarantees that it's at least 8.

Toybox uses LP64: https://landley.net/toybox/design.html#bits

Which technically only guarantees that long long is at _least_ 8 bytes, but I
have yet to see a system where it's bigger and somebody (the musl-libc
maintainer?) brought up several existing things changing it would break when I
asked him....

> But
> anyways, the intent of using sizeof() was to make sure any updates of
> the data/max_value types would propagate to the check.
> 
>>
>> I take it you're still ok with the decimal output being signed instead of
>> unsigned?
> 
> What do you mean, isn't %llx unsigned?

Line 72:

    printf((!strchr(*toys.optargs, 'x')) ? "%0*lld\n" : "0x%0*llx\n",
      bytes*2, data);

Probably fine, but now input is unsigned and output is signed so I thought I'd
ask somebody with domain experience using this command. :)

>> How does this look (untested):
> 
> I can test this out in a bit :) .
...
>>    // DATA? Report out of range values as errors rather than truncating.
>> -  if (writing) data = atolx_range(toys.optargs[2], 0, (1ULL<<(8*bytes))-1);
>> +  if (writing && (data = atollu(toys.optargs[2]))>(~0LL)>>(64-8*bytes))
> 
> Shouldn't it be (~0ULL)? I think (~0LL) would make this an arithmetic
> shift instead of a logical shift.

I said it was untested. :)

Yup, fixed.

Rob