[Bug 1001510] New: Fix compiler warnings about mismatch between log() format string and argument values.

[Bug 1001510] New: Fix compiler warnings about mismatch between log() format string and argument values.

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001510

           Summary: Fix compiler warnings about mismatch between log()
                    format string and argument values.
           Product: eCos
           Version: CVS
          Platform: All
        OS/Version: Other
            Status: NEW
          Severity: enhancement
          Priority: low
         Component: TCP/IP
        AssignedTo: unassigned@bugs.ecos.sourceware.org
        ReportedBy: grant.b.edwards@gmail.com
                CC: ecos-bugs@ecos.sourceware.org
             Class: Advice Request


Created an attachment (id=1610)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1610)
Patch to fix warnings about format-string/argument-value mismatches

log() messages that print tv_sec and tv_usec values were generating
compiler warnings about a mismatch between the format strings and
the actual argumen values.  Changing the format strings from %ld to %u
eliminates the warnings.

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 1001510] Fix compiler warnings about mismatch between log() format string and argument values.

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001510

Jonathan Larmour <jifl@ecoscentric.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jifl@ecoscentric.com

--- Comment #1 from Jonathan Larmour <jifl@ecoscentric.com> 2012-03-03 03:51:53 GMT ---
Hmm, I thought struct timeval's held time_t's which are signed ints in eCos
(see isoinfra's time.h and sys/time.h), in which case the changes should be to
%d?

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 1001510] Fix compiler warnings about mismatch between log() format string and argument values.

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001510

--- Comment #2 from Grant Edwards <grant.b.edwards@gmail.com> 2012-03-03 16:12:33 GMT ---
I'll double-check the type of the struct timeval feilds.

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 1001510] Fix compiler warnings about mismatch between log() format string and argument values.

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001510

Grant Edwards <grant.b.edwards@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |NOTABUG

--- Comment #3 from Grant Edwards <grant.b.edwards@gmail.com> 2012-03-06 19:32:08 GMT ---
You're right.  Both fields in struct timeval are signed.  tv_sec is 'int' and
tv_usec is 'long int'.  Using the old format I don't get any compiler warnings
I think "%d.%ld" is probably more correct, but it's moot on 32-bit
architectures. Either way the output is somewhat misleading -- "%d.%06ld" is
probably the best answer.

I'm going to leave things the way they are.

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 1001510] Fix compiler warnings about mismatch between log() format string and argument values.

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001510

Grant Edwards <grant.b.edwards@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |REOPENED
         Resolution|NOTABUG                     |

--- Comment #4 from Grant Edwards <grant.b.edwards@gmail.com> 2012-03-06 22:04:31 GMT ---
For some reason, I wasn't getting warnings for a while, but now I am
getting them again.  I'm a little puzzled by this, but here are the
warnings I'm getting with gcc 4.6.2:

bsd_tcpip/current/src/ecos/support.c:543:5: warning: format '%ld' expects
argument of type 'long int', but argument 3 has type 'time_t' [-Wformat]
bsd_tcpip/current/src/ecos/support.c:543:5: warning: format '%ld' expects
argument of type 'long int', but argument 4 has type 'time_t' [-Wformat]
bsd_tcpip/current/src/ecos/support.c:551:5: warning: format '%ld' expects
argument of type 'long int', but argument 3 has type 'time_t' [-Wformat]
bsd_tcpip/current/src/ecos/support.c:551:5: warning: format '%ld' expects
argument of type 'long int', but argument 4 has type 'time_t' [-Wformat]
bsd_tcpip/current/src/ecos/support.c:559:5: warning: format '%ld' expects
argument of type 'long int', but argument 3 has type 'time_t' [-Wformat]
bsd_tcpip/current/src/ecos/support.c:559:5: warning: format '%ld' expects
argument of type 'long int', but argument 4 has type 'time_t' [-Wformat]

It looks like tv_sec is always time_t (signed int), but depending on
CDL options, tv_usec can be time_t or unsigned long.  I'm now thinking
the right thing to do is to leave the format as %ld and cast each of
the parameters to (long).  Assuming the fields remain signed types,
that should work correctly regardless of whether they're (int) or
(long int).

Any opinions on whether we should change the format from "%ld.%ld" to
"%ld.%06ld"?  With the current format, 1sec+6usec is displayed as
"1.6" -- I think it would be more intuitive if it were displayed as
"1.000006" but I don't know if that's going to break some sort of
automated regression testing.

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 1001510] Fix compiler warnings about mismatch between log() format string and argument values.

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001510

Grant Edwards <grant.b.edwards@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1610|0                           |1
        is obsolete|                            |

