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

[Rob Landley]

On 8/31/23 02:00, Michael Shavit via Toybox wrote:
> Replace the left-shift method of computing the maximum allowed write as
> it may exibit undefined behavior.
> Add and use an unsigned version of atolx to support writing any 64bit
> value.
> 
> * When bytes equals 8, devmem will left-shift an unsigned long long by
>   64 bits which is undefined behavior if sizeof(unsigned long long) is
>   64.
> * In addition, atolx_range's 'high' parameter is a signed long long and
>   will therefore interpret a high value of ULLONG_MAX as -1.
> * Finally, atolx relies on strtoll under the hood, which will reject
>   values beyond LLONG_MAX.

Alas, building on 32 bit targets goes:

toys/other/devmem.c: In function 'devmem_main':
toys/other/devmem.c:61:14: warning: cast to pointer from integer of different
size [-Wint-to-pointer-cast]
   61 |   } else p = (void *)addr;

I could adjust _just_ addr to use unsigned long instead of unsigned long long,
but I don't want two atollu()/atolu() and letting you do a longer address and
silently truncating it isn't great either. Also, a 32-bit platform can't do an
atomic 64 bit read/write either (it's gonna do 2 32 bit ones behind the scenes)
and I kinda don't want to claim it won't? Which means removing the 8 byte option
when building for 32 bits, which is trivial to do in the code:

+    else if (sizeof(long)==8 && bytes==8) *(unsigned long *)p = data;

The tricksy bit is I haven't got a mechanism for making the help text vary on 32
vs 64 bit platforms. (I've designed a few over the years but never implemented
them because it was overcomplicated. I regret the grab_dashlines() stuff from
scripts/config2help.c but haven't gone back and ripped it out yet.)

So the simple fix would still CLAIM to be able to do 8 byte read/write on 32 bit
platforms, and rewriting the help text to say 'on 32 bit targets'
unconditionally in the text is just ugly...

(Why is the code usually easier to fix than the documentation and test suite?)

Anyway, all that is why I was previously using "long" and relying on LP64. I
suspect "unsigned long" and eliminating 8 byte writes on 32 bit targets (since
they can't actually DO that) is still correct, but I could instead keep data as
"long long" and accept 8 bytes on 32 bit (knowing that's NOT atomic there, but
it's a question of what you wanna be inconsistent about).

If addr becomes unsigned long but I've only got atollu() I can read addr's value
into dat afirst and then if (sizeof(long)==4 && data>0xFFFFFFFFU)
perror_exit("no"); else addr = data; which should dead code eliminate and
register rename itself away on 64 bit targets. (Sorry, variable lifetime
analysis, register renaming is hardware...)

It's a toss up. Less correct behavior on the less used platform and a rough edge
with the silently non-atomic behavior, but which matches the invariant help
text... or correct behavior on 32 bit (given the constraints) but the
documentation is always for 64 bit. (Or blather on about corner cases 99.9% of
the userbase will never care about in the help text, which I'd rather not. Terse
struggles with correct and I'm pretty sure terse wins here.)

Rob