| Message ID | 20170313195547.21466-7-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
* 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
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.
* 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
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).
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.
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 --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) ""
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(-)