Am 15.07.2013 22:49 schrieb Stefan Tauner:
On Mon, 15 Jul 2013 10:20:29 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Am 10.07.2013 21:17 schrieb Stefan Tauner:
unsigned long is not the right type for manipulating pointer values. Since C99 there are suitable unsigned and signed types available, namely uintptr_t and intptr_t respectively.
Use them in physmap.c and its callers where applicable.
This patch also changes the display width of all address values in physmap.c to 16/8 hex characters depending on the actual size.
Signed-off-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at
Why don't you use PRIxPTR_WIDTH everywhere including dummyflasher.c?
That's a bug AFAICS, thanks.
Put another way, is there a reason to use PRIxPTR_WIDTH at all?
IIRC the C99 standard says that PRIxPTR guarantees a canonical formatting. I'm guessing the canonical formatting means (for most platforms) that the printed pointers have constant width, i.e. the PRIxPTR_WIDTH specifier is superfluous. I would avoid that specifier unless shown a real-world case where PRIxPTR formatted pointers have variable length inside the same flashrom executable.
Do we care that C99 doesn't guarantee a 0x prefix for printed addresses with PRIxPTR? Will that cause confusion?
The main point of it is to get nicely aligned/readable addresses that are padded to the right(tm) size, and yes I guess the guaranteed 0x prefix too (I can't remember TBH :).
AFAIK the 0x prefix is not guaranteed, and it may even be missing on Windows. Someone would have to test (preferably not in Wine).
I think PRIxPTR is a good idea, but I'm not so sure about the side effects.
There's also the problem of programmer_map_flash_region. It should not take an uintptr_t. The address parameter is a horrible mess: A physical address from the CPU point of view for the internal programmer, a top-of-4-GB aligned fake address for some other programmers, and a bottom-aligned address for the rest of our programmers. If we want to do this the right way, we need to have a wrapper for physmap in the "internal" programmer case instead of setting map_flash_region = physmap. And then I need to update my patch which abstracts the flash bus address vs programmer address conflict into something that won't cause madness.
Hm, I need to look into this again. Using a union for the type and a wrapper does not sound so bad to me (without looking at the code :)
Not what I meant. Turns out that my proposal is unworkable anyway.
This could at least be made platform-independent without jumping through hoops.
Overall, while I'd like to have this patch in, it conceptually breaks as much stuff as it fixes. I don't see this getting fixed within the release timeframe, so I'd like to postpone.
Fine with me, but this means no win64 support in this stable.
Once the PRIxPTR_WIDTH stuff is consistent (preferably removed everywhere), this is: Acked-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Sorry for my earlier Nack.
Regards, Carl-Daniel