--- Comment #5 from Grant Edwards <grant.b.edwards@gmail.com> 2012-03-06 22:15:03 GMT ---
Created an attachment (id=1617)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1617)
Fix compiler warnings about printf format/arg mismatch.

Updated patch that goes with my recent proposal of 

    "%ld.%06ld", (long)tv_sec, (long)tv_usec

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 1001510] Fix compiler warnings about mismatch between log() format string and argument values.

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001510

--- Comment #6 from Jonathan Larmour <jifl@ecoscentric.com> 2012-03-09 03:38:21 GMT ---
Created an attachment (id=1634)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1634)
time_t patch

(In reply to comment #3)
> > You're right.  Both fields in struct timeval are signed.  tv_sec is 'int' and
> > tv_usec is 'long int'.

I don't see that. The default is that in isoinfra, struct timeval is set to
contain two time_t's, which by default are ints. If POSIX is included, then
that is overridden[1] so it contains two longs. I don't see any mix of int and
long. Are you thinking of struct timespec instead?

[1] Using this in the POSIX CDL:
 requires         { CYGBLD_ISO_STRUCTTIMEVAL_HEADER == \
                             "<cyg/posix/sys/time.h>" }

> > Using the old format I don't get any compiler warnings
> > I think "%d.%ld" is probably more correct, but it's moot on 32-bit
> > architectures. Either way the output is somewhat misleading -- "%d.%06ld" is
> > probably the best answer.

Looking at the standard, the tv_sec should be a time_t, and tv_usec should be a
suseconds_t. I can see no minimum width requirements - only that suseconds_t
must be signed; and although I haven't seen it explicitly the usage of
(time_t)-1 including example code in the POSIX standard makes it clear that it
should be signed as well.

Indeed time_t can sometimes be 32-bits, but now increasingly 64-bits, to solve
the year 2038 problem: http://en.wikipedia.org/wiki/Year_2038_problem

Also see this:
http://en.wikipedia.org/wiki/Unix_time#Representing_the_number

> > It looks like tv_sec is always time_t (signed int), but depending on
> > CDL options, tv_usec can be time_t or unsigned long.  I'm now thinking
> > the right thing to do is to leave the format as %ld and cast each of
> > the parameters to (long).  Assuming the fields remain signed types,
> > that should work correctly regardless of whether they're (int) or
> > (long int).

Although we're not yet in a position to convert wholesale to 64-bit
time_t, using long would not be sufficient when we did - it would need to
be long long and so %lld.

But in the first instance, yes we could change time_t to a long. And I'm
attaching a patch to do that which I think I ought to commit regardless.
Comments?

Even so, the format specifier for time_t does happen to be a particular
problem. So for this file, how about instead using something like:

#define INTFORMAT(_t_) (sizeof(_t_) == sizeof(long long) ? "lld" : "ld")

log(LOG_DEBUG, "%s: = %" INTFORMAT(tp->tv_sec) ".%06"
INTFORMAT(tp->tv_usec) "\n", __FUNCTION__, (

Jifl

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 1001510] Fix compiler warnings about mismatch between log() format string and argument values.

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001510

--- Comment #7 from Jonathan Larmour <jifl@ecoscentric.com> 2012-03-09 03:39:50 GMT ---
(In reply to comment #4)
> Any opinions on whether we should change the format from "%ld.%ld" to
> "%ld.%06ld"?  With the current format, 1sec+6usec is displayed as
> "1.6" -- I think it would be more intuitive if it were displayed as
> "1.000006" but I don't know if that's going to break some sort of
> automated regression testing.

FAOD, I definitely agree about the %06 bit. I suspect it was an oversight.

Jifl

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 1001510] Fix compiler warnings about mismatch between log() format string and argument values.

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001510

--- Comment #8 from Grant Edwards <grant.b.edwards@gmail.com> 2012-03-09 14:56:42 GMT ---
(In reply to comment #6)

>>> You're right.  Both fields in struct timeval are signed.  tv_sec
>>> is 'int' and tv_usec is 'long int'.
> 
> I don't see that.

Well, I "saw" it, but it wasn't really there...

> The default is that in isoinfra, struct timeval is set to contain
> two time_t's, which by default are ints. If POSIX is included, then
> that is overridden[1] so it contains two longs. I don't see any mix
> of int and long. Are you thinking of struct timespec instead?

Yes, I was looking at the definition for timespec.

> Although we're not yet in a position to convert wholesale to 64-bit
> time_t, using long would not be sufficient when we did - it would
> need to be long long and so %lld.
>
> But in the first instance, yes we could change time_t to a long. And
> I'm attaching a patch to do that which I think I ought to commit
> regardless.  Comments?

Agreed.

> Even so, the format specifier for time_t does happen to be a particular
> problem. So for this file, how about instead using something like:
> 
> #define INTFORMAT(_t_) (sizeof(_t_) == sizeof(long long) ? "lld" : "ld")
>
> log(LOG_DEBUG, "%s: = %" INTFORMAT(tp->tv_sec) ".%06" INTFORMAT(tp->tv_usec) "\n", __FUNCTION__, 

That looks like a good solution.  The evaluation of the the ternary
expression should happen at compile time so it won't slow down the
log() operation any.

Unless anybody objects, I'll go ahead and commit that.

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 1001510] Fix compiler warnings about mismatch between log() format string and argument values.

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001510

--- Comment #9 from Jonathan Larmour <jifl@ecoscentric.com> 2012-03-09 15:09:08 GMT ---
Yes go ahead. I've committed the isoinfra part.

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 1001510] Fix compiler warnings about mismatch between log() format string and argument values.

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001510

--- Comment #10 from Grant Edwards <grant.b.edwards@gmail.com> 2012-03-09 16:40:04 GMT ---
(In reply to comment #8)

>> Even so, the format specifier for time_t does happen to be a particular
>> problem. So for this file, how about instead using something like:
>> 
>> #define INTFORMAT(_t_) (sizeof(_t_) == sizeof(long long) ? "lld" : "ld")
>>
>> log(LOG_DEBUG, "%s: = %" INTFORMAT(tp->tv_sec) ".%06" INTFORMAT(tp->tv_usec) "\n", __FUNCTION__, 
> 
> That looks like a good solution.

Except it doesn't compile. :)

Concatenation of adjacent string literals only works with string
literals.  It doesn't work with expressions of type (const char *),
even if they can be evaluated at compile time.  The ternary operation
needs to be moved into the pre-processor stage, and that's a problem,
since the pre-processor knows nothing about variable types.

Using "%lld.%06lld" and casting both values to (long long) will work,
but it adds overhead in the most common case today (time_t is 32 bits
and long long is 64).

I'll try to think up something...

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 1001510] Fix compiler warnings about mismatch between log() format string and argument values.

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001510

--- Comment #11 from Jonathan Larmour <jifl@ecoscentric.com> 2012-03-09 16:53:19 GMT ---
Maybe sinec this is only used for debug, using diag_sprintf isn't so bad.

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 1001510] Fix compiler warnings about mismatch between log() format string and argument values.

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001510

--- Comment #12 from Grant Edwards <grant.b.edwards@gmail.com> 2012-03-13 14:38:25 GMT ---
(In reply to comment #11)
> Maybe sinec this is only used for debug, using diag_sprintf isn't so bad.

Something like this?

  #define INTFORMAT(_t_) (sizeof(_t_) == sizeof(long long) ? "lld" : "ld")

  void 
  microtime(struct timeval *tp)
  {
      *tp = ktime;
  #fdef CYGPKG_NET_FREEBSD_LOGGING
      if (cyg_net_log_mask & LOG_DEBUG)
        {
          char fmt[32];
          diag_sprintf(fmt, "%%s: = %%%s.%%06%s\n", INTFORMAT(tp->tv_sec),
INTFORMAT(tp->tv_usec));
          log(LOG_DEBUG, fmt, __FUNCTION__, tp->tv_sec, tp->tv_usec);
        }
  #endif
      ktime.tv_usec++;  // In case clock isn't running yet
  }

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 1001510] Fix compiler warnings about mismatch between log() format string and argument values.

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001510

--- Comment #13 from Jonathan Larmour <jifl@ecoscentric.com> 2012-03-13 16:09:07 GMT ---
(In reply to comment #12)
> (In reply to comment #11)
> > Maybe sinec this is only used for debug, using diag_sprintf isn't so bad.
> 
> Something like this?
> 
[snip]

Yes. A bit convoluted, but it seems safest. Any alternative suggestion welcome.
But if you can't think of anything better, just go for that.

Jifl

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 1001510] New: Fix compiler warnings about mismatch between log() format string and argument values.

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001510

           Summary: Fix compiler warnings about mismatch between log()
                    format string and argument values.
           Product: eCos
           Version: CVS
          Platform: All
        OS/Version: Other
            Status: NEW
          Severity: enhancement
          Priority: low
         Component: TCP/IP
        AssignedTo: unassigned@bugs.ecos.sourceware.org
        ReportedBy: grant.b.edwards@gmail.com
                CC: ecos-bugs@ecos.sourceware.org
             Class: Advice Request


Created an attachment (id=1610)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1610)
Patch to fix warnings about format-string/argument-value mismatches

log() messages that print tv_sec and tv_usec values were generating
compiler warnings about a mismatch between the format strings and
the actual argumen values.  Changing the format strings from %ld to %u
eliminates the warnings.

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.