diff mbox

[v2,06/30] trace: Fix parameter types in migration

Message ID 20170313195547.21466-7-eblake@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Eric Blake March 13, 2017, 7:55 p.m. UTC
An upcoming patch will let the compiler warn us when we are silently
losing precision in traces; update the trace definitions to pass
through the full value at the callsite.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 migration/trace-events | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Dr. David Alan Gilbert March 13, 2017, 8:07 p.m. UTC | #1
* Eric Blake (eblake@redhat.com) wrote:
> An upcoming patch will let the compiler warn us when we are silently
> losing precision in traces; update the trace definitions to pass
> through the full value at the callsite.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  migration/trace-events | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/migration/trace-events b/migration/trace-events
> index 7372ce2..079d4e6 100644
> --- a/migration/trace-events
> +++ b/migration/trace-events
> @@ -7,7 +7,7 @@ qemu_loadvm_state_section_partend(uint32_t section_id) "%u"
>  qemu_loadvm_state_post_main(int ret) "%d"
>  qemu_loadvm_state_section_startfull(uint32_t section_id, const char *idstr, uint32_t instance_id, uint32_t version_id) "%u(%s) %u %u"
>  qemu_savevm_send_packaged(void) ""
> -loadvm_handle_cmd_packaged(unsigned int length) "%u"
> +loadvm_handle_cmd_packaged(size_t length) "%zu"

OK, that makes sense.

>  loadvm_handle_cmd_packaged_main(int ret) "%d"
>  loadvm_handle_cmd_packaged_received(int ret) "%d"
>  loadvm_postcopy_handle_advise(void) ""
> @@ -186,7 +186,7 @@ postcopy_ram_enable_notify(void) ""
>  postcopy_ram_fault_thread_entry(void) ""
>  postcopy_ram_fault_thread_exit(void) ""
>  postcopy_ram_fault_thread_quit(void) ""
> -postcopy_ram_fault_thread_request(uint64_t hostaddr, const char *ramblock, size_t offset) "Request for HVA=%" PRIx64 " rb=%s offset=%zx"
> +postcopy_ram_fault_thread_request(unsigned long long hostaddr, const char *ramblock, size_t offset) "Request for HVA=%llx rb=%s offset=%zx"

Hmm - why?
That's called as:
        trace_postcopy_ram_fault_thread_request(msg.arg.pagefault.address,
                                                qemu_ram_get_idstr(rb),
                                                rb_offset);
    struct uffd_msg msg;
struct uffd_msg {
..
        union {
                struct {
                        __u64   flags;
                        __u64   address;
                } pagefault;
..
        } arg;
}

So why is a PRIx64 not the right way to print a __u64 ?
(I prefer %llx to the horrid PRIx64 syntax, but it still seems weird in this case)

Dave


>  postcopy_ram_incoming_cleanup_closeuf(void) ""
>  postcopy_ram_incoming_cleanup_entry(void) ""
>  postcopy_ram_incoming_cleanup_exit(void) ""
> -- 
> 2.9.3
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Eric Blake March 13, 2017, 8:18 p.m. UTC | #2
On 03/13/2017 03:07 PM, Dr. David Alan Gilbert wrote:
> * Eric Blake (eblake@redhat.com) wrote:
>> An upcoming patch will let the compiler warn us when we are silently
>> losing precision in traces; update the trace definitions to pass
>> through the full value at the callsite.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>>  migration/trace-events | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>

>> -postcopy_ram_fault_thread_request(uint64_t hostaddr, const char *ramblock, size_t offset) "Request for HVA=%" PRIx64 " rb=%s offset=%zx"
>> +postcopy_ram_fault_thread_request(unsigned long long hostaddr, const char *ramblock, size_t offset) "Request for HVA=%llx rb=%s offset=%zx"
> 
> Hmm - why?
> That's called as:
>         trace_postcopy_ram_fault_thread_request(msg.arg.pagefault.address,
>                                                 qemu_ram_get_idstr(rb),
>                                                 rb_offset);
>     struct uffd_msg msg;
> struct uffd_msg {
> ..
>         union {
>                 struct {
>                         __u64   flags;
>                         __u64   address;
>                 } pagefault;
> ..
>         } arg;
> }
> 
> So why is a PRIx64 not the right way to print a __u64 ?

Because __u64 is not the same type as uint64_t.  On 64-bit Linux, __u64
is 'unsigned long long', while uint64_t is 'unsigned long'.

> (I prefer %llx to the horrid PRIx64 syntax, but it still seems weird in this case)

