[flashrom] [PATCH 06/10] uintptr_t-ify physmap functions.

[Carl-Daniel Hailfinger]

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 at 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 at 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 at gmx.net>

Sorry for my earlier Nack.

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/