On Tuesday 13 October 2009, Redirect "Slash" NIL wrote:
> Sorry about the inline patch. Please find the original diff file as a text
> attachment.
See appended comments. As is usual with this type of port, some
of the type warnings are real issues that need fixing. But a lot
of them seem to be compiler glitches:
- "%lld" being correct when the parameter is a "long long",
as it *IS* here, but for some reason this patch changed
those to require uint64_t and PRIi64.
- "%zu" being correct for a "size_t", but this changes them
to uint64_t and PRIu64
- "%jd" being correct for an "intmax_t", but again this
changing them to use 64-bit types.
Of the ones that are real issues, fixes for most of them are
in the tree now. (Can you verify they're fixed in your build
environment too?) But others seem to need different fixes.
Fixes that won't break _other_ systems, that is!
The jim-eventloop thing deserves a different approach...
> The only warning fix is in /src/helper/replacements.c and has to do with
> MinGW not linking a "long" being cast to "HANDLE" (= 64 bit void*), so I'm
> not sure it's worth splitting it (it's still 64 bit related).
> If you insist, I'll do it.
>
> The only way I found to address this was to explicitly detect the MinGW 64
> bit compilation flags ( #if (defined(_M_X64) || (defined(_M_AMD64))) ),
> which I'm not entirely satisfied with since it's far from universal. There's
> probably a better way to address this.
I think that deserves to merge standalone -- it's not a general
portability fix like the rest, it's Win64-specific. I'd be
inclined to merge it as-is, and let better fixes be submitted
later. The whole thing is inside an #ifdef _WIN32 anyway...
Can you address the comments below, against current git (which
should have fixes already, as noted)?
- Dave
> --- a/src/flash/flash.c
> +++ b/src/flash/flash.c
> @@ -939,7 +939,7 @@ static int handle_flash_write_bank_command(struct
> command_context_s *cmd_ctx, ch
> if (retval == ERROR_OK)
> {
> command_print(cmd_ctx,
> - "wrote %lld byte from file %s to flash bank
> %li at offset 0x%8.8" PRIx32 " in %s (%f kb/s)",
> + "wrote %" PRIi64 " byte from file %s to
> flash bank %li at offset 0x%8.8" PRIx32 " in %s (%f kb/s)",
> fileio.size,
This is wrong; fileio.size is a "long long" so "%lld" is correct;
while PRIi64 isn't.
This looks like a compiler bug. The toolchain you referenced seems
to be in active development ... so I'm quite reluctant to accept
such changes!
Possibly related: I notice that http://mingw-w64.sourceforge.net
has a release dated *TODAY*. Maybe your toolchain has an update
that addresses these issues?
> args[1],
> strtoul(args[0], NULL, 0),
> --- a/src/flash/mflash.c
> +++ b/src/flash/mflash.c
> @@ -474,8 +474,8 @@ static int mg_mflash_read_sects(void *buff, uint32_t
> sect_num, uint32_t sect_cnt
> residue = sect_cnt % 256;
>
> for (i = 0; i < quotient; i++) {
> - LOG_DEBUG("mflash: sect num : %" PRIu32 " buff : 0x%0lx",
> sect_num,
> - (unsigned long)buff_ptr);
> + LOG_DEBUG("mflash: sect num : %" PRIu32 " buff : 0x%0"PRIx64,
> sect_num,
> + (unsigned long long)buff_ptr);
This is wrong; buff_ptr is a pointer. A better fix for this
would be to use "%p" instead ...
(FIX merged)
> ret = mg_mflash_do_read_sects(buff_ptr, sect_num, 256);
> if (ret != ERROR_OK)
> return ret;
> @@ -485,8 +485,8 @@ static int mg_mflash_read_sects(void *buff, uint32_t
> sect_num, uint32_t sect_cnt
> }
>
> if (residue) {
> - LOG_DEBUG("mflash: sect num : %" PRIx32 " buff : %0lx",
> sect_num,
> - (unsigned long)buff_ptr);
> + LOG_DEBUG("mflash: sect num : %" PRIx32 " buff : %0"PRIx64,
> sect_num,
> + (unsigned long long)buff_ptr);
Same: buff_ptr is a pointer, so "%p" is better.
(FIX merged)
> return mg_mflash_do_read_sects(buff_ptr, sect_num, residue);
> }
>
> @@ -751,7 +751,7 @@ static int mg_write_cmd(struct command_context_s
> *cmd_ctx, char *cmd, char **arg
>
> duration_stop_measure(&duration, &duration_text);
>
> - command_print(cmd_ctx, "wrote %lli byte from file %s in %s (%f kB/s)",
> + command_print(cmd_ctx, "wrote %" PRIi64 " byte from file %s in %s (%f
> kB/s)",
> fileio.size, args[1], duration_text,
Same issue as before, with fileio.size ... it's "long long", so
what's coded there is already correct. Alternatively, maybe make
fileio.size become a "size_t" and use "%zd". (Except ... you had
issues with that, elsewhere.)
> (float)fileio.size / 1024.0 / ((float)duration.duration.tv_sec
> + ((float)duration.duration.tv_usec / 1000000.0)));
>
> --- a/src/flash/nand.c
> +++ b/src/flash/nand.c
> @@ -1609,7 +1609,7 @@ static int handle_nand_dump_command(struct
> command_context_s *cmd_ctx, char *cmd
> fileio_close(&fileio);
>
> duration_stop_measure(&duration, &duration_text);
> - command_print(cmd_ctx, "dumped %lld byte in %s",
> fileio.size, duration_text);
> + command_print(cmd_ctx, "dumped %" PRIi64 " byte in %s",
> fileio.size, duration_text);
Same fileio.size issue as before ...
> free(duration_text);
> duration_text = NULL;
> }
> --- a/src/helper/jim-eventloop.c
> +++ b/src/helper/jim-eventloop.c
> @@ -498,7 +498,7 @@ static int JimELAfterCommand(Jim_Interp *interp, int argc,
> int tlen ;
> jim_wide remain = 0;
> const char *tok = Jim_GetString(argv[2], &tlen);
> - if (sscanf(tok,"after#%lld",&id) == 1) {
> + if (sscanf(tok,"after#%" PRIi64 ,&id) == 1) {
This is ugly; "id" is a "jim_wide". There are two ways that
gets defined:
- Some crufty thing if "long long" exists; oddly, not "uint64_t".
- Else "long" (so "%lld" will be broken)
So that particular code has inherent portability issues. It's
implementing "after cancel after#NNN" ... I suspect the simplest
fix is likely to make that code use a generic type like "unsigned",
along with JimEventLoop.timeEventNextId (and its users, all of
which are in that file).
Otherwise, it's introducing nasty #ifdefs just for the scscanf,
and to no particular point ... IDs can roll over, doing so after
just 4 billion served is no more permitted to cause failure than
after ... a lot more. If either case ever happens. ;)
> remain = Jim_DeleteTimeHandler(interp, id);
> if (remain > -2) {
> Jim_SetResult(interp, Jim_NewIntObj(interp, remain));
> --- a/src/helper/jim.c
> +++ b/src/helper/jim.c
> @@ -4754,7 +4754,7 @@ const char *Jim_GetSharedString(Jim_Interp *interp,
> const char *str)
> Jim_AddHashEntry(&interp->sharedStrings, strCopy, (void*)1);
> return strCopy;
> } else {
> - long refCount = (long) he->val;
> + long long refCount = (long long) he->val;
Better fix: "intptr_t refCount" ... on 32 bit machines you
don't really want "long long".
(FIX merged)
>
> refCount++;
> he->val = (void*) refCount;
> @@ -4764,13 +4764,13 @@ const char *Jim_GetSharedString(Jim_Interp *interp,
> const char *str)
>
> void Jim_ReleaseSharedString(Jim_Interp *interp, const char *str)
> {
> - long refCount;
> + long long refCount;
> Jim_HashEntry *he = Jim_FindHashEntry(&interp->sharedStrings, str);
>
> if (he == NULL)
> Jim_Panic(interp,"Jim_ReleaseSharedString called with "
> "unknown shared string '%s'", str);
> - refCount = (long) he->val;
> + refCount = (long long) he->val;
As above: "intptr_t" not "long long".
(FIX merged)
> refCount--;
> if (refCount == 0) {
> Jim_DeleteHashEntry(&interp->sharedStrings, str);
> --- a/src/helper/log.c
> +++ b/src/helper/log.c
> @@ -473,12 +473,12 @@ void keep_alive()
> if (gdb_actual_connections)
> LOG_WARNING("keep_alive() was not invoked in the "
> "1000ms timelimit. GDB alive packet not "
> - "sent! (%lld). Workaround: increase "
> + "sent! (%" PRIi64 "). Workaround: increase "
I don't understand. Doesn't subtracting one "long long" from
another give you a "long long", making "%lld" correct? If not,
then it should give "int", making "%d" correct.
> "\"set remotetimeout\" in GDB",
> current_time-last_time);
> else
> LOG_DEBUG("keep_alive() was not invoked in the "
> - "1000ms timelimit (%lld). This may cause "
> + "1000ms timelimit (%" PRIi64 "). This may cause
> "
Same as above. Toolchain issue?
> "trouble with GDB connections.",
> current_time-last_time);
> }
> --- a/src/helper/replacements.c
> +++ b/src/helper/replacements.c
> @@ -169,7 +169,12 @@ int win_select(int max_fd, fd_set *rfds, fd_set *wfds,
> fd_set *efds, struct time
> /* build an array of handles for non-sockets */
> for (i = 0; i < max_fd; i++) {
> if (SAFE_FD_ISSET(i, rfds) || SAFE_FD_ISSET(i, wfds) ||
> SAFE_FD_ISSET(i, efds)) {
> + /* Fix cast warnings treated as errors for MinGW-64 */
> +#if (defined(_M_X64) || (defined(_M_AMD64)))
> + long long handle = _get_osfhandle(i);
> +#else
> long handle = _get_osfhandle(i);
> +#endif
This is all within an "#ifdef _WIN32" ... so I'm just a
bit puzzled why it applies on WIN64. ;)
But then, I don't claim to understand much about Windows any more.
I would have expected "_WIN64" to be the relevant #ifdef...
(I didn't touch this. Suggest you send this patch alone with
a subject line calling it out as a Windows-specific issue, to
get more attention from folk with the relevant background...)
> handles[n_handles] = (HANDLE)handle;
> if (handles[n_handles] == INVALID_HANDLE_VALUE) {
> /* socket */
> @@ -244,7 +249,12 @@ int win_select(int max_fd, fd_set *rfds, fd_set *wfds,
> fd_set *efds, struct time
> if (WAIT_OBJECT_0 ==
> WaitForSingleObject(handles[i], 0)) {
> if
> (SAFE_FD_ISSET(handle_slot_to_fd[i], rfds)) {
> DWORD dwBytes;
> +
> +#if (defined(_M_X64) || (defined(_M_AMD64)))
> + long long handle =
> _get_osfhandle(handle_slot_to_fd[i]);
> +#else
> long handle =
> _get_osfhandle(handle_slot_to_fd[i]);
> +#endif
>
> if
> (PeekNamedPipe((HANDLE)handle, NULL, 0, NULL, &dwBytes, NULL))
> {
> --- a/src/jtag/core.c
> +++ b/src/jtag/core.c
> @@ -1008,7 +1008,7 @@ static bool jtag_examine_chain_match_tap(const struct
> jtag_tap_s *tap)
> for (ii = 0; ii < tap->expected_ids_cnt; ii++)
> {
> char msg[32];
> - snprintf(msg, sizeof(msg), "expected %hhu of %hhu",
> + snprintf(msg, sizeof(msg), "expected %" PRIu8 " of %" PRIu8,
While not incorrect, the issue here is that a "uint8_t" is being
used as a loop counter. I had problems here in some other context.
Better fix: convert to integers, and work like sanes loops already do...
(FIX merged)
> ii + 1, tap->expected_ids_cnt);
> jtag_examine_chain_display(LOG_LVL_ERROR, msg,
> tap->dotted_name, tap->expected_ids[ii]);
> --- a/src/pld/pld.c
> +++ b/src/pld/pld.c
> @@ -195,9 +195,9 @@ static int handle_pld_load_command(struct
> command_context_s *cmd_ctx,
> gettimeofday(&end, NULL);
> timeval_subtract(&duration, &end, &start);
>
> - command_print(cmd_ctx, "loaded file %s to pld device %lu in
> %jis %jius",
> + command_print(cmd_ctx, "loaded file %s to pld device %lu in %"
> PRIi64 "s %" PRIi64 "us",
> args[1], strtoul(args[0], NULL, 0),
> - (intmax_t)duration.tv_sec, (intmax_t)duration.tv_usec);
> + (unsigned long long)duration.tv_sec, (unsigned long
> long)duration.tv_usec);
Again, "unsigned long long" != "int64_t"...
If the toolchain you're using doesn't understand "%ji" for some
reason (intmax_t) then I agree that "unsigned long long" is a
more portable answer. But not PRIi64; plain old "%lld" should
work just fine. (If you must use PRIi64, then use "uint64_t"
instead of "unsigned long long".)
On the other hand, I'm not sure that "long long" is guaranteed
to exist... we use it all over the place though, so depending
on it will not add more breakage.
> }
>
> return ERROR_OK;
> --- a/src/svf/svf.c
> +++ b/src/svf/svf.c
> @@ -442,7 +442,7 @@ static int handle_svf_command(struct command_context_s
> *cmd_ctx, char *cmd, char
> }
>
> // print time
> - command_print(cmd_ctx, "%lld ms used", timeval_ms() - time_ago);
> + command_print(cmd_ctx, "%" PRIi64 " ms used", timeval_ms() - time_ago);
This is "long long" minus "long long" thing again ... why is
your toolchain not treating that result as a "long long"? Seems
like a toolchain bug. The "%lld" format is correct.
>
> free_all:
>
> --- a/src/target/arm11.c
> +++ b/src/target/arm11.c
> @@ -1606,10 +1606,10 @@ int arm11_run_algorithm(struct target_s *target, int
> num_mem_params, mem_param_t
> // return ERROR_FAIL;
>
> // Save regs
> - for (size_t i = 0; i < 16; i++)
> + for (unsigned long long i = 0; i < 16; i++)
Using a 64-bit integer to count up to 16 ... overkill!
Fixed by changing to "unsigned".
(FIX merged)
> {
> context[i] =
> buf_get_u32((uint8_t*)(&arm11->reg_values[i]),0,32);
> - LOG_DEBUG("Save %zi: 0x%" PRIx32 "",i,context[i]);
> + LOG_DEBUG("Save %" PRIu64 ": 0x%" PRIx32 "",i,context[i]);
> }
>
> cpsr = buf_get_u32((uint8_t*)(arm11->reg_values + ARM11_RC_CPSR),0,32);
> --- a/src/target/armv7a.c
> +++ b/src/target/armv7a.c
> @@ -226,7 +226,7 @@ int armv7a_arch_state(struct target_s *target)
> state[armv7a->armv4_5_mmu.armv4_5_cache.d_u_cache_enabled],
> state[armv7a->armv4_5_mmu.armv4_5_cache.i_cache_enabled]);
>
> - if (armv4_5->core_mode == ARMV7A_MODE_ABT)
> + if ((armv7a_t)armv4_5->core_mode == ARMV7A_MODE_ABT)
Well that's a pickle! The difference between the two core type
enums is that V7A adds a (secure) monitor mode. And since both
types have a non-existent "MODE_ANY" case ... neither enum is
a very type-safe thing in the first place.
Plus, clearly it's a larger scale type-mismatch to treat ARMv7 as
an ARMv5 ... I think I see the armv4_5 core more or less evolving
into "core ARM stuff"...
So I just made core_mode into an "int", since that's less of a
rude type violation than casting one enum type into another.
Those enums are used more or less like #defines, in any case.
(FIX merged)
> armv7a_show_fault_registers(target);
>
> return ERROR_OK;
> --- a/src/target/target.c
> +++ b/src/target/target.c
> @@ -2441,7 +2441,7 @@ static int handle_dump_image_command(struct
> command_context_s *cmd_ctx, char *cm
>
> if (retval == ERROR_OK)
> {
> - command_print(cmd_ctx, "dumped %lld byte in %s",
> + command_print(cmd_ctx, "dumped %"PRIi64" byte in %s",
> fileio.size, duration_text);
The fileio.size bug creeps in again....
> free(duration_text);
> }
> @@ -2831,9 +2831,9 @@ static int handle_virt2phys_command(command_context_t
> *cmd_ctx,
>
> static void writeData(FILE *f, const void *data, size_t len)
> {
> - size_t written = fwrite(data, 1, len, f);
> + unsigned long long written = fwrite(data, 1, len, f);
> if (written != len)
> - LOG_ERROR("failed to write %zu bytes: %s", len,
> strerror(errno));
> + LOG_ERROR("failed to write %" PRIu64 " bytes: %s", len,
> strerror(errno));
This was correct as written. What is the toolchain complaint?
A "%zu" format is specifically for printing a size_t value.
And again, "unsigned long long" != "uint64_t" as required for PRIu64...
> }
>
> static void writeLong(FILE *f, int l)
> --- a/src/target/trace.c
> +++ b/src/target/trace.c
> @@ -58,7 +58,7 @@ static int handle_trace_point_command(struct
> command_context_s *cmd_ctx, char *c
>
> for (i = 0; i < trace->num_trace_points; i++)
> {
> - command_print(cmd_ctx, "trace point 0x%8.8" PRIx32 "
> (%lld times hit)",
> + command_print(cmd_ctx, "trace point 0x%8.8" PRIx32 "
> (%" PRIi64 " times hit)",
> trace->trace_points[i].address,
> (long
> long)trace->trace_points[i].hit_counter);
This is again the issue that "%lld" is specifically for a "long long"
value ... what's the issue?
> }
> --- a/src/xsvf/xsvf.c
> +++ b/src/xsvf/xsvf.c
> @@ -935,9 +935,9 @@ static int handle_xsvf_command(struct command_context_s
> *cmd_ctx, char *cmd, cha
>
> if (unsupported)
> {
> - off_t offset = lseek(xsvf_fd, 0, SEEK_CUR) - 1;
> + unsigned long long offset = lseek(xsvf_fd, 0, SEEK_CUR) - 1;
> command_print(cmd_ctx,
> - "unsupported xsvf command (0x%02X) at offset
> %jd, aborting",
> + "unsupported xsvf command (0x%02X) at offset %"
> PRIi64 ", aborting",
Again, "%jd" is correct for an intmax_t value, and PRIi64 is
not guaranteed to work. What's the toolchain error this is
is trying to resolve?
> uc, (intmax_t)offset);
> return ERROR_FAIL;
> }
_______________________________________________
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development