As it is, I'm not sure if __u64 is always 'unsigned long long' in ALL
Linux clients; an even-more-conservative patch would be to switch all
callers to use explicit casts to something (like uint64_t or unsigned
long long) that we have full control over, rather than passing __u64
where we have no control over what type it ultimately resolves to.
Dr. David Alan Gilbert March 14, 2017, 11:32 a.m. UTC | #3
* Eric Blake (eblake@redhat.com) wrote:
> On 03/13/2017 03:07 PM, Dr. David Alan Gilbert wrote:
> > * Eric Blake (eblake@redhat.com) wrote:
> >> An upcoming patch will let the compiler warn us when we are silently
> >> losing precision in traces; update the trace definitions to pass
> >> through the full value at the callsite.
> >>
> >> Signed-off-by: Eric Blake <eblake@redhat.com>
> >> ---
> >>  migration/trace-events | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> 
> >> -postcopy_ram_fault_thread_request(uint64_t hostaddr, const char *ramblock, size_t offset) "Request for HVA=%" PRIx64 " rb=%s offset=%zx"
> >> +postcopy_ram_fault_thread_request(unsigned long long hostaddr, const char *ramblock, size_t offset) "Request for HVA=%llx rb=%s offset=%zx"
> > 
> > Hmm - why?
> > That's called as:
> >         trace_postcopy_ram_fault_thread_request(msg.arg.pagefault.address,
> >                                                 qemu_ram_get_idstr(rb),
> >                                                 rb_offset);
> >     struct uffd_msg msg;
> > struct uffd_msg {
> > ..
> >         union {
> >                 struct {
> >                         __u64   flags;
> >                         __u64   address;
> >                 } pagefault;
> > ..
> >         } arg;
> > }
> > 
> > So why is a PRIx64 not the right way to print a __u64 ?
> 
> Because __u64 is not the same type as uint64_t.  On 64-bit Linux, __u64
> is 'unsigned long long', while uint64_t is 'unsigned long'.
> 
> > (I prefer %llx to the horrid PRIx64 syntax, but it still seems weird in this case)
> 
> As it is, I'm not sure if __u64 is always 'unsigned long long' in ALL
> Linux clients; an even-more-conservative patch would be to switch all
> callers to use explicit casts to something (like uint64_t or unsigned
> long long) that we have full control over, rather than passing __u64
> where we have no control over what type it ultimately resolves to.

That would be my preference - casting to (uint64_t) at the caller and
keep this as PRIx64.  We know it's a 64bit value so we should never
use unsigned long long anywhere for it.

Dave

> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 



--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Eric Blake March 14, 2017, 2:36 p.m. UTC | #4
On 03/14/2017 06:32 AM, Dr. David Alan Gilbert wrote:
> * Eric Blake (eblake@redhat.com) wrote:
>> On 03/13/2017 03:07 PM, Dr. David Alan Gilbert wrote:
>>> * Eric Blake (eblake@redhat.com) wrote:
>>>> An upcoming patch will let the compiler warn us when we are silently
>>>> losing precision in traces; update the trace definitions to pass
>>>> through the full value at the callsite.
>>>>

>>>
>>> So why is a PRIx64 not the right way to print a __u64 ?
>>
>> Because __u64 is not the same type as uint64_t.  On 64-bit Linux, __u64
>> is 'unsigned long long', while uint64_t is 'unsigned long'.
>>
>>> (I prefer %llx to the horrid PRIx64 syntax, but it still seems weird in this case)
>>
>> As it is, I'm not sure if __u64 is always 'unsigned long long' in ALL
>> Linux clients; an even-more-conservative patch would be to switch all
>> callers to use explicit casts to something (like uint64_t or unsigned
>> long long) that we have full control over, rather than passing __u64
>> where we have no control over what type it ultimately resolves to.
> 
> That would be my preference - casting to (uint64_t) at the caller and
> keep this as PRIx64.  We know it's a 64bit value so we should never
> use unsigned long long anywhere for it.

Okay, my next version will insert an explicit cast in any caller that is
otherwise passing a __u64 (since we can't guarantee what type __u64
resolves to, and therefore what format string is appropriate for it).
Eric Blake March 15, 2017, 7:55 p.m. UTC | #5
On 03/14/2017 09:36 AM, Eric Blake wrote:

