On Fri, 2009-06-19 at 21:24 -0400, Duane Ellis wrote:
> Zach Welch wrote:
> > On Fri, 2009-06-19 at 20:31 -0400, Duane Ellis wrote:
> >
> >> duane> FYI - I committed several cygwin specific printf() warning fixes.
> >> duane> Simple cast to fix
> >> duane> these where causing "-Werror" failures on cygwin.
> >>
> >> zach> I was just about to post some patches to show how to fix all of these
> >> zach> correctly, as casts are not the right way to do it.
> >>
> >> In some cases, I can agree, but in others - I don't agree. A cast is by
> >> far the most simplest solution.
> >>
> >> For instance, look at this:
> >>
> >> LOG_WARNING("writing %d bytes only - as image section is %d
> >> bytes and bank is only %d bytes", \
> >> (int)(c->base + c->size - run_address),
> >> (int)(run_size), (int)(c->size));
> >>
> >> (a) look at what is being printed
> >> (b) the possible ranges of numbers
> >> (c) the number of parameters involved in the expressions
> >>
> >> At some point - having to dig back 20 to 30 -lines- to *random* places -
> >> because some variables are:
> >>
> >> (1) function parameters
> >> (2) defined at the top most block
> >> (3) defined locally to the local block
> >> (4) defined a few lines down from the top most block
> >> (5) often simplistic 'i/j/k' type vars that are reused because they are
> >> handy.
> >> (6) By the time I personally dig through the above, and determine all
> >> types involved
> >> (7) Then understand the underlying arithmetic integer promotion order...
> >> *note* this set of equations are simple...
> >> (8) a cast to a basic type is truly a very *simple* solution,
> >> (9) a cast to a basic type in this case is the K.I.S.S. solution.
> >>
> >> Either that - or we *need* a different solution here, the above is absurd.
> >>
> >> Comments?
> >>
> >
> > The simple solution is not the correct one in this case. The problem
> > is that your assumptions about ranges are not portable; that is the
> > point in using these macros.
> >
> >
>
> ====
>
> You miss an important point, it is *NOT* the possible range of values
> the *TYPE* may take on.
>
> It is the range of values the **ACTUAL*DATA** might take on that is
> important here.
> Consider:
>
> uint32_t x;
> void funny_function( uint32_t );
>
> for( x = 0 ; x < 10 ; x++ ){
> printf(" X = %d, Calling funny function\n", (int)(x));
> funny_function( x ) ;
> }
>
> In the above, I have several choices, (a) I could have used an "int"
> variable - then *cast* it to a uint32_t at the function call (note here:
> I am partial to x as a loop control variable, others like i/j/k) or -
> because (b) the value will never be negative, I just choose 'uint32_t'
> because it was convient.
>
> Is the printf() statement *WRONG* - or is the printf() statement
> *CORRECT* in this senario.
It is wrong if you want to be safe when the ranges change, and with
general purpose functions the ranges may be set outside the scope of a
function that uses them. Safe code would not be making such assumptions
about the actual data; even if they hold today, they may not tomorrow.
That said, your pedantic example would work acceptably, though I would
argue the cast would be more correct to 'unsigned' using format '%u'.
> ====
>
> If we had to fix this, what would we do to fix this:
>
> Today - we do not have macros, or types like GDB/GCC/GAS.
> GDB for instance uses CORE_ADDR - and has infrastructure behind it.
>
> Here, under OpenOCD we generally:
>
> (1) assume that all embedded openocd targets are *32bit* at most...
Today maybe, but this is an unacceptable shortcoming.
> (2) assume all host platforms are at least 32bit host platforms
Again, this is a fair assumption today, but I do not think it again to
be a shortcoming that C99 types are designed to overcome.
> (3) and thus, target addresses are generally equal to - or smaller then
> the host "unsigned integers"
This is why we have intptr_t. The code shouldn't care. It's a bug if
it does.
> (4) In most, if not all of the "u32" cases, the format string specifies
> "%08x" or equal.
It should... an audit would be required to confirm this is the case.
> Are these assumptions *wrong*? If so, we have major other problems we
> must deal with.
We have major other problems that we must deal with.
> ====
>
> There are numerous examples of this where - for instance the code does
> the following:
>
> LOG_ERROR("Verfication error address 0x%08x, read back 0x%02x,
> expected 0x%02x", address + wrote + i, readback[i], chunk[i]);
>
> In this specific case, the *correct* fix is something else.
>
> The "address" parameter really needs a *function* that handles the width
> value. Otherwise, a "64bit" target will not work. We are not proposing
> to fix these situations today are we? Doing so would really be better
> served by a function something like:
>
> char *target_address_as_hex( CORE_ADDR address_value );
>
> One can 'question' the two 8bit values printed, allow me to make my
> point another way.
Yup. I am definitely not advocating for completely pedantic replacement
of all format strings. Certainly, the single byte values can probably
be cast to unsigned. :)
I was saying that -- if you saw a warning -- the correct fix is probably
to use the PRI macros. Otherwise, I am not suggesting changing all
instances of these strings.
> ====
>
> Here is another example:
>
> LOG_INFO( "device id = 0x%08x (manuf 0x%03x dev 0x%02x, ver 0x%03x)",
> device_id, (device_id>>1)&0x7ff, (device_id>>12)&0xff,
> (device_id>>20)&0xfff );
>
> The format strings alone tell me that a 32bit unsigned *INTEGER* is
> sufficient.
> As does the math operators.
>
> Since our *basic* assumption of the host platform is at least a "32bit
> unsigned int"
> A simple cast is valid here.
>
> Are you suggesting that our *basic* host assumption is wrong?
If we want OpenOCD to work on more C99-compatible targets, yes.
> ====
>
> duane> 9) a cast to a basic type in this case is the K.I.S.S.
> solution.duane>
>
> zach> I don't care one white about KISS if the code is wrong.
>
> Is this a pedantic position/point you are trying to make?
Clearly. The real world will never be so clear cut, no matter how hard
anyone tries to make it. ;)
> The code today has *major* problems if/when we have a 64bit target.
> The code is more *wrong* when we have 32bit hosts and 64bit targets.
Yup, and yup.
Cheers,
Zach
_______________________________________________
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development