>>>> So why is a PRIx64 not the right way to print a __u64 ?
>>>
>>> Because __u64 is not the same type as uint64_t.  On 64-bit Linux, __u64
>>> is 'unsigned long long', while uint64_t is 'unsigned long'.
>>>
>>>> (I prefer %llx to the horrid PRIx64 syntax, but it still seems weird in this case)
>>>
>>> As it is, I'm not sure if __u64 is always 'unsigned long long' in ALL
>>> Linux clients; an even-more-conservative patch would be to switch all
>>> callers to use explicit casts to something (like uint64_t or unsigned
>>> long long) that we have full control over, rather than passing __u64
>>> where we have no control over what type it ultimately resolves to.
>>
>> That would be my preference - casting to (uint64_t) at the caller and
>> keep this as PRIx64.  We know it's a 64bit value so we should never
>> use unsigned long long anywhere for it.
> 
> Okay, my next version will insert an explicit cast in any caller that is
> otherwise passing a __u64 (since we can't guarantee what type __u64
> resolves to, and therefore what format string is appropriate for it).

Or maybe I will just omit those changes from my next version, in lieu of
a gcc bug report.  Here's my summary of an IRC conversation on the topic:

I just discovered that 32-bit mingw has a bug: it's <winsock.h> header
declared ntohl() as a function that returns 'u_long' ('unsigned long'),
but that POSIX requires ntohl() to return 'uin32_t' (which is 'unsigned
int' in that platform).  The only workaround to buggy system headers is
to insert casts at all call sites, or to quit using overly-picky
-Wformat warnings.

The gist of my gcc bug report will be that newer gcc recently introduced
a fine-tuning for -Wformat, namely -Wformat-signedness.  Some people
really do care about mismatches between "%d" and 'unsigned', or between
"%x" and 'int', but most of us don't.  When -Wformat was originally
invented, there was a lot of 32-bit code, and very little 64-bit code,
so it made TOTAL sense that you wanted to flag mismatches between "%d"
and 'long', or between "%ld" and 'int', as it was a porting aid to
64-bit platforms.  But these days, when we KNOW that a 32-bit platform
has 32-bit 'int' and 'long', and therefore that printf will behave
identically, it would be nice to squelch the warning, and instead only
issue it when there really is a mismatch (such as "%d" and 'long' on
64-bit).  If there were such a knob (call it -Wformat-rank), then in
qemu, we'd turn it off (-Wno-format-rank) so we can play fast and sloppy
with same-width but different-rank typedefs (such as the kernel's __u64
vs. uint64_t, or mingw's 'u_long' vs. 'uint32_t), but still get the full
benefit of real warnings when there is a 32-bit vs. 64-bit difference.
Eric Blake March 15, 2017, 10:33 p.m. UTC | #6
On 03/15/2017 02:55 PM, Eric Blake wrote:

>> Okay, my next version will insert an explicit cast in any caller that is
>> otherwise passing a __u64 (since we can't guarantee what type __u64
>> resolves to, and therefore what format string is appropriate for it).
> 
> Or maybe I will just omit those changes from my next version, in lieu of
> a gcc bug report.

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80060 if you are interested
diff mbox

Patch

diff --git a/migration/trace-events b/migration/trace-events
index 7372ce2..079d4e6 100644
--- a/migration/trace-events
+++ b/migration/trace-events
@@ -7,7 +7,7 @@  qemu_loadvm_state_section_partend(uint32_t section_id) "%u"
 qemu_loadvm_state_post_main(int ret) "%d"
 qemu_loadvm_state_section_startfull(uint32_t section_id, const char *idstr, uint32_t instance_id, uint32_t version_id) "%u(%s) %u %u"
 qemu_savevm_send_packaged(void) ""
-loadvm_handle_cmd_packaged(unsigned int length) "%u"
+loadvm_handle_cmd_packaged(size_t length) "%zu"
 loadvm_handle_cmd_packaged_main(int ret) "%d"
 loadvm_handle_cmd_packaged_received(int ret) "%d"
 loadvm_postcopy_handle_advise(void) ""
@@ -186,7 +186,7 @@  postcopy_ram_enable_notify(void) ""
 postcopy_ram_fault_thread_entry(void) ""
 postcopy_ram_fault_thread_exit(void) ""
 postcopy_ram_fault_thread_quit(void) ""
-postcopy_ram_fault_thread_request(uint64_t hostaddr, const char *ramblock, size_t offset) "Request for HVA=%" PRIx64 " rb=%s offset=%zx"
+postcopy_ram_fault_thread_request(unsigned long long hostaddr, const char *ramblock, size_t offset) "Request for HVA=%llx rb=%s offset=%zx"
 postcopy_ram_incoming_cleanup_closeuf(void) ""
 postcopy_ram_incoming_cleanup_entry(void) ""
 postcopy_ram_incoming_cleanup_exit(void) ""